From 7f517d43e7720ac2592fad0a7bce94bc8c9dde51 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Thu, 19 Sep 2024 18:06:53 +0200 Subject: [PATCH] shell: Fix missing loop for portal assignment (lots of asserts) --- Core/include/Acts/Geometry/PortalShell.hpp | 12 ++ Core/src/Geometry/PortalShell.cpp | 77 +++++++- .../Core/Geometry/PortalShellTests.cpp | 178 +++++++++++++++++- 3 files changed, 263 insertions(+), 4 deletions(-) diff --git a/Core/include/Acts/Geometry/PortalShell.hpp b/Core/include/Acts/Geometry/PortalShell.hpp index fdf890840bc..942d10a37d1 100644 --- a/Core/include/Acts/Geometry/PortalShell.hpp +++ b/Core/include/Acts/Geometry/PortalShell.hpp @@ -33,6 +33,10 @@ class PortalShellBase { // @TODO: Revisit, I'm not super happy with this interface. virtual void applyToVolume() = 0; + + virtual bool isValid() const = 0; + + virtual std::string label() const = 0; }; class CylinderPortalShell : public PortalShellBase { @@ -72,6 +76,10 @@ class SingleCylinderPortalShell : public CylinderPortalShell { void applyToVolume() override; + bool isValid() const override; + + std::string label() const override; + private: std::array, 6> m_portals{}; @@ -94,6 +102,10 @@ class CylinderStackPortalShell : public CylinderPortalShell { // No-op, because it's a composite portal shell void applyToVolume() override {} + bool isValid() const override; + + std::string label() const override; + private: BinningValue m_direction; std::vector m_shells; diff --git a/Core/src/Geometry/PortalShell.cpp b/Core/src/Geometry/PortalShell.cpp index 26e524d5377..d06e8905eed 100644 --- a/Core/src/Geometry/PortalShell.cpp +++ b/Core/src/Geometry/PortalShell.cpp @@ -15,7 +15,10 @@ #include "Acts/Geometry/TrackingVolume.hpp" #include -#include +#include +#include + +#include namespace Acts { @@ -90,6 +93,8 @@ std::shared_ptr SingleCylinderPortalShell::portalPtr(Face face) { void SingleCylinderPortalShell::setPortal(std::shared_ptr portal, Face face) { + assert(portal != nullptr); + assert(portal->isValid()); m_portals.at(toUnderlying(face)) = std::move(portal); } @@ -101,13 +106,32 @@ std::size_t SingleCylinderPortalShell::size() const { } void SingleCylinderPortalShell::applyToVolume() { - for (const auto& portal : m_portals) { + for (std::size_t i = 0; i < m_portals.size(); i++) { + const auto& portal = m_portals.at(i); if (portal != nullptr) { + if (!portal->isValid()) { + std::stringstream ss; + ss << static_cast(i); + throw std::runtime_error{"Invalid portal found in shell at " + + ss.str()}; + } m_volume->addPortal(portal); } } } +bool SingleCylinderPortalShell::isValid() const { + return std::ranges::all_of(m_portals, [](const auto& portal) { + return portal == nullptr || portal->isValid(); + }); +}; + +std::string SingleCylinderPortalShell::label() const { + std::stringstream ss; + ss << "CylinderShell(vol=" << m_volume->volumeName() << ")"; + return ss.str(); +} + CylinderStackPortalShell::CylinderStackPortalShell( const GeometryContext& gctx, std::vector shells, BinningValue direction, const Logger& logger) @@ -120,6 +144,14 @@ CylinderStackPortalShell::CylinderStackPortalShell( throw std::invalid_argument("Invalid shell pointer"); } + ACTS_VERBOSE(" ~> " << label()); + + if (!std::ranges::all_of( + m_shells, [](const auto* shell) { return shell->isValid(); })) { + ACTS_ERROR("Invalid shell"); + throw std::invalid_argument("Invalid shell"); + } + auto merge = [&gctx, direction, &shells = m_shells, &logger](Face face) { std::vector> portals; std::transform(shells.begin(), shells.end(), std::back_inserter(portals), @@ -136,6 +168,9 @@ CylinderStackPortalShell::CylinderStackPortalShell( Portal::merge(gctx, *aPortal, *bPortal, direction, logger)); }); + assert(merged != nullptr); + assert(merged->isValid()); + // reset merged portal on all shells for (auto& shell : shells) { shell->setPortal(merged, face); @@ -146,11 +181,19 @@ CylinderStackPortalShell::CylinderStackPortalShell( for (std::size_t i = 1; i < shells.size(); i++) { auto& shellA = shells.at(i - 1); auto& shellB = shells.at(i); + ACTS_VERBOSE("Fusing " << shellA->label() << " and " << shellB->label()); + auto fused = std::make_shared(Portal::fuse( gctx, *shellA->portalPtr(faceA), *shellB->portalPtr(faceB), logger)); + assert(fused != nullptr && "Invalid fused portal"); + assert(fused->isValid() && "Fused portal is invalid"); + shellA->setPortal(fused, faceA); shellB->setPortal(fused, faceB); + + assert(shellA->isValid() && "Shell A is not valid after fusing"); + assert(shellB->isValid() && "Shell B is not valid after fusing"); } }; @@ -178,18 +221,23 @@ CylinderStackPortalShell::CylinderStackPortalShell( ACTS_VERBOSE("Merging portals at outer cylinders"); merge(OuterCylinder); + assert(isValid() && "Shell is not valid after outer merging"); if (m_hasInnerCylinder) { ACTS_VERBOSE("Merging portals at inner cylinders"); merge(InnerCylinder); + assert(isValid() && "Shell is not valid after inner merging"); } ACTS_VERBOSE("Fusing portals at positive and negative discs"); fuse(PositiveDisc, NegativeDisc); + assert(isValid() && "Shell is not valid after disc fusing"); } else { throw std::invalid_argument("Invalid direction"); } + + assert(isValid() && "Shell is not valid after construction"); } std::size_t CylinderStackPortalShell::size() const { @@ -245,12 +293,16 @@ std::shared_ptr CylinderStackPortalShell::portalPtr(Face face) { void CylinderStackPortalShell::setPortal(std::shared_ptr portal, Face face) { + assert(portal != nullptr); + if (m_direction == BinningValue::binR) { switch (face) { case NegativeDisc: [[fallthrough]]; case PositiveDisc: - m_shells.front()->setPortal(std::move(portal), face); + for (auto* shell : m_shells) { + shell->setPortal(portal, face); + } break; case OuterCylinder: m_shells.back()->setPortal(std::move(portal), OuterCylinder); @@ -293,6 +345,25 @@ void CylinderStackPortalShell::setPortal(std::shared_ptr portal, } } +bool CylinderStackPortalShell::isValid() const { + return std::ranges::all_of(m_shells, [](const auto* shell) { + assert(shell != nullptr); + return shell->isValid(); + }); +} + +std::string CylinderStackPortalShell::label() const { + std::stringstream ss; + ss << "CylinderStackShell(dir=" << m_direction << ", children="; + + std::vector labels; + std::ranges::transform(m_shells, std::back_inserter(labels), + [](const auto* shell) { return shell->label(); }); + ss << boost::algorithm::join(labels, "|"); + ss << ")"; + return ss.str(); +} + std::ostream& operator<<(std::ostream& os, CylinderPortalShell::Face face) { switch (face) { using enum CylinderPortalShell::Face; diff --git a/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp b/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp index 58e62725129..98bace4e207 100644 --- a/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp +++ b/Tests/UnitTests/Core/Geometry/PortalShellTests.cpp @@ -13,6 +13,7 @@ #include "Acts/Definitions/Units.hpp" #include "Acts/Geometry/CuboidVolumeBounds.hpp" +#include "Acts/Geometry/CutoutCylinderVolumeBounds.hpp" #include "Acts/Geometry/CylinderVolumeBounds.hpp" #include "Acts/Geometry/GridPortalLink.hpp" #include "Acts/Geometry/Portal.hpp" @@ -38,9 +39,11 @@ auto makeVolume(auto&&... pars) { std::make_shared( std::forward(pars)...)); vol.setVolumeName("cyl" + std::to_string(getVolumeIndex())); - return std::move(vol); + return vol; }; +auto logger = Acts::getDefaultLogger("UnitTests", Acts::Logging::VERBOSE); + BOOST_AUTO_TEST_SUITE(PortalShellTests) BOOST_AUTO_TEST_CASE(ConstructionFromVolume) { @@ -544,6 +547,179 @@ BOOST_AUTO_TEST_CASE(RDirection) { } } +BOOST_AUTO_TEST_CASE(NestedStacks) { + // ^ + // r | +---------------------------------+---------+ + // | | | | + // | | | | + // | | vol2 | | + // | | | | + // | | | | + // | +---------------------------------+ | + // | | | | + // | | | | + // | | gap | vol3 | + // | | | | + // | | | | + // | +---------------------------------+ | + // | | | | + // | | | | + // | | vol1 | | + // | | | | + // | | | | + // | +---------------------------------+---------+ + // | + // +--------------------------------------------------> + // z + + Transform3 base = Transform3::Identity(); + + TrackingVolume vol1( + base, std::make_shared(23_mm, 48_mm, 200_mm), + "PixelLayer0"); + + TrackingVolume gap( + base, std::make_shared(48_mm, 250_mm, 200_mm), + "Gap"); + + TrackingVolume vol2( + base, std::make_shared(250_mm, 400_mm, 200_mm), + "PixelLayer3"); + + TrackingVolume vol3( + base * Translation3{Vector3::UnitZ() * 300_mm}, + std::make_shared(23_mm, 400_mm, 100_mm), + "PixelEcPos"); + + SingleCylinderPortalShell shell1{vol1}; + BOOST_CHECK(shell1.isValid()); + SingleCylinderPortalShell gapShell{gap}; + BOOST_CHECK(gapShell.isValid()); + SingleCylinderPortalShell shell2{vol2}; + BOOST_CHECK(shell2.isValid()); + + CylinderStackPortalShell stack{ + gctx, {&shell1, &gapShell, &shell2}, BinningValue::binR}; + + BOOST_CHECK(stack.isValid()); + + SingleCylinderPortalShell shell3{vol3}; + BOOST_CHECK(shell3.isValid()); + + CylinderStackPortalShell stack2{ + gctx, {&stack, &shell3}, BinningValue::binZ, *logger}; + BOOST_CHECK(stack2.isValid()); + + using enum CylinderPortalShell::Face; + + auto lookup = [](auto& shell, CylinderPortalShell::Face face, + Vector3 position, + Vector3 direction) -> const TrackingVolume* { + const auto* portal = shell.portal(face); + BOOST_REQUIRE_NE(portal, nullptr); + return portal->resolveVolume(gctx, position, direction).value(); + }; + + // Interconnection in the r direction + + BOOST_CHECK_EQUAL(lookup(shell1, InnerCylinder, 23_mm * Vector3::UnitX(), + -Vector3::UnitX()), + nullptr); + BOOST_CHECK_EQUAL( + lookup(shell1, InnerCylinder, 23_mm * Vector3::UnitX(), Vector3::UnitX()), + &vol1); + + BOOST_CHECK_EQUAL( + lookup(shell1, OuterCylinder, 48_mm * Vector3::UnitX(), Vector3::UnitX()), + &gap); + + BOOST_CHECK_EQUAL(lookup(gapShell, InnerCylinder, 48_mm * Vector3::UnitX(), + -Vector3::UnitX()), + &vol1); + + BOOST_CHECK_EQUAL(lookup(gapShell, OuterCylinder, 250_mm * Vector3::UnitX(), + Vector3::UnitX()), + &vol2); + + BOOST_CHECK_EQUAL(lookup(shell2, InnerCylinder, 250_mm * Vector3::UnitX(), + -Vector3::UnitX()), + &gap); + + BOOST_CHECK_EQUAL(lookup(shell2, OuterCylinder, 400_mm * Vector3::UnitX(), + Vector3::UnitX()), + nullptr); + + BOOST_CHECK_EQUAL(lookup(shell2, OuterCylinder, 400_mm * Vector3::UnitX(), + -Vector3::UnitX()), + &vol2); + + BOOST_CHECK_EQUAL(lookup(shell2, OuterCylinder, 400_mm * Vector3::UnitX(), + -Vector3::UnitX()), + &vol2); + + // Left connection + + BOOST_CHECK_EQUAL(lookup(shell1, NegativeDisc, Vector3(30_mm, 0, -200_mm), + -Vector3::UnitZ()), + nullptr); + + BOOST_CHECK_EQUAL(lookup(shell1, NegativeDisc, Vector3(30_mm, 0, -200_mm), + Vector3::UnitZ()), + &vol1); + + BOOST_CHECK_EQUAL(lookup(gapShell, NegativeDisc, Vector3(60_mm, 0, -200_mm), + -Vector3::UnitZ()), + nullptr); + + BOOST_CHECK_EQUAL(lookup(gapShell, NegativeDisc, Vector3(60_mm, 0, -200_mm), + Vector3::UnitZ()), + &gap); + + BOOST_CHECK_EQUAL(lookup(shell2, NegativeDisc, Vector3(300_mm, 0, -200_mm), + -Vector3::UnitZ()), + nullptr); + + BOOST_CHECK_EQUAL(lookup(shell2, NegativeDisc, Vector3(300_mm, 0, -200_mm), + Vector3::UnitZ()), + &vol2); + + // Right connection + + BOOST_CHECK_EQUAL(lookup(shell1, PositiveDisc, Vector3(30_mm, 0, 200_mm), + -Vector3::UnitZ()), + &vol1); + + BOOST_CHECK_EQUAL( + lookup(shell1, PositiveDisc, Vector3(30_mm, 0, 200_mm), Vector3::UnitZ()), + &vol3); + + BOOST_CHECK_EQUAL(lookup(gapShell, PositiveDisc, Vector3(60_mm, 0, 200_mm), + -Vector3::UnitZ()), + &gap); + + BOOST_CHECK_EQUAL(lookup(gapShell, PositiveDisc, Vector3(60_mm, 0, 200_mm), + Vector3::UnitZ()), + &vol3); + + BOOST_CHECK_EQUAL(lookup(shell2, PositiveDisc, Vector3(300_mm, 0, 200_mm), + -Vector3::UnitZ()), + &vol2); + + BOOST_CHECK_EQUAL(lookup(shell2, PositiveDisc, Vector3(300_mm, 0, 200_mm), + Vector3::UnitZ()), + &vol3); + + // Right outer connection + + BOOST_CHECK_EQUAL(lookup(shell3, PositiveDisc, Vector3(300_mm, 0, 400_mm), + -Vector3::UnitZ()), + &vol3); + + BOOST_CHECK_EQUAL(lookup(shell3, PositiveDisc, Vector3(300_mm, 0, 400_mm), + Vector3::UnitZ()), + nullptr); +} + BOOST_AUTO_TEST_CASE(ConnectOuter) { auto cyl1 = makeVolume(30_mm, 40_mm, 100_mm); auto cyl2 = makeVolume(0_mm, 50_mm, 110_mm);