fix: DetectorVolume containment check

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"