Skip to content

Commit

Permalink
refactor: Portal fusing and remove shared-from-this (acts-project#2746)
Browse files Browse the repository at this point in the history 
The shared-from-this functionality in `Portal` is only needed because `Portal::fuse` uses it to reset the other fuse partner to itself. This PR changes `Portal::fuse` into a static symmetric function which returns a new merged portal.

The input portals are invalidated.

It then removes the shared-from-this functionality from portal, as it is no longer needed.
  • Loading branch information
@paulgessinger
paulgessinger authored Nov 29, 2023
1 parent cf84e21 commit c676ac1
Show file tree
Hide file tree
Showing 10 changed files with 142 additions and 150 deletions.
46 changes: 11 additions & 35 deletions Core/include/Acts/Detector/Portal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand All @@ -60,34 +59,7 @@ 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;

/// Const access to the surface representation
const RegularSurface& surface() const;
Expand All @@ -110,18 +82,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 aPortal is the first portal to fuse
/// @param bPortal is the second portal to fuse
///
/// @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>& aPortal,
std::shared_ptr<Portal>& bPortal) noexcept(false);

/// Update the volume link
///
Expand Down
120 changes: 66 additions & 54 deletions Core/src/Detector/Portal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -140,3 +150,5 @@ void Acts::Experimental::Portal::updateDetectorVolume(
nState.currentVolume = nullptr;
}
}

} // namespace Acts::Experimental
2 changes: 1 addition & 1 deletion Core/src/Detector/PortalGenerators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
30 changes: 15 additions & 15 deletions Core/src/Detector/detail/CylindricalDetectorHelper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Plugins/Json/src/PortalJsonConverter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,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};
Expand Down
Loading

0 comments on commit c676ac1

Please sign in to comment.