From da0d486684ec4eec6540b01b53ee5344db2fd187 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 4 Oct 2024 11:17:54 +0200 Subject: [PATCH 1/5] refactor: Add logger to Volume::update This makes it easier to pass it through if you have one (CylinderVolumeStack mainly) --- .../Acts/Geometry/CylinderVolumeStack.hpp | 17 ++----- Core/include/Acts/Geometry/Volume.hpp | 5 +- Core/src/Geometry/CylinderVolumeStack.cpp | 46 ++++++++----------- Core/src/Geometry/Volume.cpp | 3 +- 4 files changed, 28 insertions(+), 43 deletions(-) diff --git a/Core/include/Acts/Geometry/CylinderVolumeStack.hpp b/Core/include/Acts/Geometry/CylinderVolumeStack.hpp index 5e708a5c3c9..72f2c2a9458 100644 --- a/Core/include/Acts/Geometry/CylinderVolumeStack.hpp +++ b/Core/include/Acts/Geometry/CylinderVolumeStack.hpp @@ -85,23 +85,12 @@ class CylinderVolumeStack : public Volume { /// construction. /// @param volbounds is the new bounds /// @param transform is the new transform - /// @pre The volume bounds need to be of type - /// @c CylinderVolumeBounds. - void update(std::shared_ptr volbounds, - std::optional transform = std::nullopt) override; - - /// Update the volume bounds and transform. This - /// will update the bounds of all volumes in the stack - /// to accommodate the new bounds and optionally create - /// gap volumes according to the resize strategy set during - /// construction. - /// @param newBounds is the new bounds - /// @param transform is the new transform /// @param logger is the logger /// @pre The volume bounds need to be of type /// @c CylinderVolumeBounds. - void update(std::shared_ptr newBounds, - std::optional transform, const Logger& logger); + void update(std::shared_ptr volbounds, + std::optional transform = std::nullopt, + const Logger& logger = getDummyLogger()) override; /// Access the gap volume that were created during attachment or resizing. /// @return the vector of gap volumes diff --git a/Core/include/Acts/Geometry/Volume.hpp b/Core/include/Acts/Geometry/Volume.hpp index 9217e758444..da8932b1eab 100644 --- a/Core/include/Acts/Geometry/Volume.hpp +++ b/Core/include/Acts/Geometry/Volume.hpp @@ -13,6 +13,7 @@ #include "Acts/Geometry/GeometryObject.hpp" #include "Acts/Utilities/BinningType.hpp" #include "Acts/Utilities/BoundingBox.hpp" +#include "Acts/Utilities/Logger.hpp" #include #include @@ -83,8 +84,10 @@ class Volume : public GeometryObject { /// Set the volume bounds and optionally also update the volume transform /// @param volbounds The volume bounds to be assigned /// @param transform The transform to be assigned, can be optional + /// @param logger A logger object to log messages virtual void update(std::shared_ptr volbounds, - std::optional transform = std::nullopt); + std::optional transform = std::nullopt, + const Logger& logger = Acts::getDummyLogger()); /// Construct bounding box for this shape /// @param envelope Optional envelope to add / subtract from min/max diff --git a/Core/src/Geometry/CylinderVolumeStack.cpp b/Core/src/Geometry/CylinderVolumeStack.cpp index cb1cfdeb822..5668b07b506 100644 --- a/Core/src/Geometry/CylinderVolumeStack.cpp +++ b/Core/src/Geometry/CylinderVolumeStack.cpp @@ -67,7 +67,7 @@ struct CylinderVolumeStack::VolumeTuple { transformDirty = true; } - void commit() { + void commit(const Logger& logger) { // make a copy so we can't accidentally modify in-place auto copy = std::make_shared(*updatedBounds); @@ -76,7 +76,7 @@ struct CylinderVolumeStack::VolumeTuple { transform = globalTransform; } - volume->update(std::move(updatedBounds), transform); + volume->update(std::move(updatedBounds), transform, logger); bounds = copy.get(); updatedBounds = std::move(copy); transformDirty = false; @@ -151,7 +151,8 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, const auto* cylBounds = dynamic_cast( &m_volumes.front()->volumeBounds()); assert(cylBounds != nullptr && "Volume bounds are not cylinder bounds"); - Volume::update(std::make_shared(*cylBounds)); + Volume::update(std::make_shared(*cylBounds), + std::nullopt, logger); return; } @@ -185,7 +186,7 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, << vt.localTransform.translation()[eZ]); ACTS_VERBOSE(*vt.updatedBounds); - vt.commit(); + vt.commit(logger); } ACTS_VERBOSE("*** Volume configuration after r synchronization:"); @@ -209,7 +210,8 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, m_transform = m_groupTransform * Translation3{0, 0, midZ}; - Volume::update(std::make_shared(minR, maxR, hlZ)); + Volume::update(std::make_shared(minR, maxR, hlZ), + std::nullopt, logger); ACTS_DEBUG("Outer bounds are:\n" << volumeBounds()); ACTS_DEBUG("Outer transform / new group transform is:\n" << m_transform.matrix()); @@ -241,7 +243,7 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, for (auto& vt : volumeTuples) { ACTS_VERBOSE("Updated bounds for volume at r: " << vt.midR()); ACTS_VERBOSE(*vt.updatedBounds); - vt.commit(); + vt.commit(logger); } ACTS_VERBOSE("*** Volume configuration after z synchronization:"); @@ -265,7 +267,8 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, m_transform = m_groupTransform * Translation3{0, 0, midZ}; - Volume::update(std::make_shared(minR, maxR, hlZ)); + Volume::update(std::make_shared(minR, maxR, hlZ), + std::nullopt, logger); ACTS_DEBUG("Outer bounds are:\n" << volumeBounds()); ACTS_DEBUG("Outer transform is:\n" << m_transform.matrix()); @@ -622,30 +625,19 @@ std::pair CylinderVolumeStack::synchronizeZBounds( } void CylinderVolumeStack::update(std::shared_ptr volbounds, - std::optional transform) { - if (volbounds == nullptr) { - throw std::invalid_argument("New bounds are nullptr"); - } + std::optional transform, + const Logger& logger) { auto cylBounds = std::dynamic_pointer_cast(volbounds); if (cylBounds == nullptr) { throw std::invalid_argument( "CylinderVolumeStack requires CylinderVolumeBounds"); } - update(std::move(cylBounds), transform, - *Acts::getDefaultLogger("CYLSTACK", Logging::VERBOSE)); -} -void CylinderVolumeStack::update( - std::shared_ptr newBounds, - std::optional transform, const Logger& logger) { - ACTS_DEBUG( - "Resizing CylinderVolumeStack with strategy: " << m_resizeStrategy); - - if (newBounds == nullptr) { + if (cylBounds == nullptr) { throw std::invalid_argument("New bounds are nullptr"); } - if (*newBounds == volumeBounds()) { + if (*cylBounds == volumeBounds()) { ACTS_VERBOSE("Bounds are the same, no resize needed"); return; } @@ -656,7 +648,7 @@ void CylinderVolumeStack::update( VolumeTuple oldVolume{*this, m_transform}; VolumeTuple newVolume{*this, m_transform}; - newVolume.updatedBounds = std::make_shared(*newBounds); + newVolume.updatedBounds = std::make_shared(*cylBounds); newVolume.globalTransform = transform.value_or(m_transform); newVolume.localTransform = m_groupTransform.inverse() * newVolume.globalTransform; @@ -673,7 +665,7 @@ void CylinderVolumeStack::update( checkVolumeAlignment(volTemp, logger); } - checkNoPhiOrBevel(*newBounds, logger); + checkNoPhiOrBevel(*cylBounds, logger); const ActsScalar newMinR = newVolume.minR(); const ActsScalar newMaxR = newVolume.maxR(); @@ -831,7 +823,7 @@ void CylinderVolumeStack::update( ACTS_VERBOSE("Commit and update outer vector of volumes"); m_volumes.clear(); for (auto& vt : volumeTuples) { - vt.commit(); + vt.commit(logger); m_volumes.push_back(vt.volume); } @@ -923,13 +915,13 @@ void CylinderVolumeStack::update( ACTS_VERBOSE("Commit and update outer vector of volumes"); m_volumes.clear(); for (auto& vt : volumeTuples) { - vt.commit(); + vt.commit(logger); m_volumes.push_back(vt.volume); } } m_transform = newVolume.globalTransform; - Volume::update(std::move(newBounds)); + Volume::update(std::move(cylBounds), std::nullopt, logger); } void CylinderVolumeStack::checkNoPhiOrBevel(const CylinderVolumeBounds& bounds, diff --git a/Core/src/Geometry/Volume.cpp b/Core/src/Geometry/Volume.cpp index c1aefc96186..97a026b039f 100644 --- a/Core/src/Geometry/Volume.cpp +++ b/Core/src/Geometry/Volume.cpp @@ -79,7 +79,8 @@ void Volume::assignVolumeBounds(std::shared_ptr volbounds) { } void Volume::update(std::shared_ptr volbounds, - std::optional transform) { + std::optional transform, + const Logger& /*logger*/) { if (volbounds) { m_volumeBounds = std::move(volbounds); } From 5bd91eb93581e05c8b3d6a8b45b04bee306b40dc Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 4 Oct 2024 11:21:29 +0200 Subject: [PATCH 2/5] refactor: More descriptive CylVolStack overlap printout --- .../Acts/Geometry/CylinderVolumeStack.hpp | 5 ++- Core/src/Geometry/CylinderVolumeStack.cpp | 41 +++++++++++++------ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/Core/include/Acts/Geometry/CylinderVolumeStack.hpp b/Core/include/Acts/Geometry/CylinderVolumeStack.hpp index 72f2c2a9458..3fcb8f1039b 100644 --- a/Core/include/Acts/Geometry/CylinderVolumeStack.hpp +++ b/Core/include/Acts/Geometry/CylinderVolumeStack.hpp @@ -122,11 +122,12 @@ class CylinderVolumeStack : public Volume { Acts::Logging::Level lvl); /// Helper function that prints output helping in debugging overlaps + /// @param direction is the overlap check direction /// @param a is the first volume /// @param b is the second volume /// @param logger is the logger - static void overlapPrint(const VolumeTuple& a, const VolumeTuple& b, - const Logger& logger); + static void overlapPrint(BinningValue direction, const VolumeTuple& a, + const VolumeTuple& b, const Logger& logger); /// Helper function that checks if volumes are properly aligned /// for attachment. diff --git a/Core/src/Geometry/CylinderVolumeStack.cpp b/Core/src/Geometry/CylinderVolumeStack.cpp index 5668b07b506..62f5d2b7bdc 100644 --- a/Core/src/Geometry/CylinderVolumeStack.cpp +++ b/Core/src/Geometry/CylinderVolumeStack.cpp @@ -286,25 +286,40 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, } void CylinderVolumeStack::overlapPrint( - const CylinderVolumeStack::VolumeTuple& a, + BinningValue direction, const CylinderVolumeStack::VolumeTuple& a, const CylinderVolumeStack::VolumeTuple& b, const Logger& logger) { if (logger().doPrint(Acts::Logging::DEBUG)) { std::stringstream ss; ss << std::fixed; ss << std::setprecision(3); ss << std::setfill(' '); - ACTS_VERBOSE("Checking overlap between"); + int w = 9; - ss << " - " - << " z: [ " << std::setw(w) << a.minZ() << " <- " << std::setw(w) - << a.midZ() << " -> " << std::setw(w) << a.maxZ() << " ]"; - ACTS_VERBOSE(ss.str()); - ss.str(""); - ss << " - " - << " z: [ " << std::setw(w) << b.minZ() << " <- " << std::setw(w) - << b.midZ() << " -> " << std::setw(w) << b.maxZ() << " ]"; - ACTS_VERBOSE(ss.str()); + ACTS_VERBOSE("Checking overlap between"); + if (direction == BinningValue::binZ) { + ss << " - " + << " z: [ " << std::setw(w) << a.minZ() << " <- " << std::setw(w) + << a.midZ() << " -> " << std::setw(w) << a.maxZ() << " ]"; + ACTS_VERBOSE(ss.str()); + + ss.str(""); + ss << " - " + << " z: [ " << std::setw(w) << b.minZ() << " <- " << std::setw(w) + << b.midZ() << " -> " << std::setw(w) << b.maxZ() << " ]"; + ACTS_VERBOSE(ss.str()); + } else { + ss << " - " + << " r: [ " << std::setw(w) << a.minR() << " <-> " << std::setw(w) + << a.maxR() << " ]"; + ACTS_VERBOSE(ss.str()); + + ss.str(""); + ss << " - " + << " r: [ " << std::setw(w) << b.minR() << " <-> " << std::setw(w) + << b.maxR() << " ]"; + ACTS_VERBOSE(ss.str()); + } } } @@ -319,7 +334,7 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( auto& a = volumes.at(i); auto& b = volumes.at(j); - overlapPrint(a, b, logger); + overlapPrint(BinningValue::binZ, a, b, logger); if (a.maxZ() > b.minZ()) { ACTS_ERROR(" -> Overlap in z"); @@ -446,7 +461,7 @@ CylinderVolumeStack::checkOverlapAndAttachInR( auto& a = volumes.at(i); auto& b = volumes.at(j); - overlapPrint(a, b, logger); + overlapPrint(BinningValue::binR, a, b, logger); if (a.maxR() > b.minR()) { ACTS_ERROR(" -> Overlap in r"); From 88aaf5abafbfe2ca3ccb6ed37ddf53f0f2d0801d Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 4 Oct 2024 11:29:15 +0200 Subject: [PATCH 3/5] refactor: Improved output in CylStack, CylBounds, portal links --- Core/src/Geometry/CylinderVolumeStack.cpp | 46 ++++++++++++++++------- Core/src/Geometry/GridPortalLink.cpp | 5 ++- Core/src/Geometry/TrivialPortalLink.cpp | 2 +- Core/src/Surfaces/CylinderBounds.cpp | 6 ++- 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/Core/src/Geometry/CylinderVolumeStack.cpp b/Core/src/Geometry/CylinderVolumeStack.cpp index 62f5d2b7bdc..f4b978647fc 100644 --- a/Core/src/Geometry/CylinderVolumeStack.cpp +++ b/Core/src/Geometry/CylinderVolumeStack.cpp @@ -153,6 +153,7 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction, assert(cylBounds != nullptr && "Volume bounds are not cylinder bounds"); Volume::update(std::make_shared(*cylBounds), std::nullopt, logger); + ACTS_VERBOSE("Transform is now: " << m_transform.matrix()); return; } @@ -642,6 +643,17 @@ std::pair CylinderVolumeStack::synchronizeZBounds( void CylinderVolumeStack::update(std::shared_ptr volbounds, std::optional transform, const Logger& logger) { + ACTS_DEBUG( + "Resizing CylinderVolumeStack with strategy: " << m_resizeStrategy); + ACTS_DEBUG("Currently have " << m_volumes.size() << " children"); + ACTS_DEBUG(m_gaps.size() << " gaps"); + for (const auto& v : m_volumes) { + ACTS_DEBUG(" - volume bounds: \n" << v->volumeBounds()); + ACTS_DEBUG(" transform: \n" << v->transform().matrix()); + } + + ACTS_DEBUG("New bounds are: \n" << *volbounds); + auto cylBounds = std::dynamic_pointer_cast(volbounds); if (cylBounds == nullptr) { throw std::invalid_argument( @@ -657,8 +669,10 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, return; } + ACTS_VERBOSE("Group transform is:\n" << m_groupTransform.matrix()); + ACTS_VERBOSE("Current transform is:\n" << m_transform.matrix()); if (transform.has_value()) { - ACTS_VERBOSE(transform.value().matrix()); + ACTS_VERBOSE("Input transform:\n" << transform.value().matrix()); } VolumeTuple oldVolume{*this, m_transform}; @@ -697,30 +711,36 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, const ActsScalar oldHlZ = oldVolume.halfLengthZ(); ACTS_VERBOSE("Previous bounds are: z: [ " - << oldMinZ << " <- " << oldMidZ << " -> " << oldMaxZ - << " ], r: [ " << oldMinR << " <-> " << oldMaxR << " ]"); + << oldMinZ << " <- " << oldMidZ << " -> " << oldMaxZ << " ] (" + << oldHlZ << "), r: [ " << oldMinR << " <-> " << oldMaxR + << " ]"); ACTS_VERBOSE("New bounds are: z: [ " - << newMinZ << " <- " << newMidZ << " -> " << newMaxZ - << " ], r: [ " << newMinR << " <-> " << newMaxR << " ]"); + << newMinZ << " <- " << newMidZ << " -> " << newMaxZ << " ] (" + << newHlZ << "), r: [ " << newMinR << " <-> " << newMaxR + << " ]"); if (newMinZ > oldMinZ) { - ACTS_ERROR("Shrinking the stack size in z is not supported"); - throw std::invalid_argument("Shrinking the stack is not supported"); + ACTS_ERROR("Shrinking the stack size in z is not supported: " + << newMinZ << " -> " << oldMinZ); + throw std::invalid_argument("Shrinking the stack in z is not supported"); } if (newMaxZ < oldMaxZ) { - ACTS_ERROR("Shrinking the stack size in z is not supported"); - throw std::invalid_argument("Shrinking the stack is not supported"); + ACTS_ERROR("Shrinking the stack size in z is not supported: " + << newMaxZ << " -> " << oldMaxZ); + throw std::invalid_argument("Shrinking the stack in z is not supported"); } if (newMinR > oldMinR) { - ACTS_ERROR("Shrinking the stack size in r is not supported"); - throw std::invalid_argument("Shrinking the stack is not supported"); + ACTS_ERROR("Shrinking the stack size in r is not supported: " + << newMinR << " -> " << oldMinR); + throw std::invalid_argument("Shrinking the stack in r is not supported"); } if (newMaxR < oldMaxR) { - ACTS_ERROR("Shrinking the stack size in r is not supported"); - throw std::invalid_argument("Shrinking the stack is not supported"); + ACTS_ERROR("Shrinking the stack size in r is not supported: " + << newMaxR << " -> " << oldMaxR); + throw std::invalid_argument("Shrinking the stack is r in not supported"); } if (m_direction == BinningValue::binZ) { diff --git a/Core/src/Geometry/GridPortalLink.cpp b/Core/src/Geometry/GridPortalLink.cpp index 2ad7ea11ae5..5e724b63120 100644 --- a/Core/src/Geometry/GridPortalLink.cpp +++ b/Core/src/Geometry/GridPortalLink.cpp @@ -89,7 +89,10 @@ void GridPortalLink::checkConsistency(const CylinderSurface& cyl) const { ActsScalar hlZ = cyl.bounds().get(CylinderBounds::eHalfLengthZ); if (!same(axis.getMin(), -hlZ) || !same(axis.getMax(), hlZ)) { throw std::invalid_argument( - "GridPortalLink: CylinderBounds: invalid length setup."); + "GridPortalLink: CylinderBounds: invalid length setup: " + + std::to_string(axis.getMin()) + " != " + std::to_string(-hlZ) + + " or " + std::to_string(axis.getMax()) + + " != " + std::to_string(hlZ)); } }; auto checkRPhi = [&cyl, same](const IAxis& axis) { diff --git a/Core/src/Geometry/TrivialPortalLink.cpp b/Core/src/Geometry/TrivialPortalLink.cpp index abcdb44929c..d0e9fd70b8a 100644 --- a/Core/src/Geometry/TrivialPortalLink.cpp +++ b/Core/src/Geometry/TrivialPortalLink.cpp @@ -39,7 +39,7 @@ Result TrivialPortalLink::resolveVolume( } void TrivialPortalLink::toStream(std::ostream& os) const { - os << "TrivialPortalLink"; + os << "TrivialPortalLinkvolumeName() << ">"; } } // namespace Acts diff --git a/Core/src/Surfaces/CylinderBounds.cpp b/Core/src/Surfaces/CylinderBounds.cpp index e7d5032c993..27123d29ec0 100644 --- a/Core/src/Surfaces/CylinderBounds.cpp +++ b/Core/src/Surfaces/CylinderBounds.cpp @@ -153,10 +153,12 @@ std::vector Acts::CylinderBounds::circleVertices( void Acts::CylinderBounds::checkConsistency() noexcept(false) { if (get(eR) <= 0.) { - throw std::invalid_argument("CylinderBounds: invalid radial setup."); + throw std::invalid_argument( + "CylinderBounds: invalid radial setup: radius is negative"); } if (get(eHalfLengthZ) <= 0.) { - throw std::invalid_argument("CylinderBounds: invalid length setup."); + throw std::invalid_argument( + "CylinderBounds: invalid length setup: half length is negative"); } if (get(eHalfPhiSector) <= 0. || get(eHalfPhiSector) > M_PI) { throw std::invalid_argument("CylinderBounds: invalid phi sector setup."); From d57fc22382fe670820e217dfc1632ff3f4eb981b Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 4 Oct 2024 11:31:11 +0200 Subject: [PATCH 4/5] fix: CylVolStack resizing issue This fixes a bug where the local transform was not correctly synced after resizing. Also improves a number of assertions to not rely on floating point identity anymore --- Core/src/Geometry/CylinderVolumeStack.cpp | 69 +++++++++++-------- .../Geometry/CylinderVolumeStackTests.cpp | 52 ++++++++++++++ 2 files changed, 93 insertions(+), 28 deletions(-) diff --git a/Core/src/Geometry/CylinderVolumeStack.cpp b/Core/src/Geometry/CylinderVolumeStack.cpp index f4b978647fc..c0346e5fe2f 100644 --- a/Core/src/Geometry/CylinderVolumeStack.cpp +++ b/Core/src/Geometry/CylinderVolumeStack.cpp @@ -363,8 +363,9 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( << (aZMidNew - aHlZNew) << " <- " << aZMidNew << " -> " << (aZMidNew + aHlZNew) << "]"); - assert(a.minZ() == aZMidNew - aHlZNew && "Volume shrunk"); - assert(a.maxZ() <= aZMidNew + aHlZNew && "Volume shrunk"); + assert(std::abs(a.minZ() - (aZMidNew - aHlZNew)) < 1e-9 && + "Volume shrunk"); + assert(aHlZNew >= a.halfLengthZ() && "Volume shrunk"); ActsScalar bZMidNew = (b.minZ() + b.maxZ()) / 2.0 - gapWidth / 4.0; ActsScalar bHlZNew = b.halfLengthZ() + gapWidth / 4.0; @@ -373,15 +374,16 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( << (bZMidNew - bHlZNew) << " <- " << bZMidNew << " -> " << (bZMidNew + bHlZNew) << "]"); - assert(b.minZ() >= bZMidNew - bHlZNew && "Volume shrunk"); - assert(b.maxZ() == bZMidNew + bHlZNew && "Volume shrunk"); + assert(bHlZNew >= b.halfLengthZ() && "Volume shrunk"); + assert(std::abs(b.maxZ() - (bZMidNew + bHlZNew)) < 1e-9 && + "Volume shrunk"); - a.localTransform = Translation3{0, 0, aZMidNew}; - a.volume->setTransform(m_groupTransform * a.localTransform); + a.setLocalTransform(Transform3{Translation3{0, 0, aZMidNew}}, + m_groupTransform); a.updatedBounds->set(CylinderVolumeBounds::eHalfLengthZ, aHlZNew); - b.localTransform = Translation3{0, 0, bZMidNew}; - b.volume->setTransform(m_groupTransform * b.localTransform); + b.setLocalTransform(Transform3{Translation3{0, 0, bZMidNew}}, + m_groupTransform); b.updatedBounds->set(CylinderVolumeBounds::eHalfLengthZ, bHlZNew); break; @@ -395,11 +397,12 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( << (aZMidNew - aHlZNew) << " <- " << aZMidNew << " -> " << (aZMidNew + aHlZNew) << "]"); - assert(a.minZ() == aZMidNew - aHlZNew && "Volume shrunk"); - assert(a.maxZ() <= aZMidNew + aHlZNew && "Volume shrunk"); + assert(std::abs(a.minZ() - (aZMidNew - aHlZNew)) < 1e-9 && + "Volume shrunk"); + assert(aHlZNew >= a.halfLengthZ() && "Volume shrunk"); - a.localTransform = Translation3{0, 0, aZMidNew}; - a.volume->setTransform(m_groupTransform * a.localTransform); + a.setLocalTransform(Transform3{Translation3{0, 0, aZMidNew}}, + m_groupTransform); a.updatedBounds->set(CylinderVolumeBounds::eHalfLengthZ, aHlZNew); break; @@ -413,11 +416,12 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( << (bZMidNew - bHlZNew) << " <- " << bZMidNew << " -> " << (bZMidNew + bHlZNew) << "]"); - assert(b.minZ() >= bZMidNew - bHlZNew && "Volume shrunk"); - assert(b.maxZ() == bZMidNew + bHlZNew && "Volume shrunk"); + assert(bHlZNew >= b.halfLengthZ() && "Volume shrunk"); + assert(std::abs(b.maxZ() - (bZMidNew + bHlZNew)) < 1e-9 && + "Volume shrunk"); - b.localTransform = Translation3{0, 0, bZMidNew}; - b.volume->setTransform(m_groupTransform * b.localTransform); + b.setLocalTransform(Transform3{Translation3{0, 0, bZMidNew}}, + m_groupTransform); b.updatedBounds->set(CylinderVolumeBounds::eHalfLengthZ, bHlZNew); break; } @@ -429,10 +433,13 @@ CylinderVolumeStack::checkOverlapAndAttachInZ( ACTS_VERBOSE(" - Gap half length: " << gapHlZ << " at z: " << gapMidZ); + ActsScalar minR = std::min(a.minR(), b.minR()); + ActsScalar maxR = std::max(a.maxR(), b.maxR()); + Transform3 gapLocalTransform{Translation3{0, 0, gapMidZ}}; Transform3 gapGlobalTransform = m_groupTransform * gapLocalTransform; - auto gapBounds = std::make_shared( - a.minR(), b.maxR(), gapHlZ); + auto gapBounds = + std::make_shared(minR, maxR, gapHlZ); auto gap = addGapVolume(gapGlobalTransform, gapBounds); gapVolumes.emplace_back(*gap, m_groupTransform); @@ -679,8 +686,7 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, VolumeTuple newVolume{*this, m_transform}; newVolume.updatedBounds = std::make_shared(*cylBounds); newVolume.globalTransform = transform.value_or(m_transform); - newVolume.localTransform = - m_groupTransform.inverse() * newVolume.globalTransform; + newVolume.localTransform = m_transform.inverse() * newVolume.globalTransform; if (!transform.has_value()) { ACTS_VERBOSE("Local transform does not change"); @@ -719,25 +725,30 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, << newHlZ << "), r: [ " << newMinR << " <-> " << newMaxR << " ]"); - if (newMinZ > oldMinZ) { + constexpr auto tolerance = s_onSurfaceTolerance; + auto same = [](ActsScalar a, ActsScalar b) { + return std::abs(a - b) < tolerance; + }; + + if (!same(newMinZ, oldMinZ) && newMinZ > oldMinZ) { ACTS_ERROR("Shrinking the stack size in z is not supported: " << newMinZ << " -> " << oldMinZ); throw std::invalid_argument("Shrinking the stack in z is not supported"); } - if (newMaxZ < oldMaxZ) { + if (!same(newMaxZ, oldMaxZ) && newMaxZ < oldMaxZ) { ACTS_ERROR("Shrinking the stack size in z is not supported: " << newMaxZ << " -> " << oldMaxZ); throw std::invalid_argument("Shrinking the stack in z is not supported"); } - if (newMinR > oldMinR) { + if (!same(newMinR, oldMinR) && newMinR > oldMinR) { ACTS_ERROR("Shrinking the stack size in r is not supported: " << newMinR << " -> " << oldMinR); throw std::invalid_argument("Shrinking the stack in r is not supported"); } - if (newMaxR < oldMaxR) { + if (!same(newMaxR, oldMaxR) && newMaxR < oldMaxR) { ACTS_ERROR("Shrinking the stack size in r is not supported: " << newMaxR << " -> " << oldMaxR); throw std::invalid_argument("Shrinking the stack is r in not supported"); @@ -757,7 +768,7 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, ACTS_VERBOSE("*** Initial volume configuration:"); printVolumeSequence(volumeTuples, logger, Acts::Logging::DEBUG); - if (newMinR != oldMinR || newMaxR != oldMaxR) { + if (!same(newMinR, oldMinR) || !same(newMaxR, oldMaxR)) { ACTS_VERBOSE("Resize all volumes to new r bounds"); for (auto& volume : volumeTuples) { volume.set({ @@ -771,7 +782,7 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, ACTS_VERBOSE("R bounds are the same, no r resize needed"); } - if (newHlZ == oldHlZ) { + if (same(newHlZ, oldHlZ)) { ACTS_VERBOSE("Halflength z is the same, no z resize needed"); } else { if (m_resizeStrategy == ResizeStrategy::Expand) { @@ -811,7 +822,7 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, } else if (m_resizeStrategy == ResizeStrategy::Gap) { ACTS_VERBOSE("Creating gap volumes to fill the new z bounds"); - if (newMinZ < oldMinZ) { + if (!same(newMinZ, oldMinZ) && newMinZ < oldMinZ) { ACTS_VERBOSE("Adding gap volume at negative z"); ActsScalar gap1MinZ = newVolume.minZ(); @@ -831,7 +842,7 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, VolumeTuple{*gap1, m_groupTransform}); } - if (newMaxZ > oldMaxZ) { + if (!same(newMaxZ, oldMaxZ) && newMaxZ > oldMaxZ) { ACTS_VERBOSE("Adding gap volume at positive z"); ActsScalar gap2MinZ = oldVolume.maxZ(); @@ -956,6 +967,8 @@ void CylinderVolumeStack::update(std::shared_ptr volbounds, } m_transform = newVolume.globalTransform; + // @TODO: We probably can reuse m_transform + m_groupTransform = m_transform; Volume::update(std::move(cylBounds), std::nullopt, logger); } diff --git a/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp b/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp index ab3c23a3dfc..6593ae64c36 100644 --- a/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp +++ b/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp @@ -22,6 +22,7 @@ #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/BinningType.hpp" #include "Acts/Utilities/Logger.hpp" +#include "Acts/Utilities/TrackHelpers.hpp" #include "Acts/Utilities/Zip.hpp" using namespace Acts::UnitLiterals; @@ -813,6 +814,57 @@ BOOST_DATA_TEST_CASE( } } +BOOST_AUTO_TEST_CASE(ResizeReproduction1) { + Transform3 trf1 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * -2000}; + auto bounds1 = std::make_shared(70, 100, 100.0); + Volume vol1{trf1, bounds1}; + + std::vector volumes = {&vol1}; + CylinderVolumeStack stack(volumes, BinningValue::binZ, + CylinderVolumeStack::AttachmentStrategy::Gap, + CylinderVolumeStack::ResizeStrategy::Gap, *logger); + + Transform3 trf2 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * -1500}; + stack.update(std::make_shared(30.0, 100, 600), trf2, + *logger); + + std::cout << stack.volumeBounds() << std::endl; + std::cout << stack.transform().matrix() << std::endl; + + Transform3 trf3 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * -1600}; + stack.update(std::make_shared(30.0, 100, 700), trf3, + *logger); +} + +BOOST_AUTO_TEST_CASE(ResizeReproduction2) { + // The numbers are tuned a bit to reproduce the faulty behavior + Transform3 trf1 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * 263}; + auto bounds1 = std::make_shared(30, 100, 4.075); + Volume vol1{trf1, bounds1}; + + std::vector volumes = {&vol1}; + CylinderVolumeStack stack(volumes, BinningValue::binZ, + CylinderVolumeStack::AttachmentStrategy::Gap, + CylinderVolumeStack::ResizeStrategy::Gap, *logger); + + Transform3 trf2 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * 260.843}; + stack.update(std::make_shared(30.0, 100, 6.232), trf2, + *logger); + + std::cout << stack.volumeBounds() << std::endl; + std::cout << stack.transform().matrix() << std::endl; + + Transform3 trf3 = + Transform3::Identity() * Translation3{Vector3::UnitZ() * 1627.31}; + stack.update(std::make_shared(30.0, 100, 1372.699), + trf3, *logger); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE(RDirection) From ddb4d46a4e7127e9663b0272ea1e42160988af12 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 11 Oct 2024 10:52:39 +0200 Subject: [PATCH 5/5] remove unused header --- Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp b/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp index 6593ae64c36..cdd56a22899 100644 --- a/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp +++ b/Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp @@ -22,7 +22,6 @@ #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/BinningType.hpp" #include "Acts/Utilities/Logger.hpp" -#include "Acts/Utilities/TrackHelpers.hpp" #include "Acts/Utilities/Zip.hpp" using namespace Acts::UnitLiterals;