refactor: Portal fusing and remove shared-from-this

Core/include/Acts/Detector/Portal.hpp

@@ -41,14 +41,13 @@ struct NavigationState;
 /// The surface can carry material to allow mapping onto
 /// portal positions if required.
 ///
-class Portal : public std::enable_shared_from_this<Portal> {
- protected:
+class Portal {
+ public:
   /// Constructor from surface w/o portal links
   ///
   /// @param surface is the representing surface
   Portal(std::shared_ptr<RegularSurface> surface);
 
- public:
   /// The volume links forward/backward with respect to the surface normal
   using DetectorVolumeUpdaters = std::array<DetectorVolumeUpdater, 2u>;
 
@@ -60,32 +59,6 @@ class Portal : public std::enable_shared_from_this<Portal> {
   /// Declare the DetectorVolume friend for portal setting
   friend class DetectorVolume;
 
-  /// Factory for producing memory managed instances of Portal.
-  static std::shared_ptr<Portal> makeShared(
-      std::shared_ptr<RegularSurface> surface);
-
-  /// Retrieve a @c std::shared_ptr for this surface (non-const version)
-  ///
-  /// @note Will error if this was not created through the @c makeShared factory
-  ///       since it needs access to the original reference. In C++14 this is
-  ///       undefined behavior (but most likely implemented as a @c bad_weak_ptr
-  ///       exception), in C++17 it is defined as that exception.
-  /// @note Only call this if you need shared ownership of this object.
-  ///
-  /// @return The shared pointer
-  std::shared_ptr<Portal> getSharedPtr();
-
-  /// Retrieve a @c std::shared_ptr for this surface (const version)
-  ///
-  /// @note Will error if this was not created through the @c makeShared factory
-  ///       since it needs access to the original reference. In C++14 this is
-  ///       undefined behavior, but most likely implemented as a @c bad_weak_ptr
-  ///       exception, in C++17 it is defined as that exception.
-  /// @note Only call this if you need shared ownership of this object.
-  ///
-  /// @return The shared pointer
-  std::shared_ptr<const Portal> getSharedPtr() const;
-
   Portal() = delete;
   virtual ~Portal() = default;
 
@@ -110,18 +83,22 @@ class Portal : public std::enable_shared_from_this<Portal> {
 
   /// Fuse with another portal, this one is kept
   ///
-  /// @param other is the portal that will be fused
+  /// @param keep is the portal that will be kept
+  /// @param discard is the portal that will be fused
   ///
-  /// @note this will move the portal links from the other
-  /// into this volume, it will throw an exception if the
+  /// @note this will combine the portal links from the both
+  /// portals into a new one, it will throw an exception if the
   /// portals are not fusable
   ///
   /// @note if one portal carries material, it will be kept,
   /// however, if both portals carry material, an exception
   /// will be thrown and the portals are not fusable
   ///
-  /// @note that other will be overwritten to point to this
-  void fuse(std::shared_ptr<Portal>& other) noexcept(false);
+  /// @note Both input portals become invalid, in that their update
+  /// delegates and attached volumes are reset
+  static std::shared_ptr<Portal> fuse(
+      std::shared_ptr<Portal>& keep,
+      std::shared_ptr<Portal>& discard) noexcept(false);
 
   /// Update the volume link
   ///

Core/src/Detector/Portal.cpp

@@ -18,101 +18,111 @@
 #include <stdexcept>
 #include <utility>
 
-namespace Acts {
-namespace Experimental {
-class DetectorVolume;
-}  // namespace Experimental
-}  // namespace Acts
+namespace Acts::Experimental {
 
-Acts::Experimental::Portal::Portal(std::shared_ptr<RegularSurface> surface)
+Portal::Portal(std::shared_ptr<RegularSurface> surface)
     : m_surface(std::move(surface)) {
   throw_assert(m_surface, "Portal surface is nullptr");
 }
 
-std::shared_ptr<Acts::Experimental::Portal>
-Acts::Experimental::Portal::makeShared(
-    std::shared_ptr<RegularSurface> surface) {
-  return std::shared_ptr<Portal>(new Portal(std::move(surface)));
-}
-
-const Acts::RegularSurface& Acts::Experimental::Portal::surface() const {
+const Acts::RegularSurface& Portal::surface() const {
   return *m_surface.get();
 }
 
-Acts::RegularSurface& Acts::Experimental::Portal::surface() {
+Acts::RegularSurface& Portal::surface() {
   return *m_surface.get();
 }
 
-const Acts::Experimental::Portal::DetectorVolumeUpdaters&
-Acts::Experimental::Portal::detectorVolumeUpdaters() const {
+const Portal::DetectorVolumeUpdaters& Portal::detectorVolumeUpdaters() const {
   return m_volumeUpdaters;
 }
 
-Acts::Experimental::Portal::AttachedDetectorVolumes&
-Acts::Experimental::Portal::attachedDetectorVolumes() {
+Portal::AttachedDetectorVolumes& Portal::attachedDetectorVolumes() {
   return m_attachedVolumes;
 }
 
-std::shared_ptr<Acts::Experimental::Portal>
-Acts::Experimental::Portal::getSharedPtr() {
-  return shared_from_this();
+void Portal::assignGeometryId(const GeometryIdentifier& geometryId) {
+  m_surface->assignGeometryId(geometryId);
 }
 
-std::shared_ptr<const Acts::Experimental::Portal>
-Acts::Experimental::Portal::getSharedPtr() const {
-  return shared_from_this();
-}
+std::shared_ptr<Portal> Portal::fuse(std::shared_ptr<Portal>& aPortal,
+                                     std::shared_ptr<Portal>& bPortal) {
+  if (aPortal == bPortal) {
+    return aPortal;
+  }
 
-void Acts::Experimental::Portal::assignGeometryId(
-    const GeometryIdentifier& geometryId) {
-  m_surface->assignGeometryId(geometryId);
-}
+  auto bothConnected = [](const auto& p) {
+    return p.m_volumeUpdaters[0u].connected() &&
+           p.m_volumeUpdaters[1u].connected();
+  };
 
-void Acts::Experimental::Portal::fuse(std::shared_ptr<Portal>& other) {
-  Direction bDir = Direction::Backward;
+  auto noneConnected = [](const auto& p) {
+    return !p.m_volumeUpdaters[0u].connected() &&
+           !p.m_volumeUpdaters[1u].connected();
+  };
 
-  // Determine this directioon
-  Direction tDir = (!m_volumeUpdaters[bDir.index()].connected())
-                       ? Direction::Forward
-                       : Direction::Backward;
+  if (bothConnected(*aPortal) || bothConnected(*bPortal)) {
+    throw std::invalid_argument(
+        "Portal: trying to fuse two portals where at least one has links on "
+        "both sides.");
+  }
 
-  if (!m_volumeUpdaters[tDir.index()].connected()) {
+  if (noneConnected(*aPortal) || noneConnected(*bPortal)) {
     throw std::invalid_argument(
-        "Portal: trying to fuse portal (keep) with no links.");
+        "Portal: trying to fuse two portals where at least on has no links.");
   }
+
+  // We checked they're not both empty, so one of them must be connected
+  Direction aDir = (aPortal->m_volumeUpdaters[0].connected())
+                       ? Direction::fromIndex(0)
+                       : Direction::fromIndex(1);
+  Direction bDir = aDir.invert();
+
   // And now check other direction
-  Direction oDir = tDir.invert();
-  if (!other->m_volumeUpdaters[oDir.index()].connected()) {
+  if (!bPortal->m_volumeUpdaters[bDir.index()].connected()) {
     throw std::runtime_error(
-        "Portal: trying to fuse portal (waste) with no links.");
+        "Portal: trying to fuse portal (discard) with no links.");
   }
 
-  if (m_surface->surfaceMaterial() != nullptr &&
-      other->surface().surfaceMaterial() != nullptr) {
+  const auto& aSurface = aPortal->surface();
+  const auto& bSurface = bPortal->surface();
+
+  // @TODO: There's no safety against fusing portals with different surfaces
+  std::shared_ptr<Portal> fused = std::make_shared<Portal>(aPortal->m_surface);
+
+  if (aSurface.surfaceMaterial() != nullptr &&
+      bSurface.surfaceMaterial() != nullptr) {
     throw std::runtime_error(
         "Portal: both surfaces have surface material, fusing will lead to "
         "information loss.");
-  } else if (other->surface().surfaceMaterial() != nullptr) {
-    m_surface->assignSurfaceMaterial(
-        other->surface().surfaceMaterialSharedPtr());
+  } else if (aSurface.surfaceMaterial() != nullptr) {
+    fused->m_surface = aPortal->m_surface;
+  } else if (bSurface.surfaceMaterial() != nullptr) {
+    fused->m_surface = bPortal->m_surface;
   }
 
-  auto odx = oDir.index();
-  m_volumeUpdaters[odx] = std::move(other->m_volumeUpdaters[odx]);
-  m_attachedVolumes[odx] = other->m_attachedVolumes[odx];
-  // And finally overwrite the original portal
-  other = getSharedPtr();
+  fused->m_volumeUpdaters[aDir.index()] =
+      std::move(aPortal->m_volumeUpdaters[aDir.index()]);
+  fused->m_attachedVolumes[aDir.index()] =
+      std::move(aPortal->m_attachedVolumes[aDir.index()]);
+
+  fused->m_volumeUpdaters[bDir.index()] =
+      std::move(bPortal->m_volumeUpdaters[bDir.index()]);
+  fused->m_attachedVolumes[bDir.index()] =
+      std::move(bPortal->m_attachedVolumes[bDir.index()]);
+
+  return fused;
 }
 
-void Acts::Experimental::Portal::assignDetectorVolumeUpdater(
+void Portal::assignDetectorVolumeUpdater(
     Direction dir, DetectorVolumeUpdater dVolumeUpdater,
     std::vector<std::shared_ptr<DetectorVolume>> attachedVolumes) {
   auto idx = dir.index();
   m_volumeUpdaters[idx] = std::move(dVolumeUpdater);
   m_attachedVolumes[idx] = std::move(attachedVolumes);
 }
 
-void Acts::Experimental::Portal::assignDetectorVolumeUpdater(
+void Portal::assignDetectorVolumeUpdater(
     DetectorVolumeUpdater dVolumeUpdater,
     std::vector<std::shared_ptr<DetectorVolume>> attachedVolumes) {
   // Check and throw exceptions
@@ -127,8 +137,8 @@ void Acts::Experimental::Portal::assignDetectorVolumeUpdater(
   m_attachedVolumes[idx] = std::move(attachedVolumes);
 }
 
-void Acts::Experimental::Portal::updateDetectorVolume(
-    const GeometryContext& gctx, NavigationState& nState) const {
+void Portal::updateDetectorVolume(const GeometryContext& gctx,
+                                  NavigationState& nState) const {
   const auto& position = nState.position;
   const auto& direction = nState.direction;
   const Vector3 normal = surface().normal(gctx, position);
@@ -140,3 +150,5 @@ void Acts::Experimental::Portal::updateDetectorVolume(
     nState.currentVolume = nullptr;
   }
 }
+
+}  // namespace Acts::Experimental

Core/src/Detector/PortalGenerators.cpp

@@ -33,7 +33,7 @@ Acts::Experimental::generatePortals(
   std::vector<std::shared_ptr<Portal>> portals;
   for (auto [i, oSurface] : enumerate(orientedSurfaces)) {
     // Create a portal from the surface
-    auto portal = Portal::makeShared(oSurface.first);
+    auto portal = std::make_shared<Portal>(oSurface.first);
     // Create a shared link instance & delegate
     auto singleLinkImpl =
         std::make_unique<const SingleDetectorVolumeImpl>(dVolume.get());

Core/src/Detector/detail/CylindricalDetectorHelper.cpp

@@ -99,7 +99,7 @@ Acts::Experimental::PortalReplacement createDiscReplacement(
   const auto& stitchBoundaries =
       (stitchValue == Acts::binR) ? rBoundaries : phiBoundaries;
   return Acts::Experimental::PortalReplacement(
-      Acts::Experimental::Portal::makeShared(surface), index, dir,
+      std::make_shared<Acts::Experimental::Portal>(surface), index, dir,
       stitchBoundaries, stitchValue);
 }
 
@@ -135,7 +135,7 @@ Acts::Experimental::PortalReplacement createCylinderReplacement(
   const auto& stitchBoundaries =
       (stitchValue == Acts::binZ) ? zBoundaries : phiBoundaries;
   return Acts::Experimental::PortalReplacement(
-      Acts::Experimental::Portal::makeShared(surface), index, dir,
+      std::make_shared<Acts::Experimental::Portal>(surface), index, dir,
       stitchBoundaries, stitchValue);
 }
 
@@ -200,8 +200,8 @@ Acts::Experimental::PortalReplacement createSectorReplacement(
       transform, std::move(bounds));
   // A make a portal and indicate the new link direction
   Acts::Experimental::PortalReplacement pRep = {
-      Acts::Experimental::Portal::makeShared(surface), index, dir, boundaries,
-      binning};
+      std::make_shared<Acts::Experimental::Portal>(surface), index, dir,
+      boundaries, binning};
   return pRep;
 }
 
@@ -430,7 +430,7 @@ Acts::Experimental::detail::CylindricalDetectorHelper::connectInR(
       // the inner cylinder of the outer portal goes to waste
       auto& keepCylinder = volumes[iv - 1]->portalPtrs()[2u];
       auto& wasteCylinder = volumes[iv]->portalPtrs()[3u];
-      keepCylinder->fuse(wasteCylinder);
+      keepCylinder = Portal::fuse(keepCylinder, wasteCylinder);
       volumes[iv]->updatePortal(keepCylinder, 3u);
     }
   }
@@ -605,7 +605,7 @@ Acts::Experimental::detail::CylindricalDetectorHelper::connectInZ(
         message += " / " + Acts::toString(wastePosition);
         throw std::runtime_error(message.c_str());
       }
-      keepDisc->fuse(wasteDisc);
+      keepDisc = Portal::fuse(keepDisc, wasteDisc);
       volumes[iv]->updatePortal(keepDisc, 0u);
     }
   }
@@ -766,7 +766,7 @@ Acts::Experimental::detail::CylindricalDetectorHelper::connectInPhi(
     // Fuse and swap
     auto& keepSector = volumes[iv - 1]->portalPtrs()[iSecOffset + 1u];
     auto& wasteSector = volumes[iv]->portalPtrs()[iSecOffset];
-    keepSector->fuse(wasteSector);
+    keepSector = Portal::fuse(keepSector, wasteSector);
     volumes[iv]->updatePortal(keepSector, iSecOffset);
     // The current values
     auto curValues = volumes[iv]->volumeBounds().values();
@@ -877,19 +877,19 @@ Acts::Experimental::detail::CylindricalDetectorHelper::wrapInZR(
   // Fuse outer cover of first with inner cylinder of wrapping volume
   auto& keepCover = volumes[0u]->portalPtrs()[2u];
   auto& wasteCover = volumes[1u]->portalPtrs()[3u];
-  keepCover->fuse(wasteCover);
+  keepCover = Portal::fuse(keepCover, wasteCover);
   volumes[1u]->updatePortal(keepCover, 3u);
 
   // Stitch sides - negative
   auto& keepDiscN = volumes[1u]->portalPtrs()[4u];
   auto& wasteDiscN = volumes[0u]->portalPtrs()[0u];
-  keepDiscN->fuse(wasteDiscN);
+  keepDiscN = Portal::fuse(keepDiscN, wasteDiscN);
   volumes[0u]->updatePortal(keepDiscN, 0u);
 
   // Stich sides - positive
   auto& keepDiscP = volumes[0u]->portalPtrs()[1u];
   auto& wasteDiscP = volumes[1u]->portalPtrs()[5u];
-  keepDiscP->fuse(wasteDiscP);
+  keepDiscP = Portal::fuse(keepDiscP, wasteDiscP);
   volumes[1u]->updatePortal(keepDiscP, 5u);
 
   // If needed, insert new cylinder
@@ -963,7 +963,7 @@ Acts::Experimental::detail::CylindricalDetectorHelper::connectInR(
     // Fuse and swap
     std::shared_ptr<Portal> keepCylinder = containers[ic - 1].find(2u)->second;
     std::shared_ptr<Portal> wasteCylinder = containers[ic].find(3u)->second;
-    keepCylinder->fuse(wasteCylinder);
+    keepCylinder = Portal::fuse(keepCylinder, wasteCylinder);
     for (auto& av : wasteCylinder->attachedDetectorVolumes()[1u]) {
       av->updatePortal(keepCylinder, 3u);
     }
@@ -1019,7 +1019,7 @@ Acts::Experimental::detail::CylindricalDetectorHelper::connectInZ(
     }
     std::shared_ptr<Portal> keepDisc = formerContainer.find(1u)->second;
     std::shared_ptr<Portal> wasteDisc = currentContainer.find(0u)->second;
-    keepDisc->fuse(wasteDisc);
+    keepDisc = Portal::fuse(keepDisc, wasteDisc);
     for (auto& av : wasteDisc->attachedDetectorVolumes()[1u]) {
       ACTS_VERBOSE("Update portal of detector volume '" << av->name() << "'.");
       av->updatePortal(keepDisc, 0u);
@@ -1121,19 +1121,19 @@ Acts::Experimental::detail::CylindricalDetectorHelper::wrapInZR(
   // Fuse outer cover of first with inner cylinder of wrapping volume
   auto& keepCover = innerContainer[2u];
   auto& wasteCover = wrappingVolume->portalPtrs()[3u];
-  keepCover->fuse(wasteCover);
+  keepCover = Portal::fuse(keepCover, wasteCover);
   wrappingVolume->updatePortal(keepCover, 3u);
 
   // Stitch sides - negative
   auto& keepDiscN = innerContainer[0u];
   auto& wasteDiscN = wrappingVolume->portalPtrs()[4u];
-  keepDiscN->fuse(wasteDiscN);
+  keepDiscN = Portal::fuse(keepDiscN, wasteDiscN);
   wrappingVolume->updatePortal(keepDiscN, 4u);
 
   // Stich sides - positive
   auto& keepDiscP = innerContainer[1u];
   auto& wasteDiscP = wrappingVolume->portalPtrs()[5u];
-  keepDiscP->fuse(wasteDiscP);
+  keepDiscP = Portal::fuse(keepDiscP, wasteDiscP);
   wrappingVolume->updatePortal(keepDiscP, 5u);
 
   // If inner stitching is necessary

Plugins/Json/src/PortalJsonConverter.cpp

@@ -305,7 +305,7 @@ std::shared_ptr<Acts::Experimental::Portal> Acts::PortalJsonConverter::fromJson(
     throw std::runtime_error(
         "PortalJsonConverter: surface is not a regular surface.");
   }
-  auto portal = Experimental::Portal::makeShared(regSurface);
+  auto portal = std::make_shared<Experimental::Portal>(regSurface);
 
   std::array<Acts::Direction, 2> normalDirs = {Direction::Backward,
                                                Direction::Forward};