From 9bfcfc16897eb6e3fc5e45f932128419ab966600 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 5 Dec 2023 14:53:02 +0100 Subject: [PATCH] refactor: Remove `representation` from `ObjectIntersection` (#2760) As the templating of the representing object was rather a burden in https://github.com/acts-project/acts/pull/2625 I propose to remove it all together and compose the intersection if necessary. Here I used a `std::pair` in the `Navigator` for composition for now which may be not optimal but it works as it should for now --- Core/include/Acts/Geometry/TrackingVolume.hpp | 10 ++- .../Acts/Propagator/MultiEigenStepperLoop.hpp | 2 +- Core/include/Acts/Propagator/Navigator.hpp | 43 ++++++---- Core/include/Acts/Utilities/Intersection.hpp | 81 ++++--------------- Core/src/Geometry/TrackingVolume.cpp | 24 +++--- .../Core/Propagator/NavigatorTests.cpp | 2 +- 6 files changed, 63 insertions(+), 99 deletions(-) diff --git a/Core/include/Acts/Geometry/TrackingVolume.hpp b/Core/include/Acts/Geometry/TrackingVolume.hpp index df88e9f065c..62ef13e74dd 100644 --- a/Core/include/Acts/Geometry/TrackingVolume.hpp +++ b/Core/include/Acts/Geometry/TrackingVolume.hpp @@ -69,17 +69,19 @@ using LayerArray = BinnedArray; using LayerVector = std::vector; /// Intersection with @c Layer -using LayerIntersection = ObjectIntersection; +using LayerIntersection = std::pair; /// Multi-intersection with @c Layer -using LayerMultiIntersection = ObjectMultiIntersection; +using LayerMultiIntersection = + std::pair; /// BoundarySurface of a volume using BoundarySurface = BoundarySurfaceT; /// Intersection with a @c BoundarySurface -using BoundaryIntersection = ObjectIntersection; +using BoundaryIntersection = + std::pair; /// Multi-intersection with a @c BoundarySurface using BoundaryMultiIntersection = - ObjectMultiIntersection; + std::pair; /// @class TrackingVolume /// diff --git a/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp b/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp index 2e9cb6e206f..a49176d0297 100644 --- a/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp +++ b/Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp @@ -662,7 +662,7 @@ class MultiEigenStepperLoop template void updateStepSize(State& state, const object_intersection_t& oIntersection, Direction direction, bool release = true) const { - const Surface& surface = *oIntersection.representation(); + const Surface& surface = *oIntersection.object(); for (auto& component : state.components) { auto intersection = surface.intersect( diff --git a/Core/include/Acts/Propagator/Navigator.hpp b/Core/include/Acts/Propagator/Navigator.hpp index 46126777d01..158f0a7f185 100644 --- a/Core/include/Acts/Propagator/Navigator.hpp +++ b/Core/include/Acts/Propagator/Navigator.hpp @@ -525,7 +525,7 @@ class Navigator { state.navigation.lastHierarchySurfaceReached = false; // Update volume information // get the attached volume information - auto boundary = state.navigation.navBoundary().object(); + auto boundary = state.navigation.navBoundary().second; state.navigation.currentVolume = boundary->attachedVolume( state.geoContext, stepper.position(state.stepping), stepper.direction(state.stepping), state.options.direction); @@ -568,6 +568,19 @@ class Navigator { } private: + const SurfaceIntersection& candidateIntersection( + const NavigationSurfaces& surfaces, std::size_t index) const { + return surfaces.at(index); + } + const SurfaceIntersection& candidateIntersection( + const NavigationLayers& surfaces, std::size_t index) const { + return surfaces.at(index).first; + } + const SurfaceIntersection& candidateIntersection( + const NavigationBoundaries& surfaces, std::size_t index) const { + return surfaces.at(index).first; + } + /// @brief Status call for test surfaces (surfaces, layers, boundaries) /// /// If there are surfaces to be handled, check if the current @@ -592,9 +605,9 @@ class Navigator { if (navSurfaces.empty() || navIndex == navSurfaces.size()) { return false; } - const auto& intersection = navSurfaces.at(navIndex); + const auto& intersection = candidateIntersection(navSurfaces, navIndex); // Take the current surface - auto surface = intersection.representation(); + const auto* surface = intersection.object(); // Check if we are at a surface // If we are on the surface pointed at by the index, we can make // it the current one to pass it to the other actors @@ -844,9 +857,9 @@ class Navigator { // loop over the available navigation layer candidates while (state.navigation.navLayerIndex != state.navigation.navLayers.size()) { - const auto& intersection = state.navigation.navLayer(); + const auto& intersection = state.navigation.navLayer().first; // The layer surface - const auto* layerSurface = intersection.representation(); + const auto* layerSurface = intersection.object(); // We are on the layer if (state.navigation.currentSurface == layerSurface) { ACTS_VERBOSE(volInfo(state) << "We are on a layer, resolve Surfaces."); @@ -976,7 +989,7 @@ class Navigator { os << state.navigation.navBoundaries.size(); os << " boundary candidates found at path(s): "; for (auto& bc : state.navigation.navBoundaries) { - os << bc.pathLength() << " "; + os << bc.first.pathLength() << " "; } logger().log(Logging::VERBOSE, os.str()); } @@ -984,7 +997,8 @@ class Navigator { state.navigation.navBoundaryIndex = 0; if (!state.navigation.navBoundaries.empty()) { // Set to the first and return to the stepper - stepper.updateStepSize(state.stepping, state.navigation.navBoundary(), + stepper.updateStepSize(state.stepping, + state.navigation.navBoundary().first, state.options.direction, true); ACTS_VERBOSE(volInfo(state) << "Navigation stepSize updated to " << stepper.outputStepSize(state.stepping)); @@ -1001,9 +1015,9 @@ class Navigator { // Loop over the boundary surface while (state.navigation.navBoundaryIndex != state.navigation.navBoundaries.size()) { - const auto& intersection = state.navigation.navBoundary(); + const auto& intersection = state.navigation.navBoundary().first; // That is the current boundary surface - const auto* boundarySurface = intersection.representation(); + const auto* boundarySurface = intersection.object(); // Step towards the boundary surfrace auto boundaryStatus = stepper.updateSurfaceStatus( state.stepping, *boundarySurface, intersection.index(), @@ -1133,8 +1147,8 @@ class Navigator { const Layer* cLayer = nullptr) const { // get the layer and layer surface auto layerSurface = cLayer ? state.navigation.startSurface - : state.navigation.navLayer().representation(); - auto navLayer = cLayer ? cLayer : state.navigation.navLayer().object(); + : state.navigation.navLayer().first.object(); + auto navLayer = cLayer ? cLayer : state.navigation.navLayer().second; // are we on the start layer bool onStart = (navLayer == state.navigation.startLayer); auto startSurface = onStart ? state.navigation.startSurface : layerSurface; @@ -1248,7 +1262,7 @@ class Navigator { os << state.navigation.navLayers.size(); os << " layer candidates found at path(s): "; for (auto& lc : state.navigation.navLayers) { - os << lc.pathLength() << " "; + os << lc.first.pathLength() << " "; } logger().log(Logging::VERBOSE, os.str()); } @@ -1256,10 +1270,11 @@ class Navigator { state.navigation.navLayerIndex = 0; // Setting the step size towards first if (state.navigation.startLayer && - state.navigation.navLayer().object() != state.navigation.startLayer) { + state.navigation.navLayer().second != state.navigation.startLayer) { ACTS_VERBOSE(volInfo(state) << "Target at layer."); // The stepper updates the step size ( single / multi component) - stepper.updateStepSize(state.stepping, state.navigation.navLayer(), + stepper.updateStepSize(state.stepping, + state.navigation.navLayer().first, state.options.direction, true); ACTS_VERBOSE(volInfo(state) << "Navigation stepSize updated to " << stepper.outputStepSize(state.stepping)); diff --git a/Core/include/Acts/Utilities/Intersection.hpp b/Core/include/Acts/Utilities/Intersection.hpp index a4ef4990d52..2f82b531532 100644 --- a/Core/include/Acts/Utilities/Intersection.hpp +++ b/Core/include/Acts/Utilities/Intersection.hpp @@ -117,24 +117,9 @@ using MultiIntersection3D = boost::container::static_vector; -/// @brief class extensions to return also the object and a representation -template +template class ObjectIntersection { public: - /// Object intersection - symmetric setup - /// - /// @param intersection is the intersection - /// @param object is the object to be instersected - /// @param index is the intersection index - template ::value, int> = 0> - constexpr ObjectIntersection(const Intersection3D& intersection, - const object_t* object, std::uint8_t index = 0) - : m_intersection(intersection), - m_object(object), - m_representation(object), - m_index(index) {} - /// Object intersection /// /// @param intersection is the intersection @@ -142,13 +127,8 @@ class ObjectIntersection { /// @param representation is the object representation /// @param index is the intersection index constexpr ObjectIntersection(const Intersection3D& intersection, - const object_t* object, - const representation_t* representation, - std::uint8_t index = 0) - : m_intersection(intersection), - m_object(object), - m_representation(representation), - m_index(index) {} + const object_t* object, std::uint8_t index = 0) + : m_intersection(intersection), m_object(object), m_index(index) {} /// Returns whether the intersection was successful or not constexpr explicit operator bool() const { @@ -173,10 +153,6 @@ class ObjectIntersection { constexpr const object_t* object() const { return m_object; } - constexpr const representation_t* representation() const { - return m_representation; - } - constexpr std::uint8_t index() const { return m_index; } constexpr static ObjectIntersection invalid() { return ObjectIntersection(); } @@ -198,33 +174,18 @@ class ObjectIntersection { Intersection3D m_intersection = Intersection3D::invalid(); /// The object that was (tried to be) intersected const object_t* m_object = nullptr; - /// The representation of this object - const representation_t* m_representation = nullptr; /// The intersection index std::uint8_t m_index = 0; constexpr ObjectIntersection() = default; }; -/// @brief class extensions to return also the object and a representation -template +template class ObjectMultiIntersection { public: - using SplitIntersections = boost::container::static_vector< - ObjectIntersection, - s_maximumNumberOfIntersections>; - - /// Object intersection - symmetric setup - /// - /// @param intersections are the intersections - /// @param object is the object to be instersected - template ::value, int> = 0> - constexpr ObjectMultiIntersection(const MultiIntersection3D& intersections, - const object_t* object) - : m_intersections(intersections), - m_object(object), - m_representation(object) {} + using SplitIntersections = + boost::container::static_vector, + s_maximumNumberOfIntersections>; /// Object intersection /// @@ -232,25 +193,17 @@ class ObjectMultiIntersection { /// @param object is the object to be instersected /// @param representation is the object representation constexpr ObjectMultiIntersection(const MultiIntersection3D& intersections, - const object_t* object, - const representation_t* representation) - : m_intersections(intersections), - m_object(object), - m_representation(representation) {} - - constexpr ObjectIntersection operator[]( - std::uint8_t index) const { - return {m_intersections[index], m_object, m_representation, index}; + const object_t* object) + : m_intersections(intersections), m_object(object) {} + + constexpr ObjectIntersection operator[](std::uint8_t index) const { + return {m_intersections[index], m_object, index}; } constexpr std::size_t size() const { return m_intersections.size(); } constexpr const object_t* object() const { return m_object; } - constexpr const representation_t* representation() const { - return m_representation; - } - constexpr SplitIntersections split() const { SplitIntersections result; for (std::size_t i = 0; i < size(); ++i) { @@ -259,11 +212,11 @@ class ObjectMultiIntersection { return result; } - constexpr ObjectIntersection closest() const { + constexpr ObjectIntersection closest() const { auto splitIntersections = split(); - return *std::min_element( - splitIntersections.begin(), splitIntersections.end(), - ObjectIntersection::closestOrder); + return *std::min_element(splitIntersections.begin(), + splitIntersections.end(), + ObjectIntersection::closestOrder); } private: @@ -271,8 +224,6 @@ class ObjectMultiIntersection { MultiIntersection3D m_intersections; /// The object that was (tried to be) intersected const object_t* m_object = nullptr; - /// The representation of this object - const representation_t* m_representation = nullptr; }; namespace detail { diff --git a/Core/src/Geometry/TrackingVolume.cpp b/Core/src/Geometry/TrackingVolume.cpp index af9f111446e..bc282f551b2 100644 --- a/Core/src/Geometry/TrackingVolume.cpp +++ b/Core/src/Geometry/TrackingVolume.cpp @@ -504,9 +504,7 @@ Acts::TrackingVolume::compatibleBoundaries( ACTS_VERBOSE("- intersection path length " << std::abs(sIntersection.pathLength()) << " < overstep limit " << std::abs(oLimit)); - return BoundaryIntersection(sIntersection.intersection(), bSurface, - sIntersection.object(), - sIntersection.index()); + return BoundaryIntersection(sIntersection, bSurface); } ACTS_VERBOSE("Can't force intersection: "); ACTS_VERBOSE("- intersection path length " @@ -520,14 +518,12 @@ Acts::TrackingVolume::compatibleBoundaries( decltype(logger)>( sIntersection.intersection(), pLimit, oLimit, s_onSurfaceTolerance, logger)) { - return BoundaryIntersection(sIntersection.intersection(), bSurface, - sIntersection.object(), - sIntersection.index()); + return BoundaryIntersection(sIntersection, bSurface); } } ACTS_VERBOSE("No intersection accepted"); - return BoundaryIntersection::invalid(); + return BoundaryIntersection(SurfaceIntersection::invalid(), bSurface); }; /// Helper function to process boundary surfaces @@ -548,7 +544,7 @@ Acts::TrackingVolume::compatibleBoundaries( options.boundaryCheck); // Intersect and continue auto bIntersection = checkIntersection(bCandidate, bsIter.get()); - if (bIntersection) { + if (bIntersection.first) { ACTS_VERBOSE(" - Proceed with surface"); bIntersections.push_back(bIntersection); } else { @@ -587,8 +583,8 @@ Acts::TrackingVolume::compatibleBoundaries( }; std::sort(bIntersections.begin(), bIntersections.end(), - [&](const auto& a, const auto& b) { - return comparator(a.pathLength(), b.pathLength()); + [&](const BoundaryIntersection& a, const BoundaryIntersection& b) { + return comparator(a.first.pathLength(), b.first.pathLength()); }); return bIntersections; } @@ -623,9 +619,7 @@ Acts::TrackingVolume::compatibleLayers( if (atIntersection && (atIntersection.object() != options.targetSurface) && withinLimit) { // create a layer intersection - lIntersections.push_back(LayerIntersection( - atIntersection.intersection(), tLayer, atIntersection.object(), - atIntersection.index())); + lIntersections.push_back(LayerIntersection(atIntersection, tLayer)); } } // move to next one or break because you reached the end layer @@ -634,7 +628,9 @@ Acts::TrackingVolume::compatibleLayers( : tLayer->nextLayer(gctx, position, direction); } std::sort(lIntersections.begin(), lIntersections.end(), - LayerIntersection::forwardOrder); + [](const LayerIntersection& a, const LayerIntersection& b) { + return SurfaceIntersection::forwardOrder(a.first, b.first); + }); } // and return return lIntersections; diff --git a/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp b/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp index 96d9010efd3..9d507d5dbc5 100644 --- a/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp +++ b/Tests/UnitTests/Core/Propagator/NavigatorTests.cpp @@ -508,7 +508,7 @@ BOOST_AUTO_TEST_CASE(Navigator_target_methods) { // The index should points to the begin BOOST_CHECK_EQUAL(state.navigation.navLayerIndex, 0); // Cache the beam pipe radius - double beamPipeR = perp(state.navigation.navLayer().position()); + double beamPipeR = perp(state.navigation.navLayer().first.position()); // step size has been updated CHECK_CLOSE_ABS(state.stepping.stepSize.value(), beamPipeR, s_onSurfaceTolerance);