From d244dee611dc50d6a84564b536228846c0d235a6 Mon Sep 17 00:00:00 2001 From: ssdetlab <113530373+ssdetlab@users.noreply.github.com> Date: Wed, 13 Mar 2024 13:36:02 +0200 Subject: [PATCH] fix: DetectorVolume containment check (#2895) The PR makes changes to the DetectorVolume code that address the issue of the surfaces/subvolumes containment checks, i.e. making sure that they are fully inside the volume. Additionally, minor fixes are made in the Json-related parts of the code (see below). DetectorVolume checkContainment call is moved from the constructor to the end of the "construct" method, where the volume's portals are initialized. Because the containment checks rely on the volume having the initialized Extent and the Extent is derived from the volume's portals, keeping the call in the constructor prevents the proper check from happening, as there are yet no portals to derive the Extent from. Additionally, assert is replaced with invalid_argument thrown in case the internals are not contained. To check the compatibility of the external and internal extents a new "canonicalBinning" method is introduced to the VolumeBounds. It returns the binning values that should be enough to describe the relevant parts of the Extent for a given volume shape, e.g. X,Y,Z bins are returned for cubic volumes, as, for example, there is no reason to check the binMag. Some of the VolumeBounds return the bounding box binning for now as a placeholder. A UnitTest is added to the DetectorVolumeTests that checks incompatibility with internals that are not contained. A minor fixes had to be introduced to DetectorJsonConverter with respect to the geometry parameters. In Transform3 Json writer the default option "transpose" is changed to be false to be consistent with the default reading options. --- Core/include/Acts/Detector/DetectorVolume.hpp | 13 +++- .../Acts/Geometry/CuboidVolumeBounds.hpp | 8 +++ .../Geometry/CutoutCylinderVolumeBounds.hpp | 8 +++ .../Acts/Geometry/CylinderVolumeBounds.hpp | 8 +++ .../Geometry/GenericCuboidVolumeBounds.hpp | 8 +++ Core/include/Acts/Geometry/VolumeBounds.hpp | 12 ++++ Core/src/Detector/DetectorVolume.cpp | 28 +++++--- .../Plugins/Json/AlgebraJsonConverter.hpp | 2 +- .../Core/Detector/DetectorVolumeTests.cpp | 71 +++++++++++++++++++ .../DD4hep/DD4hepCylindricalDetectorTests.cpp | 6 +- .../Json/DetectorJsonConverterTests.cpp | 6 +- .../Json/DetectorVolumeJsonConverterTests.cpp | 1 + 12 files changed, 152 insertions(+), 19 deletions(-) diff --git a/Core/include/Acts/Detector/DetectorVolume.hpp b/Core/include/Acts/Detector/DetectorVolume.hpp index d16a06ca9cc..c0e6a91fb8c 100644 --- a/Core/include/Acts/Detector/DetectorVolume.hpp +++ b/Core/include/Acts/Detector/DetectorVolume.hpp @@ -484,6 +484,9 @@ class DetectorVolume : public std::enable_shared_from_this { /// @brief A detector volume factory which first constructs the detector volume /// and then constructs the portals. This ensures that the std::shared_ptr /// holding the detector volume is not weak when assigning to the portals. +/// +/// @note Optional containment check is invoked by setting the number +/// of segments nSeg to be greater than 0 class DetectorVolumeFactory { public: /// Create a detector volume - from factory @@ -494,11 +497,19 @@ class DetectorVolumeFactory { const std::vector>& surfaces, const std::vector>& volumes, DetectorVolumeUpdater detectorVolumeUpdater, - SurfaceCandidatesUpdater surfaceCandidateUpdater) { + SurfaceCandidatesUpdater surfaceCandidateUpdater, int nSeg = -1) { auto dVolume = DetectorVolume::makeShared( gctx, name, transform, std::move(bounds), surfaces, volumes, std::move(detectorVolumeUpdater), std::move(surfaceCandidateUpdater)); dVolume->construct(gctx, portalGenerator); + + /// Volume extent is constructed from the portals + /// So the surface/subvolume containment + /// check has to happen here + if (nSeg > 0 && !dVolume->checkContainment(gctx, nSeg)) { + throw std::invalid_argument( + "DetectorVolume: surfaces or subvolumes are not contained by volume"); + } return dVolume; } diff --git a/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp b/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp index 52f8697688b..a56636cc06b 100644 --- a/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CuboidVolumeBounds.hpp @@ -122,6 +122,14 @@ class CuboidVolumeBounds : public VolumeBounds { const Vector3& envelope = {0, 0, 0}, const Volume* entity = nullptr) const final; + /// Get the canonical binning values, i.e. the binning values + /// for that fully describe the shape's extent + /// + /// @return vector of canonical binning values + std::vector canonicalBinning() const override { + return {Acts::binX, Acts::binY, Acts::binZ}; + }; + /// Binning borders in double /// /// @param bValue is the binning schema used diff --git a/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp b/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp index 52031d2514f..3033b83595e 100644 --- a/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp @@ -114,6 +114,14 @@ class CutoutCylinderVolumeBounds : public VolumeBounds { const Vector3& envelope = {0, 0, 0}, const Volume* entity = nullptr) const final; + /// Get the canonical binning values, i.e. the binning values + /// for that fully describe the shape's extent + /// + /// @return vector of canonical binning values + std::vector canonicalBinning() const override { + return {Acts::binR, Acts::binPhi, Acts::binZ}; + }; + /// Write information about this instance to an outstream /// /// @param sl The outstream diff --git a/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp b/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp index 4f8eb9dddca..c6f60f555cf 100644 --- a/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/CylinderVolumeBounds.hpp @@ -172,6 +172,14 @@ class CylinderVolumeBounds : public VolumeBounds { const Vector3& envelope = {0, 0, 0}, const Volume* entity = nullptr) const final; + /// Get the canonical binning values, i.e. the binning values + /// for that fully describe the shape's extent + /// + /// @return vector of canonical binning values + std::vector canonicalBinning() const override { + return {Acts::binR, Acts::binPhi, Acts::binZ}; + }; + /// Binning offset - overloaded for some R-binning types /// /// @param bValue is the type used for the binning diff --git a/Core/include/Acts/Geometry/GenericCuboidVolumeBounds.hpp b/Core/include/Acts/Geometry/GenericCuboidVolumeBounds.hpp index d205f51d92a..138ab0d7692 100644 --- a/Core/include/Acts/Geometry/GenericCuboidVolumeBounds.hpp +++ b/Core/include/Acts/Geometry/GenericCuboidVolumeBounds.hpp @@ -89,6 +89,14 @@ class GenericCuboidVolumeBounds : public VolumeBounds { const Vector3& envelope = {0, 0, 0}, const Volume* entity = nullptr) const final; + /// Get the canonical binning values, i.e. the binning values + /// for that fully describe the shape's extent + /// + /// @return vector of canonical binning values + std::vector canonicalBinning() const override { + return {Acts::binX, Acts::binY, Acts::binZ}; + }; + /// @param sl is the output stream to be written into std::ostream& toStream(std::ostream& sl) const override; diff --git a/Core/include/Acts/Geometry/VolumeBounds.hpp b/Core/include/Acts/Geometry/VolumeBounds.hpp index 860977172f7..93cb16a15a5 100644 --- a/Core/include/Acts/Geometry/VolumeBounds.hpp +++ b/Core/include/Acts/Geometry/VolumeBounds.hpp @@ -115,6 +115,18 @@ class VolumeBounds { const Transform3* trf = nullptr, const Vector3& envelope = {0, 0, 0}, const Volume* entity = nullptr) const = 0; + /// Get the canonical binning values, i.e. the binning values + /// for that fully describe the shape's extent + /// + /// @return vector of canonical binning values + /// + /// @note This is the default implementation that + /// returns the bounding box binning. Individual shapes + /// should override this method + virtual std::vector canonicalBinning() const { + return {Acts::binX, Acts::binY, Acts::binZ}; + }; + /// Binning offset - overloaded for some R-binning types /// /// @param bValue is the binning schema used diff --git a/Core/src/Detector/DetectorVolume.cpp b/Core/src/Detector/DetectorVolume.cpp index c233f36ae3b..c0edaf2d11d 100644 --- a/Core/src/Detector/DetectorVolume.cpp +++ b/Core/src/Detector/DetectorVolume.cpp @@ -55,7 +55,6 @@ Acts::Experimental::DetectorVolume::DetectorVolume( } [[maybe_unused]] const auto& gctx_ref = gctx; - assert(checkContainment(gctx) && "Objects are not contained by volume."); } Acts::Experimental::DetectorVolume::DetectorVolume( @@ -264,20 +263,27 @@ Acts::Extent Acts::Experimental::DetectorVolume::extent( bool Acts::Experimental::DetectorVolume::checkContainment( const GeometryContext& gctx, std::size_t nseg) const { + // We don't have a logging instance here + // so can't throw a warning for shapes that are + // using the bounding box + auto binningValues = volumeBounds().canonicalBinning(); + // Create the volume extent auto volumeExtent = extent(gctx, nseg); // Check surfaces - for (const auto* s : surfaces()) { - auto sExtent = s->polyhedronRepresentation(gctx, nseg).extent(); - if (!volumeExtent.contains(sExtent)) { - return false; + for (auto b : binningValues) { + for (const auto* s : surfaces()) { + auto sExtent = s->polyhedronRepresentation(gctx, nseg).extent(); + if (!volumeExtent.contains(sExtent, b)) { + return false; + } } - } - // Check volumes - for (const auto* v : volumes()) { - auto vExtent = v->extent(gctx, nseg); - if (!volumeExtent.contains(vExtent)) { - return false; + // Check volumes + for (const auto* v : volumes()) { + auto vExtent = v->extent(gctx, nseg); + if (!volumeExtent.contains(vExtent, b)) { + return false; + } } } // All contained diff --git a/Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp b/Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp index b54cef52eca..b93a8703795 100644 --- a/Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp +++ b/Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp @@ -28,7 +28,7 @@ struct Options { /// Write the identity transform explicitly bool writeIdentity = false; /// Apply a transpose to flip column/row order - bool transpose = true; + bool transpose = false; }; /// @brief The Transform converter to json diff --git a/Tests/UnitTests/Core/Detector/DetectorVolumeTests.cpp b/Tests/UnitTests/Core/Detector/DetectorVolumeTests.cpp index 91b861a1f43..c93ebd345e8 100644 --- a/Tests/UnitTests/Core/Detector/DetectorVolumeTests.cpp +++ b/Tests/UnitTests/Core/Detector/DetectorVolumeTests.cpp @@ -25,6 +25,8 @@ #include "Acts/Navigation/SurfaceCandidatesUpdaters.hpp" #include "Acts/Surfaces/CylinderBounds.hpp" #include "Acts/Surfaces/CylinderSurface.hpp" +#include "Acts/Surfaces/PlaneSurface.hpp" +#include "Acts/Surfaces/RectangleBounds.hpp" #include "Acts/Surfaces/Surface.hpp" #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/BinningType.hpp" @@ -53,6 +55,75 @@ Acts::GeometryContext tContext; BOOST_AUTO_TEST_SUITE(Detector) +BOOST_AUTO_TEST_CASE(SurfaceVolumeContainment) { + // Create a surface that is placed way off + auto surfaceOutOfBounds = Acts::Surface::makeShared( + Acts::Transform3::Identity() * Acts::Translation3(-1000, 0., 0.), + std::make_shared(1, 1)); + + auto vBounds = Acts::CuboidVolumeBounds(10.0, 10.0, 10.0); + auto portalGenerator = + Acts::Experimental::defaultPortalAndSubPortalGenerator(); + BOOST_CHECK_THROW( + Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), + "CubeWithOutofBoundsSurface", Acts::Transform3::Identity(), + std::make_shared(vBounds), + {surfaceOutOfBounds}, {}, Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000), + std::invalid_argument); + + // Create a surface that is too big + auto surfaceTooBig = Acts::Surface::makeShared( + Acts::Transform3::Identity() * Acts::Translation3(0, 0., 0.), + std::make_shared(1, 100)); + + BOOST_CHECK_THROW( + Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), "CubeWithSurfaceTooBig", + Acts::Transform3::Identity(), + std::make_shared(vBounds), {surfaceTooBig}, + {}, Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000), + std::invalid_argument); + + // Envelope a bigger volume into a smaller one + auto bigVolume = Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), "BigCube", + Acts::Transform3::Identity(), + std::make_shared(vBounds), {}, {}, + Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000); + + auto smallBounds = Acts::CuboidVolumeBounds(1.0, 1.0, 1.0); + BOOST_CHECK_THROW( + Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), + "SmallCubeWithBigCubeInside", Acts::Transform3::Identity(), + std::make_shared(smallBounds), {}, + {bigVolume}, Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000), + std::invalid_argument); + + // Envelope a misaligned subvolume + auto smallVolumeMisaligned = + Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), "SmallCubeMisaligned", + Acts::Transform3::Identity() * Acts::Translation3(9.5, 0., 0.), + std::make_shared(smallBounds), {}, {}, + Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000); + + BOOST_CHECK_THROW( + Acts::Experimental::DetectorVolumeFactory::construct( + portalGenerator, Acts::GeometryContext(), "CubeWithMisalignedVolume", + Acts::Transform3::Identity(), + std::make_shared(vBounds), {}, + {smallVolumeMisaligned}, Acts::Experimental::tryAllSubVolumes(), + Acts::Experimental::tryAllPortalsAndSurfaces(), 1000), + std::invalid_argument); +} + BOOST_AUTO_TEST_CASE(CylindricalDetectorVolumePortals) { Acts::ActsScalar rInner = 10.; Acts::ActsScalar rOuter = 100.; diff --git a/Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp b/Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp index a565bad3dca..894bfb89cce 100644 --- a/Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp +++ b/Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp @@ -160,10 +160,10 @@ Acts::Test::CylindricalTrackingGeometry::DetectorStore generateXML() { std::vector> innerOuter = { {25., 35.}, {65., 75.}, {110., 120.}}; auto b0Surfaces = cGeometry.surfacesCylinder(dStore, 8.4, 36., 0.15, 0.14, - 32., 3., 2., {16, 14}); + 31., 3., 2., {16, 14}); auto b1Surfaces = cGeometry.surfacesCylinder(dStore, 8.4, 36., 0.15, 0.14, - 72., 3., 2., {32, 14}); + 71., 3., 2., {32, 14}); auto b2Surfaces = cGeometry.surfacesCylinder(dStore, 8.4, 36., 0.15, 0.14, 116., 3., 2., {52, 14}); @@ -192,7 +192,7 @@ Acts::Test::CylindricalTrackingGeometry::DetectorStore generateXML() { cxml << beampipe_head_xml << '\n'; cxml << indent_12_xml << "" << '\n'; cxml << indent_12_xml - << "" << '\n'; cxml << indent_12_xml << "" << '\n'; diff --git a/Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp index b4508076a2c..39ede9a27f8 100644 --- a/Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp @@ -178,7 +178,7 @@ BOOST_AUTO_TEST_CASE(BeamPipeEndcapBarrelDetector) { Acts::Logging::VERBOSE)); Acts::Experimental::VolumeStructureBuilder::Config shapeConfig; - shapeConfig.boundValues = {20, 100, 10., M_PI, 0.}; + shapeConfig.boundValues = {18, 100, 10., M_PI, 0.}; shapeConfig.transform = Acts::Transform3(Acts::Transform3::Identity()) .pretranslate(Acts::Vector3(0., 0., ep)); shapeConfig.boundsType = Acts::VolumeBounds::BoundsType::eCylinder; @@ -202,7 +202,7 @@ BOOST_AUTO_TEST_CASE(BeamPipeEndcapBarrelDetector) { // Central barrel Acts::Experimental::VolumeStructureBuilder::Config innerShapeConfig; - innerShapeConfig.boundValues = {20., 60., 700., M_PI, 0.}; + innerShapeConfig.boundValues = {18., 60., 700., M_PI, 0.}; innerShapeConfig.boundsType = Acts::VolumeBounds::BoundsType::eCylinder; auto innerShapeBuilder = @@ -298,7 +298,7 @@ BOOST_AUTO_TEST_CASE(BeamPipeEndcapBarrelDetector) { // Beam Pipe Acts::Experimental::VolumeStructureBuilder::Config bpShapeConfig; - bpShapeConfig.boundValues = {0., 20., 720., M_PI, 0.}; + bpShapeConfig.boundValues = {0., 18., 720., M_PI, 0.}; bpShapeConfig.boundsType = Acts::VolumeBounds::BoundsType::eCylinder; auto bpShapeBuilder = diff --git a/Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp b/Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp index 2cf710fc7e8..27ed95d7bf2 100644 --- a/Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Json/DetectorVolumeJsonConverterTests.cpp @@ -14,6 +14,7 @@ #include "Acts/Detector/LayerStructureBuilder.hpp" #include "Acts/Detector/PortalGenerators.hpp" #include "Acts/Detector/VolumeStructureBuilder.hpp" +#include "Acts/Geometry/ConeVolumeBounds.hpp" #include "Acts/Geometry/CylinderVolumeBounds.hpp" #include "Acts/Geometry/GeometryContext.hpp" #include "Acts/Navigation/DetectorVolumeFinders.hpp"