refactor: Volume holds bounds as mutable (#3595)

This PR changes the internal storage of `VolumeBounds` inside `Volume` to be mutable. This allows transitive mutable access (which we need in a few cases).

To prevent accidental mutable use of the contained bounds object (which can be shared with other objects), I'm moving the shared pointer member to be private, and allow access only via setters and getters. This way, in a const function of a derived class, only const access to the volume bounds is possible.

Core/include/Acts/Geometry/CylinderVolumeStack.hpp

@@ -87,7 +87,7 @@ class CylinderVolumeStack : public Volume {
   /// @param transform is the new transform
   /// @pre The volume bounds need to be of type
   ///      @c CylinderVolumeBounds.
-  void update(std::shared_ptr<const VolumeBounds> volbounds,
+  void update(std::shared_ptr<VolumeBounds> volbounds,
               std::optional<Transform3> transform = std::nullopt) override;
 
   /// Update the volume bounds and transform. This
@@ -100,7 +100,7 @@ class CylinderVolumeStack : public Volume {
   /// @param logger is the logger
   /// @pre The volume bounds need to be of type
   ///      @c CylinderVolumeBounds.
-  void update(std::shared_ptr<const CylinderVolumeBounds> newBounds,
+  void update(std::shared_ptr<CylinderVolumeBounds> newBounds,
               std::optional<Transform3> transform, const Logger& logger);
 
   /// Access the gap volume that were created during attachment or resizing.

Core/include/Acts/Geometry/TrackingVolume.hpp

@@ -125,7 +125,7 @@ class TrackingVolume : public Volume {
   /// @param volbounds is the description of the volume boundaries
   /// @param volumeName is a string identifier
   TrackingVolume(const Transform3& transform,
-                 std::shared_ptr<const VolumeBounds> volbounds,
+                 std::shared_ptr<VolumeBounds> volbounds,
                  const std::string& volumeName = "undefined");
 
   /// Constructor for a full equipped Tracking Volume
@@ -140,8 +140,7 @@ class TrackingVolume : public Volume {
   /// @param denseVolumeVector  The contained dense volumes
   /// @param volumeName is a string identifier
   TrackingVolume(
-      const Transform3& transform,
-      std::shared_ptr<const VolumeBounds> volumeBounds,
+      const Transform3& transform, std::shared_ptr<VolumeBounds> volumeBounds,
       std::shared_ptr<const IVolumeMaterial> volumeMaterial,
       std::unique_ptr<const LayerArray> staticLayerArray = nullptr,
       std::shared_ptr<const TrackingVolumeArray> containedVolumeArray = nullptr,
@@ -151,8 +150,7 @@ class TrackingVolume : public Volume {
   /// Constructor from a regular volume
   /// @param volume is the volume to be converted
   /// @param volumeName is a string identifier
-  TrackingVolume(const Volume& volume,
-                 const std::string& volumeName = "undefined");
+  TrackingVolume(Volume& volume, const std::string& volumeName = "undefined");
 
   // @TODO: This needs to be refactored to include Gen3 volumes
   /// Return the associated sub Volume, returns THIS if no subVolume exists

Core/include/Acts/Geometry/Volume.hpp

@@ -37,8 +37,7 @@ class Volume : public GeometryObject {
   ///
   /// @param transform is the transform to position the volume in 3D space
   /// @param volbounds is the volume boundary definitions
-  Volume(const Transform3& transform,
-         std::shared_ptr<const VolumeBounds> volbounds);
+  Volume(const Transform3& transform, std::shared_ptr<VolumeBounds> volbounds);
 
   /// Copy Constructor - with optional shift
   ///
@@ -65,20 +64,26 @@ class Volume : public GeometryObject {
   /// returns the center of the volume
   const Vector3& center() const;
 
-  /// Returns const reference to the volume bounds
+  /// Returns a const reference to the volume bounds
   const VolumeBounds& volumeBounds() const;
 
+  /// Returns a mutable reference to the volume bounds
+  VolumeBounds& volumeBounds();
+
   /// Returns shared pointer to the volume bounds
   std::shared_ptr<const VolumeBounds> volumeBoundsPtr() const;
 
+  /// Returns shared pointer to the volume bounds
+  std::shared_ptr<VolumeBounds> volumeBoundsPtr();
+
   /// Set volume bounds and update volume bounding boxes implicitly
   /// @param volbounds The volume bounds to be assigned
-  void assignVolumeBounds(std::shared_ptr<const VolumeBounds> volbounds);
+  void assignVolumeBounds(std::shared_ptr<VolumeBounds> volbounds);
 
   /// 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
-  virtual void update(std::shared_ptr<const VolumeBounds> volbounds,
+  virtual void update(std::shared_ptr<VolumeBounds> volbounds,
                       std::optional<Transform3> transform = std::nullopt);
 
   /// Construct bounding box for this shape
@@ -117,7 +122,9 @@ class Volume : public GeometryObject {
   Transform3 m_transform;
   Transform3 m_itransform;
   Vector3 m_center;
-  std::shared_ptr<const VolumeBounds> m_volumeBounds;
+
+ private:
+  std::shared_ptr<VolumeBounds> m_volumeBounds;
 };
 
 /**Overload of << operator for std::ostream for debug output*/

Core/src/Geometry/CylinderVolumeStack.cpp

@@ -150,7 +150,7 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction,
     const auto* cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
         &m_volumes.front()->volumeBounds());
     assert(cylBounds != nullptr && "Volume bounds are not cylinder bounds");
-    m_volumeBounds = std::make_shared<CylinderVolumeBounds>(*cylBounds);
+    Volume::update(std::make_shared<CylinderVolumeBounds>(*cylBounds));
     return;
   }
 
@@ -211,8 +211,8 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction,
 
     m_transform = m_groupTransform * Translation3{0, 0, midZ};
 
-    m_volumeBounds = std::make_shared<CylinderVolumeBounds>(minR, maxR, hlZ);
-    ACTS_DEBUG("Outer bounds are:\n" << *m_volumeBounds);
+    Volume::update(std::make_shared<CylinderVolumeBounds>(minR, maxR, hlZ));
+    ACTS_DEBUG("Outer bounds are:\n" << volumeBounds());
     ACTS_DEBUG("Outer transform / new group transform is:\n"
                << m_transform.matrix());
 
@@ -269,9 +269,9 @@ void CylinderVolumeStack::initializeOuterVolume(BinningValue direction,
 
     m_transform = m_groupTransform * Translation3{0, 0, midZ};
 
-    m_volumeBounds = std::make_shared<CylinderVolumeBounds>(minR, maxR, hlZ);
+    Volume::update(std::make_shared<CylinderVolumeBounds>(minR, maxR, hlZ));
 
-    ACTS_DEBUG("Outer bounds are:\n" << *m_volumeBounds);
+    ACTS_DEBUG("Outer bounds are:\n" << volumeBounds());
     ACTS_DEBUG("Outer transform is:\n" << m_transform.matrix());
 
     // Update group transform to the new center
@@ -625,13 +625,12 @@ std::pair<ActsScalar, ActsScalar> CylinderVolumeStack::synchronizeZBounds(
   return {minZ, maxZ};
 }
 
-void CylinderVolumeStack::update(std::shared_ptr<const VolumeBounds> volbounds,
+void CylinderVolumeStack::update(std::shared_ptr<VolumeBounds> volbounds,
                                  std::optional<Transform3> transform) {
   if (volbounds == nullptr) {
     throw std::invalid_argument("New bounds are nullptr");
   }
-  auto cylBounds =
-      std::dynamic_pointer_cast<const CylinderVolumeBounds>(volbounds);
+  auto cylBounds = std::dynamic_pointer_cast<CylinderVolumeBounds>(volbounds);
   if (cylBounds == nullptr) {
     throw std::invalid_argument(
         "CylinderVolumeStack requires CylinderVolumeBounds");
@@ -641,7 +640,7 @@ void CylinderVolumeStack::update(std::shared_ptr<const VolumeBounds> volbounds,
 }
 
 void CylinderVolumeStack::update(
-    std::shared_ptr<const CylinderVolumeBounds> newBounds,
+    std::shared_ptr<CylinderVolumeBounds> newBounds,
     std::optional<Transform3> transform, const Logger& logger) {
   ACTS_DEBUG(
       "Resizing CylinderVolumeStack with strategy: " << m_resizeStrategy);
@@ -934,7 +933,7 @@ void CylinderVolumeStack::update(
   }
 
   m_transform = newVolume.globalTransform;
-  m_volumeBounds = std::move(newBounds);
+  Volume::update(std::move(newBounds));
 }
 
 void CylinderVolumeStack::checkNoPhiOrBevel(const CylinderVolumeBounds& bounds,

Core/src/Geometry/TrackingVolume.cpp

@@ -31,8 +31,7 @@ namespace Acts {
 
 // constructor for arguments
 TrackingVolume::TrackingVolume(
-    const Transform3& transform,
-    std::shared_ptr<const VolumeBounds> volumeBounds,
+    const Transform3& transform, std::shared_ptr<VolumeBounds> volumeBounds,
     std::shared_ptr<const IVolumeMaterial> volumeMaterial,
     std::unique_ptr<const LayerArray> staticLayerArray,
     std::shared_ptr<const TrackingVolumeArray> containedVolumeArray,
@@ -49,14 +48,13 @@ TrackingVolume::TrackingVolume(
   connectDenseBoundarySurfaces(denseVolumeVector);
 }
 
-TrackingVolume::TrackingVolume(const Volume& volume,
-                               const std::string& volumeName)
+TrackingVolume::TrackingVolume(Volume& volume, const std::string& volumeName)
     : TrackingVolume(volume.transform(), volume.volumeBoundsPtr(), nullptr,
                      nullptr, nullptr, MutableTrackingVolumeVector{},
                      volumeName) {}
 
 TrackingVolume::TrackingVolume(const Transform3& transform,
-                               std::shared_ptr<const VolumeBounds> volbounds,
+                               std::shared_ptr<VolumeBounds> volbounds,
                                const std::string& volumeName)
     : TrackingVolume(transform, std::move(volbounds), nullptr, nullptr, nullptr,
                      {}, volumeName) {}

Core/src/Geometry/Volume.cpp

@@ -19,7 +19,7 @@ using namespace Acts::UnitLiterals;
 namespace Acts {
 
 Volume::Volume(const Transform3& transform,
-               std::shared_ptr<const VolumeBounds> volbounds)
+               std::shared_ptr<VolumeBounds> volbounds)
     : GeometryObject(),
       m_transform(transform),
       m_itransform(m_transform.inverse()),
@@ -74,11 +74,11 @@ Volume::BoundingBox Volume::orientedBoundingBox() const {
                                      this);
 }
 
-void Volume::assignVolumeBounds(std::shared_ptr<const VolumeBounds> volbounds) {
+void Volume::assignVolumeBounds(std::shared_ptr<VolumeBounds> volbounds) {
   update(std::move(volbounds));
 }
 
-void Volume::update(std::shared_ptr<const VolumeBounds> volbounds,
+void Volume::update(std::shared_ptr<VolumeBounds> volbounds,
                     std::optional<Transform3> transform) {
   if (volbounds) {
     m_volumeBounds = std::move(volbounds);
@@ -104,10 +104,18 @@ const VolumeBounds& Volume::volumeBounds() const {
   return *m_volumeBounds;
 }
 
+VolumeBounds& Volume::volumeBounds() {
+  return *m_volumeBounds;
+}
+
 std::shared_ptr<const VolumeBounds> Volume::volumeBoundsPtr() const {
   return m_volumeBounds;
 }
 
+std::shared_ptr<VolumeBounds> Volume::volumeBoundsPtr() {
+  return m_volumeBounds;
+}
+
 void Volume::setTransform(const Transform3& transform) {
   m_transform = transform;
   m_itransform = m_transform.inverse();

Examples/Python/src/Base.cpp

@@ -297,9 +297,12 @@ void addAlgebra(Acts::Python::Context& ctx) {
             Acts::Vector3(translation[0], translation[1], translation[2]));
         return t;
       }))
-      .def("getTranslation", [](const Acts::Transform3& self) {
-        return Vector3(self.translation());
-      });
+      .def("getTranslation",
+           [](const Acts::Transform3& self) {
+             return Vector3(self.translation());
+           })
+      .def_static("Identity", &Acts::Transform3::Identity);
+  ;
 }
 
 void addBinning(Context& ctx) {

Examples/Python/src/Geometry.cpp

@@ -189,11 +189,8 @@ void addGeometry(Context& ctx) {
 
     py::class_<Acts::TrackingVolume, Acts::Volume,
                std::shared_ptr<Acts::TrackingVolume>>(m, "TrackingVolume")
-        .def(py::init([](std::shared_ptr<const Acts::VolumeBounds> bounds,
-                         std::string name) {
-          return std::make_shared<Acts::TrackingVolume>(Transform3::Identity(),
-                                                        bounds, name);
-        }));
+        .def(py::init<const Transform3&, std::shared_ptr<Acts::VolumeBounds>,
+                      std::string>());
   }
 
   {