Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Backport navigation rewrite changes #2846

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
32d17ee
backport navigation rewrite changes
andiwand Dec 20, 2023
31a37cf
sort candidates
andiwand Dec 20, 2023
f9eeffc
couple of things
andiwand Dec 20, 2023
040e1e9
use far limit again
andiwand Dec 20, 2023
ea79b4f
recover existing navigation fixes
andiwand Dec 21, 2023
c94f6b0
add todo
andiwand Dec 21, 2023
90fc498
pr feedback
andiwand Dec 21, 2023
9a57cb3
Merge branch 'main' of https://github.com/acts-project/acts into back…
andiwand Dec 22, 2023
09eed3a
fix after merging main
andiwand Dec 22, 2023
3d362c0
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 9, 2024
0892b89
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 22, 2024
4d6763d
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Jan 24, 2024
7b31ef5
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Feb 27, 2024
8a61d6d
Apply suggestions from code review
andiwand Feb 27, 2024
ea4c866
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Mar 15, 2024
108ae3f
pr feedback; revive nav test
andiwand Mar 15, 2024
af6a47b
Apply suggestions from code review
andiwand Mar 15, 2024
f7b77a4
Apply suggestions from code review
andiwand Mar 15, 2024
ca0ee8a
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand Mar 16, 2024
87d9209
update references
andiwand Mar 18, 2024
4f7a764
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand Apr 30, 2024
5f483bb
Merge branch 'main' into backport-navigation-rewrite-changes
andiwand May 16, 2024
5661306
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand May 22, 2024
0d02fa9
Merge branch 'main' of github.com:acts-project/acts into backport-nav…
andiwand May 24, 2024
7fbd681
reintroduce far limit constraint in `SteppingHelper`
andiwand May 24, 2024
d2f46cd
update references
andiwand May 24, 2024
8b80a34
Merge branch 'main' into backport-navigation-rewrite-changes
AJPfleger May 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions Core/include/Acts/Geometry/Layer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class VolumeBounds;
class TrackingVolume;
class ApproachDescriptor;
class IMaterialDecorator;
template <typename T>
struct NavigationOptions;

// Simple surface intersection
Expand Down Expand Up @@ -171,8 +170,7 @@ class Layer : public virtual GeometryObject {
/// @return list of intersection of surfaces on the layer
boost::container::small_vector<SurfaceIntersection, 10> compatibleSurfaces(
const GeometryContext& gctx, const Vector3& position,
const Vector3& direction,
const NavigationOptions<Surface>& options) const;
const Vector3& direction, const NavigationOptions& options) const;

/// Surface seen on approach
/// for layers without sub structure, this is the surfaceRepresentation
Expand All @@ -184,9 +182,10 @@ class Layer : public virtual GeometryObject {
/// @param options The navigation options
///
/// @return the Surface intersection of the approach surface
SurfaceIntersection surfaceOnApproach(
const GeometryContext& gctx, const Vector3& position,
const Vector3& direction, const NavigationOptions<Layer>& options) const;
SurfaceIntersection surfaceOnApproach(const GeometryContext& gctx,
const Vector3& position,
const Vector3& direction,
const NavigationOptions& options) const;

/// Fast navigation to next layer
///
Expand Down
7 changes: 3 additions & 4 deletions Core/include/Acts/Geometry/TrackingVolume.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ namespace Acts {

class GlueVolumesDescriptor;
class VolumeBounds;
template <typename object_t>
struct NavigationOptions;
class GeometryIdentifier;
class IMaterialDecorator;
Expand Down Expand Up @@ -204,7 +203,7 @@ class TrackingVolume : public Volume {
/// @return vector of compatible intersections with layers
boost::container::small_vector<LayerIntersection, 10> compatibleLayers(
const GeometryContext& gctx, const Vector3& position,
const Vector3& direction, const NavigationOptions<Layer>& options) const;
const Vector3& direction, const NavigationOptions& options) const;

/// @brief Returns all boundary surfaces sorted by the user.
///
Expand All @@ -220,7 +219,7 @@ class TrackingVolume : public Volume {
/// @return is the templated boundary intersection
boost::container::small_vector<BoundaryIntersection, 4> compatibleBoundaries(
const GeometryContext& gctx, const Vector3& position,
const Vector3& direction, const NavigationOptions<Surface>& options,
const Vector3& direction, const NavigationOptions& options,
const Logger& logger = getDummyLogger()) const;

/// @brief Return surfaces in given direction from bounding volume hierarchy
Expand All @@ -236,7 +235,7 @@ class TrackingVolume : public Volume {
std::vector<SurfaceIntersection> compatibleSurfacesFromHierarchy(
const GeometryContext& gctx, const Vector3& position,
const Vector3& direction, double angle,
const NavigationOptions<Surface>& options) const;
const NavigationOptions& options) const;

/// Return the associated sub Volume, returns THIS if no subVolume exists
///
Expand Down
15 changes: 1 addition & 14 deletions Core/include/Acts/Propagator/EigenStepper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,7 @@ class EigenStepper {
};

/// Constructor requires knowledge of the detector's magnetic field
EigenStepper(std::shared_ptr<const MagneticFieldProvider> bField,
double overstepLimit = 100 * UnitConstants::um);
EigenStepper(std::shared_ptr<const MagneticFieldProvider> bField);

State makeState(std::reference_wrapper<const GeometryContext> gctx,
std::reference_wrapper<const MagneticFieldContext> mctx,
Expand Down Expand Up @@ -307,15 +306,6 @@ class EigenStepper {
return state.stepSize.toString();
}

/// Overstep limit
///
/// @param state The stepping state (thread-local cache)
double overstepLimit(const State& state) const {
(void)state;
// A dynamic overstep limit could sit here
return -m_overstepLimit;
}

/// Create and return the bound state at the current position
///
/// @brief This transports (if necessary) the covariance
Expand Down Expand Up @@ -416,9 +406,6 @@ class EigenStepper {
protected:
/// Magnetic field inside of the detector
std::shared_ptr<const MagneticFieldProvider> m_bField;

/// Overstep limit
double m_overstepLimit;
};
} // namespace Acts

Expand Down
4 changes: 2 additions & 2 deletions Core/include/Acts/Propagator/EigenStepper.ipp
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@

template <typename E, typename A>
Acts::EigenStepper<E, A>::EigenStepper(
std::shared_ptr<const MagneticFieldProvider> bField, double overstepLimit)
: m_bField(std::move(bField)), m_overstepLimit(overstepLimit) {}
std::shared_ptr<const MagneticFieldProvider> bField)
: m_bField(std::move(bField)) {}

template <typename E, typename A>
auto Acts::EigenStepper<E, A>::makeState(
Expand Down
8 changes: 0 additions & 8 deletions Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -726,14 +726,6 @@ class MultiEigenStepperLoop
return ss.str();
}

/// Overstep limit
///
/// @param state [in] The stepping state (thread-local cache)
double overstepLimit(const State& state) const {
// A dynamic overstep limit could sit here
return SingleStepper::overstepLimit(state.components.front().state);
}

/// Create and return the bound state at the current position
///
/// @brief This transports (if necessary) the covariance
Expand Down
100 changes: 55 additions & 45 deletions Core/include/Acts/Propagator/Navigator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#pragma once

#include "Acts/Definitions/Units.hpp"
#include "Acts/Geometry/BoundarySurfaceT.hpp"
#include "Acts/Geometry/GeometryIdentifier.hpp"
#include "Acts/Geometry/Layer.hpp"
#include "Acts/Geometry/TrackingGeometry.hpp"
Expand All @@ -19,8 +18,6 @@
#include "Acts/Utilities/Logger.hpp"
#include "Acts/Utilities/StringHelpers.hpp"

#include <iomanip>
#include <iterator>
#include <sstream>
#include <string>

Expand All @@ -30,10 +27,6 @@ namespace Acts {

/// @brief struct for the Navigation options that are forwarded to
/// the geometry
///
/// @tparam propagator_state_t Type of the object for navigation state
/// @tparam object_t Type of the object for navigation to check against
template <typename object_t>
struct NavigationOptions {
/// The boundary check directive
BoundaryCheck boundaryCheck = BoundaryCheck(true);
Expand All @@ -46,10 +39,10 @@ struct NavigationOptions {
/// always look for passive
bool resolvePassive = false;

/// object to check against: at start
const object_t* startObject = nullptr;
/// object to check against: at end
const object_t* endObject = nullptr;
/// Hint for start object
const void* startObject = nullptr;
/// Hint for end object
const void* endObject = nullptr;
andiwand marked this conversation as resolved.
Show resolved Hide resolved

/// External surface identifier for which the boundary check is ignored
std::vector<GeometryIdentifier> externalSurfaces = {};
Expand Down Expand Up @@ -726,12 +719,13 @@ class Navigator {
// check if current volume has BVH, or layers
if (state.navigation.currentVolume->hasBoundingVolumeHierarchy()) {
// has hierarchy, use that, skip layer resolution
NavigationOptions<Surface> navOpts;
NavigationOptions navOpts;
navOpts.resolveSensitive = m_cfg.resolveSensitive;
navOpts.resolveMaterial = m_cfg.resolveMaterial;
navOpts.resolvePassive = m_cfg.resolvePassive;
navOpts.endObject = state.navigation.targetSurface;
navOpts.nearLimit = stepper.overstepLimit(state.stepping);
navOpts.nearLimit = state.options.surfaceTolerance;
navOpts.farLimit =
stepper.getStepSize(state.stepping, ConstrainedStep::aborter);
double opening_angle = 0;

// Preliminary version of the frustum opening angle estimation.
Expand Down Expand Up @@ -913,10 +907,10 @@ class Navigator {
// Helper function to find boundaries
auto findBoundaries = [&]() -> bool {
// The navigation options
NavigationOptions<Surface> navOpts;
NavigationOptions navOpts;
// Exclude the current surface in case it's a boundary
navOpts.startObject = state.navigation.currentSurface;
navOpts.nearLimit = stepper.overstepLimit(state.stepping);
navOpts.nearLimit = state.options.surfaceTolerance;
andiwand marked this conversation as resolved.
Show resolved Hide resolved
navOpts.farLimit =
stepper.getStepSize(state.stepping, ConstrainedStep::aborter);
navOpts.forceIntersectBoundaries =
Expand All @@ -933,7 +927,13 @@ class Navigator {
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping),
navOpts, logger());
// The number of boundary candidates
std::sort(state.navigation.navBoundaries.begin(),
state.navigation.navBoundaries.end(),
[](const auto& a, const auto& b) {
return SurfaceIntersection::forwardOrder(a.first, b.first);
andiwand marked this conversation as resolved.
Show resolved Hide resolved
});

// Print boundary information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navBoundaries.size();
Expand All @@ -943,6 +943,7 @@ class Navigator {
}
logger().log(Logging::VERBOSE, os.str());
}

// Set the begin index
state.navigation.navBoundaryIndex = 0;
if (!state.navigation.navBoundaries.empty()) {
Expand Down Expand Up @@ -1103,7 +1104,7 @@ class Navigator {
bool onStart = (navLayer == state.navigation.startLayer);
auto startSurface = onStart ? state.navigation.startSurface : layerSurface;
// Use navigation parameters and NavigationOptions
NavigationOptions<Surface> navOpts;
NavigationOptions navOpts;
navOpts.resolveSensitive = m_cfg.resolveSensitive;
navOpts.resolveMaterial = m_cfg.resolveMaterial;
navOpts.resolvePassive = m_cfg.resolvePassive;
Expand All @@ -1122,30 +1123,31 @@ class Navigator {
navOpts.externalSurfaces.push_back(itSurface->second);
}
}
// No overstepping on start layer, otherwise ask the stepper
navOpts.nearLimit = (cLayer != nullptr)
? state.options.surfaceTolerance
: stepper.overstepLimit(state.stepping);
// Check the limit
navOpts.nearLimit = state.options.surfaceTolerance;
navOpts.farLimit =
stepper.getStepSize(state.stepping, ConstrainedStep::aborter);

// get the surfaces
state.navigation.navSurfaces = navLayer->compatibleSurfaces(
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping), navOpts);
// the number of layer candidates
if (!state.navigation.navSurfaces.empty()) {
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navSurfaces.size();
os << " surface candidates found at path(s): ";
for (auto& sfc : state.navigation.navSurfaces) {
os << sfc.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
std::sort(state.navigation.navSurfaces.begin(),
state.navigation.navSurfaces.end(),
SurfaceIntersection::forwardOrder);
andiwand marked this conversation as resolved.
Show resolved Hide resolved

// Print surface information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navSurfaces.size();
os << " surface candidates found at path(s): ";
for (auto& sfc : state.navigation.navSurfaces) {
os << sfc.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}

// Surface candidates have been found
if (!state.navigation.navSurfaces.empty()) {
// set the index
state.navigation.navSurfaceIndex = 0;
// The stepper updates the step size ( single / multi component)
Expand All @@ -1155,6 +1157,7 @@ class Navigator {
<< stepper.outputStepSize(state.stepping));
return true;
}

state.navigation.navSurfaceIndex = state.navigation.navSurfaces.size();
ACTS_VERBOSE(volInfo(state) << "No surface candidates found.");
return false;
Expand Down Expand Up @@ -1186,34 +1189,41 @@ class Navigator {
: nullptr;
// Create the navigation options
// - and get the compatible layers, start layer will be excluded
NavigationOptions<Layer> navOpts;
NavigationOptions navOpts;
navOpts.boundaryCheck = m_cfg.boundaryCheckLayerResolving;
navOpts.resolveSensitive = m_cfg.resolveSensitive;
navOpts.resolveMaterial = m_cfg.resolveMaterial;
navOpts.resolvePassive = m_cfg.resolvePassive;
navOpts.startObject = startLayer;
navOpts.nearLimit = stepper.overstepLimit(state.stepping);
navOpts.nearLimit = state.options.surfaceTolerance;
navOpts.farLimit =
stepper.getStepSize(state.stepping, ConstrainedStep::aborter);

// Request the compatible layers
state.navigation.navLayers =
state.navigation.currentVolume->compatibleLayers(
state.geoContext, stepper.position(state.stepping),
state.options.direction * stepper.direction(state.stepping),
navOpts);
std::sort(state.navigation.navLayers.begin(),
state.navigation.navLayers.end(),
[](const auto& a, const auto& b) {
return SurfaceIntersection::forwardOrder(a.first, b.first);
andiwand marked this conversation as resolved.
Show resolved Hide resolved
});

// Print layer information
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navLayers.size();
os << " layer candidates found at path(s): ";
for (auto& lc : state.navigation.navLayers) {
os << lc.first.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}

// Layer candidates have been found
if (!state.navigation.navLayers.empty()) {
// Screen output where they are
if (logger().doPrint(Logging::VERBOSE)) {
std::ostringstream os;
os << state.navigation.navLayers.size();
os << " layer candidates found at path(s): ";
for (auto& lc : state.navigation.navLayers) {
os << lc.first.pathLength() << " ";
}
logger().log(Logging::VERBOSE, os.str());
}
// Set the index to the first
state.navigation.navLayerIndex = 0;
// Setting the step size towards first
Expand Down
15 changes: 6 additions & 9 deletions Core/include/Acts/Propagator/StandardAborters.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,10 @@ struct SurfaceReached {
/// Distance limit to discard intersections "behind us"
/// @note this is only necessary because some surfaces have more than one
/// intersection
std::optional<double> overrideNearLimit;
double nearLimit = -100 * UnitConstants::um;

SurfaceReached() = default;
SurfaceReached(double nearLimit) : overrideNearLimit(nearLimit) {}
SurfaceReached(double nLimit) : nearLimit(nLimit) {}

/// boolean operator for abort condition without using the result
///
Expand All @@ -106,14 +106,11 @@ struct SurfaceReached {
return true;
}

// not blindly using the stepper overstep limit here because it does not
// always work for perigee surfaces.
// not using the stepper overstep limit here because it does not always work
// for perigee surfaces
// note: the near limit is necessary for surfaces with more than one
// intersections in order to discard the ones which are behind us.
const double nearLimit =
overrideNearLimit.value_or(stepper.overstepLimit(state.stepping));
const double farLimit =
state.stepping.stepSize.value(ConstrainedStep::aborter);
// intersections in order to discard the ones which are behind us
andiwand marked this conversation as resolved.
Show resolved Hide resolved
const double farLimit = std::numeric_limits<double>::max();
const double tolerance = state.options.surfaceTolerance;

const auto sIntersection = surface->intersect(
Expand Down
3 changes: 0 additions & 3 deletions Core/include/Acts/Propagator/StepperConcept.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ METHOD_TRAIT(absolute_momentum_t, absoluteMomentum);
METHOD_TRAIT(momentum_t, momentum);
METHOD_TRAIT(charge_t, charge);
METHOD_TRAIT(time_t, time);
METHOD_TRAIT(overstep_t, overstepLimit);
METHOD_TRAIT(bound_state_method_t, boundState);
METHOD_TRAIT(curvilinear_state_method_t, curvilinearState);
METHOD_TRAIT(update_t, update);
Expand Down Expand Up @@ -115,8 +114,6 @@ constexpr bool MultiStepperStateConcept= require<
static_assert(charge_exists, "charge method not found");
constexpr static bool time_exists = has_method<const S, double, time_t, const state&>;
static_assert(time_exists, "time method not found");
constexpr static bool overstep_exists = has_method<const S, double, overstep_t, const state&>;
static_assert(overstep_exists, "overstepLimit method not found");
constexpr static bool bound_state_method_exists= has_method<const S, Result<typename S::BoundState>, bound_state_method_t, state&, const Surface&, bool, const FreeToBoundCorrection&>;
static_assert(bound_state_method_exists, "boundState method not found");
constexpr static bool curvilinear_state_method_exists = has_method<const S, typename S::CurvilinearState, curvilinear_state_method_t, state&, bool>;
Expand Down
Loading
Loading