From 748698da9081b4c72b357e8a649ef954f4560a6d Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Tue, 14 Jan 2025 17:10:15 +0100 Subject: [PATCH] refactor: `ProtoLayer` can (optionally) hold mutable surfaces In the Gen3 building context, using mutable pointers makes more sense since we store as much as possible internally as mutable. This is implemented in a API backwards compatible way, because the Gen1 building assumes the opposite. --- Core/include/Acts/Geometry/ProtoLayer.hpp | 187 ++++++++++++------ Core/src/Geometry/ProtoLayer.cpp | 86 +++----- .../Core/Geometry/ProtoLayerTests.cpp | 35 ++-- 3 files changed, 180 insertions(+), 128 deletions(-) diff --git a/Core/include/Acts/Geometry/ProtoLayer.hpp b/Core/include/Acts/Geometry/ProtoLayer.hpp index 93ea871eb55..d93bb0b2060 100644 --- a/Core/include/Acts/Geometry/ProtoLayer.hpp +++ b/Core/include/Acts/Geometry/ProtoLayer.hpp @@ -20,13 +20,13 @@ namespace Acts { -/// @struct ProtoLayer -/// -/// Encapsulates min/max boundaries that will be turned into a layer. -/// The struct allows this information to be obtained in a consistent -/// way, or be caller provided. +namespace detail { -struct ProtoLayer { +/// @class ProtoLayerBase +/// +/// Base class containing common functionality for ProtoLayer implementations +/// @note This will go away once we remove the Gen1 geometry which assumes this only takes const pointers. +struct ProtoLayerBase { public: /// The extent of the ProtoLayer Extent extent; @@ -37,6 +37,52 @@ struct ProtoLayer { /// The local transform Transform3 transform = Transform3::Identity(); + /// Get the parameters : min + /// @param aDir The accessed axis direction + /// @param addenv The steering if enevlope is added or not + double min(AxisDirection aDir, bool addenv = true) const; + + // Get the parameters : max + /// @param aDir The accessed axis direction + /// @param addenv The steering if enevlope is added or not + double max(AxisDirection aDir, bool addenv = true) const; + + // Get the parameters : medium + /// @param aDir The accessed axis direction + /// @param addenv The steering if enevlope is added or not + double medium(AxisDirection aDir, bool addenv = true) const; + + // Get the parameters : range + /// @param aDir The accessed axis direction + /// @param addenv The steering if enevlope is added or not + double range(AxisDirection aDir, bool addenv = true) const; + + /// Output to ostream + /// @param sl the input ostream + std::ostream& toStream(std::ostream& sl) const; + + protected: + /// Helper method which performs the actual min/max calculation + /// + /// @param gctx The current geometry context object, e.g. alignment + /// @param surfaces The surfaces to build this protolayer out of + /// @param extent The extent to modify + /// @param transform The transform to use + static void measureImpl(const GeometryContext& gctx, + const std::vector& surfaces, + Extent& extent, const Transform3& transform); +}; + +/// @struct ProtoLayerT +/// +/// Encapsulates min/max boundaries that will be turned into a layer. +/// The struct allows this information to be obtained in a consistent +/// way, or be caller provided. +template +struct ProtoLayerT : public ProtoLayerBase { + using SurfacePtr = std::conditional_t; + using SurfaceType = std::conditional_t; + /// Constructor /// /// Loops over a provided vector of surface and calculates the various @@ -46,9 +92,22 @@ struct ProtoLayer { /// @param gctx The current geometry context object, e.g. alignment /// @param surfaces The vector of surfaces to consider /// @param transformIn The local transform to evaluate the sizing in - ProtoLayer(const GeometryContext& gctx, - const std::vector& surfaces, - const Transform3& transformIn = Transform3::Identity()); + ProtoLayerT(const GeometryContext& gctx, + const std::vector& surfaces, + const Transform3& transformIn = Transform3::Identity()) + : m_surfaces(surfaces) { + transform = transformIn; + std::vector constSurfaces; + if constexpr (!IsConst) { + constSurfaces.reserve(surfaces.size()); + for (auto* sf : surfaces) { + constSurfaces.push_back(sf); + } + measureImpl(gctx, constSurfaces, extent, transform); + } else { + measureImpl(gctx, surfaces, extent, transform); + } + } /// Constructor /// @@ -59,79 +118,89 @@ struct ProtoLayer { /// @param gctx The current geometry context object, e.g. alignment /// @param surfaces The vector of surfaces to consider /// @param transformIn The local transform to evaluate the sizing in - ProtoLayer(const GeometryContext& gctx, - const std::vector>& surfaces, - const Transform3& transformIn = Transform3::Identity()); + ProtoLayerT(const GeometryContext& gctx, + const std::vector>& surfaces, + const Transform3& transformIn = Transform3::Identity()) { + transform = transformIn; + m_surfaces.reserve(surfaces.size()); + for (const auto& sf : surfaces) { + m_surfaces.push_back(sf.get()); + } + std::vector constSurfaces; + if constexpr (!IsConst) { + constSurfaces.reserve(surfaces.size()); + for (auto* sf : m_surfaces) { + constSurfaces.push_back(sf); + } + measureImpl(gctx, constSurfaces, extent, transform); + } else { + measureImpl(gctx, m_surfaces, extent, transform); + } + } - /// Constructor - /// - /// Loops over a provided vector of surface and calculates the various - /// min/max values in one go. Also takes into account the thickness - /// of an associated DetectorElement, if it exists. + /// Constructor that accepts non-const shared pointers even when IsConst is + /// true /// /// @param gctx The current geometry context object, e.g. alignment /// @param surfaces The vector of surfaces to consider /// @param transformIn The local transform to evaluate the sizing in - ProtoLayer(const GeometryContext& gctx, - const std::vector>& surfaces, - const Transform3& transformIn = Transform3::Identity()); - - ProtoLayer() = default; - - /// Get the parameters : min - /// @param aDir The accessed axis direction - /// @param addenv The steering if enevlope is added or not - double min(AxisDirection aDir, bool addenv = true) const; - - // Get the parameters : max - /// @param aDir The accessed axis direction - /// @param addenv The steering if enevlope is added or not - double max(AxisDirection aDir, bool addenv = true) const; - - // Get the parameters : max - /// @param aDir The accessed axis direction - /// @param addenv The steering if enevlope is added or not - double medium(AxisDirection aDir, bool addenv = true) const; - - // Get the parameters : max - /// @param aDir The accessed axis direction - /// @param addenv The steering if enevlope is added or not - double range(AxisDirection aDir, bool addenv = true) const; + ProtoLayerT(const GeometryContext& gctx, + const std::vector>& surfaces, + const Transform3& transformIn = Transform3::Identity()) + requires(IsConst) + { + transform = transformIn; + m_surfaces.reserve(surfaces.size()); + for (const auto& sf : surfaces) { + m_surfaces.push_back(sf.get()); + } + measureImpl(gctx, m_surfaces, extent, transform); + } - /// Output to ostream - /// @param sl the input ostream - std::ostream& toStream(std::ostream& sl) const; + ProtoLayerT() = default; /// Output stream operator /// @param sl the input ostream /// @param pl the ProtoLayer to be printed /// @return the output ostream - friend std::ostream& operator<<(std::ostream& sl, const ProtoLayer& pl) { + friend std::ostream& operator<<(std::ostream& sl, const ProtoLayerT& pl) { return pl.toStream(sl); } /// Give access to the surfaces used/assigned to the ProtoLayer - const std::vector& surfaces() const; + const std::vector& surfaces() const { return m_surfaces; } /// Add a surface, this will also increase the extent /// @param gctx The current geometry context object, e.g. alignment /// @param surface The surface which is added to the ProtoLayer - void add(const GeometryContext& gctx, const Surface& surface); + void add(const GeometryContext& gctx, SurfaceType& surface) { + m_surfaces.push_back(&surface); + std::vector constSurfaces; + if constexpr (!IsConst) { + constSurfaces.reserve(m_surfaces.size()); + for (auto* sf : m_surfaces) { + constSurfaces.push_back(sf); + } + measureImpl(gctx, constSurfaces, extent, transform); + } else { + measureImpl(gctx, m_surfaces, extent, transform); + } + } private: - /// Helper method which performs the actual min/max calculation - /// - /// @param gctx The current geometry context object, e.g. alignment - /// @param surfaces The surfaces to build this protolayer out of - void measure(const GeometryContext& gctx, - const std::vector& surfaces); - /// Store the list of surfaces used for this proto layer - std::vector m_surfaces = {}; + std::vector m_surfaces = {}; }; -inline const std::vector& ProtoLayer::surfaces() const { - return m_surfaces; -} +} // namespace detail + +// Forward-declaration friendly class for backward compatibility +struct ProtoLayer : public detail::ProtoLayerT { + using detail::ProtoLayerT::ProtoLayerT; +}; + +struct MutableProtoLayer : public detail::ProtoLayerT { + using detail::ProtoLayerT::ProtoLayerT; +}; } // namespace Acts diff --git a/Core/src/Geometry/ProtoLayer.cpp b/Core/src/Geometry/ProtoLayer.cpp index 2956e6a2788..6ba568f1434 100644 --- a/Core/src/Geometry/ProtoLayer.cpp +++ b/Core/src/Geometry/ProtoLayer.cpp @@ -16,89 +16,59 @@ using Acts::VectorHelpers::perp; using Acts::VectorHelpers::phi; -namespace Acts { +namespace Acts::detail { -ProtoLayer::ProtoLayer(const GeometryContext& gctx, - const std::vector& surfaces, - const Transform3& transformIn) - : transform(transformIn), m_surfaces(surfaces) { - measure(gctx, surfaces); -} - -ProtoLayer::ProtoLayer( - const GeometryContext& gctx, - const std::vector>& surfaces, - const Transform3& transformIn) - : transform(transformIn), m_surfaces(unpack_shared_vector(surfaces)) { - measure(gctx, m_surfaces); -} - -ProtoLayer::ProtoLayer(const GeometryContext& gctx, - const std::vector>& surfaces, - const Transform3& transformIn) - : transform(transformIn) { - m_surfaces.reserve(surfaces.size()); +void ProtoLayerBase::measureImpl(const GeometryContext& gctx, + const std::vector& surfaces, + Extent& extent, const Transform3& transform) { for (const auto& sf : surfaces) { - m_surfaces.push_back(sf.get()); + // To prevent problematic isInsidePolygon check for straw surfaces with only + // one lseg + int lseg = (sf->type() != Surface::Straw) ? 1 : 2; + auto sfPolyhedron = sf->polyhedronRepresentation(gctx, lseg); + const DetectorElementBase* element = sf->associatedDetectorElement(); + const auto* regSurface = dynamic_cast(sf); + if (element != nullptr && regSurface != nullptr) { + // Take the thickness in account if necessary + double thickness = element->thickness(); + // We need a translation along and opposite half thickness + Vector3 sfNormal = regSurface->normal(gctx, sf->center(gctx)); + for (const auto& dT : {-0.5 * thickness, 0.5 * thickness}) { + Transform3 dtransform = transform * Translation3{dT * sfNormal}; + extent.extend(sfPolyhedron.extent(dtransform)); + } + continue; + } + extent.extend(sfPolyhedron.extent(transform)); } - measure(gctx, m_surfaces); } -double ProtoLayer::min(AxisDirection aDir, bool addenv) const { +double ProtoLayerBase::min(AxisDirection aDir, bool addenv) const { if (addenv) { return extent.min(aDir) - envelope[aDir][0u]; } return extent.min(aDir); } -double ProtoLayer::max(AxisDirection aDir, bool addenv) const { +double ProtoLayerBase::max(AxisDirection aDir, bool addenv) const { if (addenv) { return extent.max(aDir) + envelope[aDir][1u]; } return extent.max(aDir); } -double ProtoLayer::medium(AxisDirection aDir, bool addenv) const { +double ProtoLayerBase::medium(AxisDirection aDir, bool addenv) const { return 0.5 * (min(aDir, addenv) + max(aDir, addenv)); } -double ProtoLayer::range(AxisDirection aDir, bool addenv) const { +double ProtoLayerBase::range(AxisDirection aDir, bool addenv) const { return std::abs(max(aDir, addenv) - min(aDir, addenv)); } -std::ostream& ProtoLayer::toStream(std::ostream& sl) const { +std::ostream& ProtoLayerBase::toStream(std::ostream& sl) const { sl << "ProtoLayer with dimensions (min/max)" << std::endl; sl << extent.toString(); return sl; } -void ProtoLayer::measure(const GeometryContext& gctx, - const std::vector& surfaces) { - for (const auto& sf : surfaces) { - // To prevent problematic isInsidePolygon check for straw surfaces with only - // one lseg - int lseg = (sf->type() != Surface::Straw) ? 1 : 2; - auto sfPolyhedron = sf->polyhedronRepresentation(gctx, lseg); - const DetectorElementBase* element = sf->associatedDetectorElement(); - const auto* regSurface = dynamic_cast(sf); - if (element != nullptr && regSurface != nullptr) { - // Take the thickness in account if necessary - double thickness = element->thickness(); - // We need a translation along and opposite half thickness - Vector3 sfNormal = regSurface->normal(gctx, sf->center(gctx)); - for (const auto& dT : {-0.5 * thickness, 0.5 * thickness}) { - Transform3 dtransform = transform * Translation3{dT * sfNormal}; - extent.extend(sfPolyhedron.extent(dtransform)); - } - continue; - } - extent.extend(sfPolyhedron.extent(transform)); - } -} - -void ProtoLayer::add(const GeometryContext& gctx, const Surface& surface) { - m_surfaces.push_back(&surface); - measure(gctx, m_surfaces); -} - -} // namespace Acts +} // namespace Acts::detail diff --git a/Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp b/Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp index a0cc315b4b1..88fd5fbaeb5 100644 --- a/Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp +++ b/Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp @@ -35,8 +35,12 @@ GeometryContext tgContext = GeometryContext(); BOOST_AUTO_TEST_SUITE(Geometry) -BOOST_AUTO_TEST_CASE(ProtoLayerTests) { +// Test both const and non-const versions +template +void testProtoLayer() { using enum AxisDirection; + using TestProtoLayer = detail::ProtoLayerT; + using SurfacePtr = typename TestProtoLayer::SurfacePtr; // Create a proto layer with 4 surfaces on the x/y grid auto recBounds = std::make_shared(3., 6.); @@ -51,11 +55,13 @@ BOOST_AUTO_TEST_CASE(ProtoLayerTests) { AngleAxis3(-std::numbers::pi / 2., Vector3::UnitZ()) * Transform3::Identity(); - std::vector> surfaceStore; + std::vector< + std::shared_ptr>> + surfaceStore; surfaceStore.reserve(100); auto createProtoLayer = [&](const Transform3& trf, - bool shared = false) -> ProtoLayer { + bool shared = false) -> TestProtoLayer { auto atNegX = Surface::makeShared( Transform3(trf * Translation3(Vector3(-3., 0., 0.)) * planeYZ), recBounds); @@ -72,17 +78,18 @@ BOOST_AUTO_TEST_CASE(ProtoLayerTests) { Transform3(trf * Translation3(Vector3(0., 3., 0.)) * planeZX), recBounds); - std::vector> sharedSurfaces = { - atNegX, atNegY, atPosX, atPosY}; + std::vector< + std::shared_ptr>> + sharedSurfaces = {atNegX, atNegY, atPosX, atPosY}; surfaceStore.insert(surfaceStore.begin(), sharedSurfaces.begin(), sharedSurfaces.end()); if (!shared) { - std::vector surfaces = {atNegX.get(), atNegY.get(), - atPosX.get(), atPosY.get()}; + std::vector surfaces = {atNegX.get(), atNegY.get(), + atPosX.get(), atPosY.get()}; - return ProtoLayer(tgContext, surfaces); + return TestProtoLayer(tgContext, surfaces); } - return ProtoLayer(tgContext, sharedSurfaces); + return TestProtoLayer(tgContext, sharedSurfaces); }; // Test 0 - check constructor with surfaces and shared surfaces @@ -126,8 +133,6 @@ BOOST_AUTO_TEST_CASE(ProtoLayerTests) { CHECK_CLOSE_ABS(protoLayer.max(AxisR), std::hypot(3, 6), 1e-8); CHECK_CLOSE_ABS(protoLayer.min(AxisR), 3., 1e-8); - // Test 1a - // Test 2 - rotate around Z-Axis, should leave R, Z untouched, // only preserves medium values auto protoLayerRot = createProtoLayer(AngleAxis3(-0.345, Vector3::UnitZ()) * @@ -160,6 +165,14 @@ Extent in space : BOOST_CHECK_EQUAL(sstream.str(), oString); } +BOOST_AUTO_TEST_CASE(ProtoLayerTests) { + testProtoLayer(); // Test const version +} + +BOOST_AUTO_TEST_CASE(ProtoLayerTestsNonConst) { + testProtoLayer(); // Test non-const version +} + BOOST_AUTO_TEST_CASE(OrientedLayer) { using enum AxisDirection; using namespace Acts::UnitLiterals;