From 7eb13e8a0f24cf360a370272718bae066f463757 Mon Sep 17 00:00:00 2001 From: Andreas Salzburger Date: Thu, 1 Aug 2024 15:19:01 +0200 Subject: [PATCH] feat: splitting fill and update function (#3465) Thi PR splits the `fill(...)` and `update(...)` function or `NavigationStateUpdators` of the `Gen2` geometry. This avoid uneccesary copying and multiple intersections in case of chained updaters. --- .../Acts/Navigation/InternalNavigation.hpp | 110 ++++++++----- .../Acts/Navigation/MultiLayerNavigation.hpp | 19 ++- .../Navigation/NavigationStateUpdaters.hpp | 145 +++++++++++++----- .../Navigation/DetectorNavigatorTests.cpp | 2 +- 4 files changed, 192 insertions(+), 84 deletions(-) diff --git a/Core/include/Acts/Navigation/InternalNavigation.hpp b/Core/include/Acts/Navigation/InternalNavigation.hpp index 4301676e1c5..75f8ec87ea9 100644 --- a/Core/include/Acts/Navigation/InternalNavigation.hpp +++ b/Core/include/Acts/Navigation/InternalNavigation.hpp @@ -25,49 +25,51 @@ namespace Acts::Experimental { struct AllPortalsNavigation : public IInternalNavigation { - /// A ordered portal provider + /// Fills all portals into the navigation state /// /// @param gctx is the Geometry context of this call /// @param nState is the navigation state to be updated /// - /// @note that the intersections are ordered, such that the - /// smallest intersection pathlength >= overstep tolerance is the lowest - /// - /// @return an ordered list of portal candidates - inline void update(const GeometryContext& gctx, - NavigationState& nState) const { + /// @note no intersection ordering is done at this stage + inline void fill([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { if (nState.currentVolume == nullptr) { throw std::runtime_error( "AllPortalsNavigation: no detector volume set to navigation state."); } - // Retrieval necessary - if (nState.surfaceCandidates.empty()) { - // Fill internal portals if existing - for (const auto v : nState.currentVolume->volumes()) { - const auto& iPortals = v->portals(); - PortalsFiller::fill(nState, iPortals); - } - // Filling the new portal candidates - const auto& portals = nState.currentVolume->portals(); - PortalsFiller::fill(nState, portals); + // Fill internal portals if existing + for (const auto v : nState.currentVolume->volumes()) { + const auto& iPortals = v->portals(); + PortalsFiller::fill(nState, iPortals); } - // Sort and update - updateCandidates(gctx, nState); + // Filling the new portal candidates + const auto& portals = nState.currentVolume->portals(); + PortalsFiller::fill(nState, portals); } -}; -struct AllPortalsAndSurfacesNavigation : public IInternalNavigation { - /// An ordered list of portals and surfaces provider + /// A ordered portal provider - update method that calls fill and initialize /// /// @param gctx is the Geometry context of this call /// @param nState is the navigation state to be updated /// /// @note that the intersections are ordered, such that the /// smallest intersection pathlength >= overstep tolerance is the lowest - /// - /// @return an ordered list of portal and surface candidates inline void update(const GeometryContext& gctx, NavigationState& nState) const { + fill(gctx, nState); + intitializeCandidates(gctx, nState); + } +}; + +struct AllPortalsAndSurfacesNavigation : public IInternalNavigation { + /// Fills all portals and surfaces into the navigation state + /// + /// @param gctx is the Geometry context of this call + /// @param nState is the navigation state to be updated + /// + /// @note no intersection ordering is done at this stage + inline void fill([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { if (nState.currentDetector == nullptr) { throw std::runtime_error( "AllPortalsAndSurfacesNavigation: no detector volume set to " @@ -75,20 +77,31 @@ struct AllPortalsAndSurfacesNavigation : public IInternalNavigation { "state."); } // A volume switch has happened, update list of portals & surfaces - if (nState.surfaceCandidates.empty()) { - // Fill internal portals if existing - for (const auto v : nState.currentVolume->volumes()) { - const auto& iPortals = v->portals(); - PortalsFiller::fill(nState, iPortals); - } - // Assign the new volume - const auto& portals = nState.currentVolume->portals(); - const auto& surfaces = nState.currentVolume->surfaces(); - PortalsFiller::fill(nState, portals); - SurfacesFiller::fill(nState, surfaces); + for (const auto v : nState.currentVolume->volumes()) { + const auto& iPortals = v->portals(); + PortalsFiller::fill(nState, iPortals); } - // Update internal candidates - updateCandidates(gctx, nState); + // Assign the new volume + const auto& portals = nState.currentVolume->portals(); + const auto& surfaces = nState.currentVolume->surfaces(); + PortalsFiller::fill(nState, portals); + SurfacesFiller::fill(nState, surfaces); + } + + /// A ordered list of portals and surfaces provider + /// - update method that calls fill and initialize + /// + /// @param gctx is the Geometry context of this call + /// @param nState is the navigation state to be updated + /// + /// @note that the intersections are ordered, such that the + /// smallest intersection pathlength >= overstep tolerance is the lowest + /// + /// @return an ordered list of portal and surface candidates + inline void update(const GeometryContext& gctx, + NavigationState& nState) const { + fill(gctx, nState); + intitializeCandidates(gctx, nState); } }; @@ -124,15 +137,32 @@ struct AdditionalSurfacesNavigation : public IInternalNavigation { /// The volumes held by this collection std::vector surfaces = {}; - /// Extract the sub volumes from the volume + /// Extract the additional surfaces from the this volume /// /// @param gctx the geometry contextfor this extraction call (ignored) /// @param nState is the navigation state /// - inline void update([[maybe_unused]] const GeometryContext& gctx, - NavigationState& nState) const { + /// @note no intersection ordering is done at this stage + inline void fill([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { SurfacesFiller::fill(nState, surfaces); } + + /// Extract the additional surfaces from the this volume + /// - update method that calls fill and initialize + /// + /// @param gctx is the Geometry context of this call + /// @param nState is the navigation state to be updated + /// + /// @note that the intersections are ordered, such that the + /// smallest intersection pathlength >= overstep tolerance is the lowest + /// + /// @return an ordered list of portal and surface candidates + inline void update(const GeometryContext& gctx, + NavigationState& nState) const { + fill(gctx, nState); + intitializeCandidates(gctx, nState); + } }; /// @brief An indexed surface implementation access diff --git a/Core/include/Acts/Navigation/MultiLayerNavigation.hpp b/Core/include/Acts/Navigation/MultiLayerNavigation.hpp index a18e8c438cc..15042881147 100644 --- a/Core/include/Acts/Navigation/MultiLayerNavigation.hpp +++ b/Core/include/Acts/Navigation/MultiLayerNavigation.hpp @@ -48,10 +48,13 @@ class MultiLayerNavigation : public IInternalNavigation { MultiLayerNavigation() = delete; - /// Update the navigation state + /// Fill the navigation state + /// + /// @note no initialization is done here (sorting and update) + /// /// @param gctx is the geometry context /// @param nState is the navigation state - void update(const GeometryContext& gctx, NavigationState& nState) const { + void fill(const GeometryContext& gctx, NavigationState& nState) const { // get the local position and direction auto lposition = transform * nState.position; auto ldirection = transform.linear() * nState.direction; @@ -72,8 +75,16 @@ class MultiLayerNavigation : public IInternalNavigation { resolveDuplicates(gctx, surfCandidates); SurfacesFiller::fill(nState, surfCandidates); - - updateCandidates(gctx, nState); + } + /// Fill the update the navigation state with candidates + /// + /// @note initialization is done here (sorting and update) + /// + /// @param gctx is the geometry context + /// @param nState is the navigation state + void update(const GeometryContext& gctx, NavigationState& nState) const { + fill(gctx, nState); + intitializeCandidates(gctx, nState); } /// Cast into a lookup position diff --git a/Core/include/Acts/Navigation/NavigationStateUpdaters.hpp b/Core/include/Acts/Navigation/NavigationStateUpdaters.hpp index 8bd00d6a406..3903d1e123c 100644 --- a/Core/include/Acts/Navigation/NavigationStateUpdaters.hpp +++ b/Core/include/Acts/Navigation/NavigationStateUpdaters.hpp @@ -34,36 +34,45 @@ namespace Acts::Experimental { /// @param nState [in,out] is the navigation state to be updated /// /// @todo for surfaces skip the non-reached ones, while keep for portals -inline void updateCandidates(const GeometryContext& gctx, - NavigationState& nState) { +inline void intitializeCandidates(const GeometryContext& gctx, + NavigationState& nState) { const auto& position = nState.position; const auto& direction = nState.direction; - NavigationState::SurfaceCandidates nextSurfaceCandidates; + nState.surfaceCandidateIndex = 0; - for (NavigationState::SurfaceCandidate c : nState.surfaceCandidates) { - // Get the surface representation: either native surface of portal - const Surface& sRep = - c.surface != nullptr ? *c.surface : c.portal->surface(); + NavigationState::SurfaceCandidates confirmedCandidates; + confirmedCandidates.reserve(nState.surfaceCandidates.size()); + for (auto& sc : nState.surfaceCandidates) { + // Get the surface representation: either native surface of portal + const Surface& surface = + sc.surface != nullptr ? *sc.surface : sc.portal->surface(); // Only allow overstepping if it's not a portal ActsScalar overstepTolerance = - c.portal != nullptr ? s_onSurfaceTolerance : nState.overstepTolerance; - - // Get the intersection @todo make a templated intersector - // TODO surface tolerance - auto sIntersection = sRep.intersect( - gctx, position, direction, c.boundaryTolerance, s_onSurfaceTolerance); + sc.portal != nullptr ? s_onSurfaceTolerance : nState.overstepTolerance; + // Boundary tolerance is forced to 0 for portals + BoundaryTolerance boundaryTolerance = + sc.portal != nullptr ? BoundaryTolerance::None() : sc.boundaryTolerance; + // Check the surface intersection + auto sIntersection = surface.intersect( + gctx, position, direction, boundaryTolerance, s_onSurfaceTolerance); for (auto& si : sIntersection.split()) { - c.objectIntersection = si; - if (c.objectIntersection.isValid() && - c.objectIntersection.pathLength() > overstepTolerance) { - nextSurfaceCandidates.emplace_back(c); + if (si.isValid() && si.pathLength() > overstepTolerance) { + confirmedCandidates.emplace_back(NavigationState::SurfaceCandidate{ + si, sc.surface, sc.portal, boundaryTolerance}); } } } - nState.surfaceCandidates = std::move(nextSurfaceCandidates); + std::sort(confirmedCandidates.begin(), confirmedCandidates.end(), + [&](const auto& a, const auto& b) { + return a.objectIntersection.pathLength() < + b.objectIntersection.pathLength(); + }); + + nState.surfaceCandidates = std::move(confirmedCandidates); + nState.surfaceCandidateIndex = 0; } /// @brief This sets a single object, e.g. single surface or single volume @@ -84,15 +93,32 @@ class SingleObjectNavigation : public navigation_type { } } - /// @brief updates the navigation state with a single object that is filled in + /// @brief Fill the navigation state with a single object that it holds /// /// @param gctx is the Geometry context of this call /// @param nState the navigation state to which the surfaces are attached /// /// @note this is attaching objects without intersecting nor checking + void fill([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { + filler_type::fill(nState, m_object); + } + + /// @brief Update the navigation state with a single object that it holds + /// + /// @note it calls fill and then initializes the candidates (including intersection) + /// + /// @param gctx is the Geometry context of this call + /// @param nState the navigation state to which the surfaces are attached + /// + /// @note this is attaching objects and will perform a check void update([[maybe_unused]] const GeometryContext& gctx, NavigationState& nState) const { - filler_type::fill(nState, m_object); + fill(gctx, nState); + // If the delegate type is of type IInternalNavigation + if constexpr (std::is_base_of_v) { + intitializeCandidates(gctx, nState); + } } /// Const Access to the object @@ -103,7 +129,7 @@ class SingleObjectNavigation : public navigation_type { const object_type* m_object = nullptr; }; -/// @brief This uses state less extractor and fillers to manipulate +/// @brief This uses stateless extractor and fillers to manipulate /// the navigation state /// /// @tparam navigation_type distinguishes between internal and external navigation @@ -114,17 +140,34 @@ template class StaticAccessNavigation : public navigation_type { public: - /// @brief updates the navigation state with a single object that is filled in + /// @brief fills the navigation state with extracted objects /// /// @param gctx is the Geometry context of this call /// @param nState the navigation state to which the surfaces are attached /// /// @note this is attaching objects without intersecting nor checking - void update([[maybe_unused]] const GeometryContext& gctx, - NavigationState& nState) const { + void fill([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { auto extracted = extractor_type::extract(gctx, nState); filler_type::fill(nState, extracted); } + + /// @brief Update the navigation state with extracted objects + /// + /// @note it calls fill and then initializes the candidates (including intersection) + /// + /// @param gctx is the Geometry context of this call + /// @param nState the navigation state to which the surfaces are attached + /// + /// @note this will perform the intersection test + void update([[maybe_unused]] const GeometryContext& gctx, + NavigationState& nState) const { + fill(gctx, nState); + // If the delegate type is of type IInternalNavigation + if constexpr (std::is_base_of_v) { + intitializeCandidates(gctx, nState); + } + } }; /// @brief This is an index grid based navigation state updator, it uses @@ -139,7 +182,6 @@ class StaticAccessNavigation : public navigation_type { /// @tparam grid_t is the type of the grid /// @tparam extractor_type is the helper to extract the object /// @tparam filler_type is the helper to fill the object into the nState -/// template class IndexedGridNavigation : public navigation_type { @@ -170,32 +212,43 @@ class IndexedGridNavigation : public navigation_type { IndexedGridNavigation() = delete; - /// @brief updates the navigation state with objects from the grid according - /// to the filling type AFTER applying `p3loc = transform * p3` + /// @brief fill the navigation state with objects from the grid entries + /// AFTER applying `p3loc = transform * p3` and casting to subsbpace /// /// @param gctx is the Geometry context of this call /// @param nState the navigation state to which the surfaces are attached /// - /// @note this is attaching objects without intersecting nor checking - void update(const GeometryContext& gctx, NavigationState& nState) const { - // Extract the index grid entry + /// @note this will only fill the state without intersecting + void fill(const GeometryContext& gctx, NavigationState& nState) const { + // Extract the index grid entry a const auto& entry = grid.atPosition(GridAccessHelpers::castPosition( transform * nState.position, casts)); auto extracted = extractor.extract(gctx, nState, entry); filler_type::fill(nState, extracted); + } + /// @brief Update the navigation state with objects from the entries + /// AFTER applying `p3loc = transform * p3` and casting to subsbpace + /// + /// @note it calls fill and then initializes the candidates (including intersection) + /// + /// @param gctx is the Geometry context of this call + /// @param nState the navigation state to which the surfaces are attached + /// + /// @note this will perform the intersection test for internal navigation paths + void update(const GeometryContext& gctx, NavigationState& nState) const { + fill(gctx, nState); // If the delegate type is of type IInternalNavigation if constexpr (std::is_base_of_v) { - // Update the candidates - updateCandidates(gctx, nState); + // Update the candidates: initial update + intitializeCandidates(gctx, nState); } } }; -/// This is a chained extractor/filler implementation -/// Since there is no control whether it is a static or -/// payload extractor, these have to be provided by a tuple +/// This is a chained updated, which can either performed a chained filling, +/// or a chaine update (which included intersection test) /// /// @tparam navigation_type distinguishes between internal and external navigation /// @tparam updators_t the updators that will be called in sequence @@ -213,16 +266,30 @@ class ChainedNavigation : public navigation_type { ChainedNavigation(const std::tuple&& upts) : updators(std::move(upts)) {} - /// A combined navigation state updator w/o intersection specifics + /// A combined navigation state updated to fill the candidates from + /// a sequence of pre-defined updators + /// + /// @param gctx is the Geometry context of this call + /// @param nState the navigation state to which the objects are attached + /// + void fill(const GeometryContext& gctx, NavigationState& nState) const { + // Unfold the tuple and add the attachers + std::apply([&](auto&&... updator) { ((updator.fill(gctx, nState)), ...); }, + updators); + } + + /// A combined navigation state to fill & update the candidatesthe candidates /// /// @param gctx is the Geometry context of this call /// @param nState the navigation state to which the objects are attached /// + /// @note It will call the chained filling and then perform a single update on + /// the candidates to avoid copying and multiple intersection tests void update(const GeometryContext& gctx, NavigationState& nState) const { // Unfold the tuple and add the attachers - std::apply( - [&](auto&&... updator) { ((updator.update(gctx, nState)), ...); }, - updators); + fill(gctx, nState); + // Update the candidates: initial update + intitializeCandidates(gctx, nState); } }; diff --git a/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp b/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp index d9d8dac01e6..2ad242c1acc 100644 --- a/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp +++ b/Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp @@ -176,7 +176,7 @@ BOOST_AUTO_TEST_CASE(DetectorNavigatorTestsInitialization) { detector->findDetectorVolume(geoContext, start.position())); BOOST_CHECK_EQUAL(initState.currentSurface, nullptr); BOOST_CHECK_EQUAL(initState.currentPortal, nullptr); - BOOST_CHECK_EQUAL(initState.surfaceCandidates.size(), 1); + BOOST_CHECK_EQUAL(initState.surfaceCandidates.size(), 2u); } }