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

@@ -484,6 +484,9 @@ class DetectorVolume : public std::enable_shared_from_this<DetectorVolume> {
 /// @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<std::shared_ptr<Surface>>& surfaces,
       const std::vector<std::shared_ptr<DetectorVolume>>& 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;
   }
 

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<Acts::BinningValue> canonicalBinning() const override {
+    return {Acts::binX, Acts::binY, Acts::binZ};
+  };
+
   /// Binning borders in double
   ///
   /// @param bValue is the binning schema used

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<Acts::BinningValue> canonicalBinning() const override {
+    return {Acts::binR, Acts::binPhi, Acts::binZ};
+  };
+
   /// Write information about this instance to an outstream
   ///
   /// @param sl The outstream

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<Acts::BinningValue> 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

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<Acts::BinningValue> 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;
 

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<Acts::BinningValue> canonicalBinning() const {
+    return {Acts::binX, Acts::binY, Acts::binZ};
+  };
+
   /// Binning offset - overloaded for some R-binning types
   ///
   /// @param bValue is the binning schema used

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

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

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::CylinderSurface>(
+      Acts::Transform3::Identity() * Acts::Translation3(-1000, 0., 0.),
+      std::make_shared<Acts::CylinderBounds>(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<Acts::CuboidVolumeBounds>(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::PlaneSurface>(
+      Acts::Transform3::Identity() * Acts::Translation3(0, 0., 0.),
+      std::make_shared<Acts::RectangleBounds>(1, 100));
+
+  BOOST_CHECK_THROW(
+      Acts::Experimental::DetectorVolumeFactory::construct(
+          portalGenerator, Acts::GeometryContext(), "CubeWithSurfaceTooBig",
+          Acts::Transform3::Identity(),
+          std::make_shared<Acts::CuboidVolumeBounds>(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<Acts::CuboidVolumeBounds>(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<Acts::CuboidVolumeBounds>(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<Acts::CuboidVolumeBounds>(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<Acts::CuboidVolumeBounds>(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.;

Tests/UnitTests/Plugins/DD4hep/DD4hepCylindricalDetectorTests.cpp

@@ -160,10 +160,10 @@ Acts::Test::CylindricalTrackingGeometry::DetectorStore generateXML() {
   std::vector<std::array<Acts::ActsScalar, 2u>> 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 << "<acts_passive_surface>" << '\n';
   cxml << indent_12_xml
-       << "<tubs rmin=\"25*mm\" rmax=\"25.8*mm\" dz=\"1800*mm\" "
+       << "<tubs rmin=\"19*mm\" rmax=\"20*mm\" dz=\"800*mm\" "
           "material=\"Air\"/>"
        << '\n';
   cxml << indent_12_xml << "</acts_passive_surface>" << '\n';

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 =

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"