From 430085bd063cbe046c62deba8400eb6aee86d494 Mon Sep 17 00:00:00 2001 From: Paul Gessinger Date: Fri, 28 Jun 2024 15:52:37 +0200 Subject: [PATCH] refactor(geometry): Surface merging uses custom exception (#3334) This switches the surface merging introduced in #3288 over to a dedicated exception `SurfaceMergingException`. --- .../Acts/Surfaces/SurfaceMergingException.hpp | 35 +++++++++++++++ Core/src/Surfaces/CylinderSurface.cpp | 45 ++++++++++++------- Core/src/Surfaces/DiscSurface.cpp | 43 ++++++++++++------ .../Core/Surfaces/CylinderSurfaceTests.cpp | 25 ++++++----- .../Core/Surfaces/DiscSurfaceTests.cpp | 25 ++++++----- 5 files changed, 120 insertions(+), 53 deletions(-) create mode 100644 Core/include/Acts/Surfaces/SurfaceMergingException.hpp diff --git a/Core/include/Acts/Surfaces/SurfaceMergingException.hpp b/Core/include/Acts/Surfaces/SurfaceMergingException.hpp new file mode 100644 index 00000000000..ed5511302ad --- /dev/null +++ b/Core/include/Acts/Surfaces/SurfaceMergingException.hpp @@ -0,0 +1,35 @@ +// This file is part of the Acts project. +// +// Copyright (C) 2024 CERN for the benefit of the Acts project +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#pragma once + +#include +#include +#include + +namespace Acts { +class Surface; + +/// @brief Exception type failures to merge two surfaces +class SurfaceMergingException : public std::exception { + public: + SurfaceMergingException(std::weak_ptr surfaceA, + std::weak_ptr surfaceB, + const std::string& reason) + : m_surfaceA(std::move(surfaceA)), + m_surfaceB(std::move(surfaceB)), + m_message(std::string{"Failure to merge surfaces: "} + reason) {} + + const char* what() const throw() override { return m_message.c_str(); } + + private: + std::weak_ptr m_surfaceA; + std::weak_ptr m_surfaceB; + std::string m_message; +}; +} // namespace Acts diff --git a/Core/src/Surfaces/CylinderSurface.cpp b/Core/src/Surfaces/CylinderSurface.cpp index ac88ee667d8..8d5bad141a7 100644 --- a/Core/src/Surfaces/CylinderSurface.cpp +++ b/Core/src/Surfaces/CylinderSurface.cpp @@ -14,6 +14,7 @@ #include "Acts/Geometry/GeometryObject.hpp" #include "Acts/Surfaces/CylinderBounds.hpp" #include "Acts/Surfaces/SurfaceError.hpp" +#include "Acts/Surfaces/SurfaceMergingException.hpp" #include "Acts/Surfaces/detail/AlignmentHelper.hpp" #include "Acts/Surfaces/detail/FacesHelper.hpp" #include "Acts/Surfaces/detail/MergeHelper.hpp" @@ -28,6 +29,7 @@ #include #include #include +#include #include #include @@ -378,16 +380,18 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( // surface cannot have any relative rotation if (!otherLocal.linear().isApprox(RotationMatrix3::Identity())) { ACTS_ERROR("CylinderSurface::merge: surfaces have relative rotation"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderSurface::merge: surfaces have relative rotation"); } - auto checkNoBevel = [&logger](const auto& bounds) { + auto checkNoBevel = [this, &logger, &other](const auto& bounds) { if (bounds.get(CylinderBounds::eBevelMinZ) != 0.0) { ACTS_ERROR( "CylinderVolumeStack requires all volumes to have a bevel angle of " "0"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderVolumeStack requires all volumes to have a bevel angle of " "0"); } @@ -396,7 +400,8 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( ACTS_ERROR( "CylinderVolumeStack requires all volumes to have a bevel angle of " "0"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderVolumeStack requires all volumes to have a bevel angle of " "0"); } @@ -409,7 +414,8 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( if (std::abs(bounds().get(CylinderBounds::eR) - other.bounds().get(CylinderBounds::eR)) > tolerance) { ACTS_ERROR("CylinderSurface::merge: surfaces have different radii"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderSurface::merge: surfaces have different radii"); } @@ -422,7 +428,8 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( std::abs(translation[1]) > tolerance) { ACTS_ERROR( "CylinderSurface::merge: surfaces have relative translation in x/y"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderSurface::merge: surfaces have relative translation in x/y"); } @@ -449,7 +456,8 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( if (std::abs(maxZ - otherMinZ) > tolerance && std::abs(minZ - otherMaxZ) > tolerance) { ACTS_ERROR("CylinderSurface::merge: surfaces have incompatible z bounds"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderSurface::merge: surfaces have incompatible z bounds"); } @@ -473,7 +481,8 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( ACTS_ERROR( "CylinderSurface::merge: surfaces have relative translation in z for " "rPhi merging"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "CylinderSurface::merge: surfaces have relative translation in z for " "rPhi merging"); } @@ -484,15 +493,21 @@ std::shared_ptr Acts::CylinderSurface::mergedWith( ActsScalar otherHlPhi = other.bounds().get(CylinderBounds::eHalfPhiSector); ActsScalar otherAvgPhi = other.bounds().get(CylinderBounds::eAveragePhi); - auto [newHlPhi, newAvgPhi] = detail::mergedPhiSector( - hlPhi, avgPhi, otherHlPhi, otherAvgPhi, logger, tolerance); + try { + auto [newHlPhi, newAvgPhi] = detail::mergedPhiSector( + hlPhi, avgPhi, otherHlPhi, otherAvgPhi, logger, tolerance); - auto newBounds = std::make_shared( - r, bounds().get(CylinderBounds::eHalfLengthZ), newHlPhi, newAvgPhi); + auto newBounds = std::make_shared( + r, bounds().get(CylinderBounds::eHalfLengthZ), newHlPhi, newAvgPhi); - return Surface::makeShared(transform(gctx), newBounds); + return Surface::makeShared(transform(gctx), newBounds); + } catch (const std::invalid_argument& e) { + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + e.what()); + } } else { - throw std::invalid_argument("CylinderSurface::merge: invalid direction " + - binningValueNames()[direction]); + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + "CylinderSurface::merge: invalid direction " + + binningValueNames()[direction]); } } diff --git a/Core/src/Surfaces/DiscSurface.cpp b/Core/src/Surfaces/DiscSurface.cpp index 1ad326da70d..1c5b12cb1de 100644 --- a/Core/src/Surfaces/DiscSurface.cpp +++ b/Core/src/Surfaces/DiscSurface.cpp @@ -18,6 +18,7 @@ #include "Acts/Surfaces/RadialBounds.hpp" #include "Acts/Surfaces/SurfaceBounds.hpp" #include "Acts/Surfaces/SurfaceError.hpp" +#include "Acts/Surfaces/SurfaceMergingException.hpp" #include "Acts/Surfaces/detail/FacesHelper.hpp" #include "Acts/Surfaces/detail/MergeHelper.hpp" #include "Acts/Surfaces/detail/PlanarHelper.hpp" @@ -392,7 +393,8 @@ std::shared_ptr Acts::DiscSurface::mergedWith( // surface cannot have any relative rotation if (!otherLocal.linear().isApprox(RotationMatrix3::Identity())) { ACTS_ERROR("DiscSurface::merge: surfaces have relative rotation"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces have relative rotation"); } @@ -403,7 +405,8 @@ std::shared_ptr Acts::DiscSurface::mergedWith( std::abs(translation[2]) > tolerance) { ACTS_ERROR( "DiscSurface::merge: surfaces have relative translation in x/y/z"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces have relative translation in x/y/z"); } @@ -413,7 +416,8 @@ std::shared_ptr Acts::DiscSurface::mergedWith( if (bounds == nullptr || otherBounds == nullptr) { ACTS_ERROR("DiscSurface::merge: surfaces have bounds other than radial"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces have bounds other than radial"); } @@ -447,19 +451,22 @@ std::shared_ptr Acts::DiscSurface::mergedWith( if (std::abs(minR - otherMaxR) > tolerance && std::abs(maxR - otherMinR) > tolerance) { ACTS_ERROR("DiscSurface::merge: surfaces are not touching r"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces are not touching in r"); } if (std::abs(avgPhi - otherAvgPhi) > tolerance) { ACTS_ERROR("DiscSurface::merge: surfaces have different average phi"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces have different average phi"); } if (std::abs(hlPhi - otherHlPhi) > tolerance) { ACTS_ERROR("DiscSurface::merge: surfaces have different half phi sector"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces have different half phi sector"); } @@ -476,22 +483,30 @@ std::shared_ptr Acts::DiscSurface::mergedWith( if (std::abs(maxR - otherMaxR) > tolerance || std::abs(minR - otherMinR) > tolerance) { ACTS_ERROR("DiscSurface::merge: surfaces don't have same r bounds"); - throw std::invalid_argument( + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), "DiscSurface::merge: surfaces don't have same r bounds"); } - auto [newHlPhi, newAvgPhi] = detail::mergedPhiSector( - hlPhi, avgPhi, otherHlPhi, otherAvgPhi, logger, tolerance); + try { + auto [newHlPhi, newAvgPhi] = detail::mergedPhiSector( + hlPhi, avgPhi, otherHlPhi, otherAvgPhi, logger, tolerance); - auto newBounds = - std::make_shared(minR, maxR, newHlPhi, newAvgPhi); + auto newBounds = + std::make_shared(minR, maxR, newHlPhi, newAvgPhi); - return Surface::makeShared(transform(gctx), newBounds); + return Surface::makeShared(transform(gctx), newBounds); + } catch (const std::invalid_argument& e) { + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + e.what()); + } } else { ACTS_ERROR("DiscSurface::merge: invalid direction " << binningValueNames()[direction]); - throw std::invalid_argument("DiscSurface::merge: invalid direction " + - binningValueNames()[direction]); + + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + "DiscSurface::merge: invalid direction " + + binningValueNames()[direction]); } } diff --git a/Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp b/Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp index 44e5c61a13b..67dd7d9d5a2 100644 --- a/Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp +++ b/Tests/UnitTests/Core/Surfaces/CylinderSurfaceTests.cpp @@ -20,6 +20,7 @@ #include "Acts/Surfaces/CylinderSurface.hpp" #include "Acts/Surfaces/Surface.hpp" #include "Acts/Surfaces/SurfaceBounds.hpp" +#include "Acts/Surfaces/SurfaceMergingException.hpp" #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/BinningType.hpp" #include "Acts/Utilities/Helpers.hpp" @@ -352,13 +353,13 @@ BOOST_DATA_TEST_CASE(IncompatibleZDirection, base * Translation3{Vector3::UnitZ() * 200_mm}, 30_mm, 100_mm); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl2, Acts::binPhi, *logger), - std::invalid_argument); + SurfaceMergingException); auto cylShiftedXy = Surface::makeShared( base * Translation3{Vector3{1_mm, 2_mm, 200_mm}}, 30_mm, 100_mm); BOOST_CHECK_THROW( cyl->mergedWith(testContext, *cylShiftedXy, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); auto cylRotatedZ = Surface::makeShared( base * AngleAxis3{10_degree, Vector3::UnitZ()} * @@ -366,7 +367,7 @@ BOOST_DATA_TEST_CASE(IncompatibleZDirection, 30_mm, 100_mm); BOOST_CHECK_THROW( cyl->mergedWith(testContext, *cylRotatedZ, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); auto cylRotatedX = Surface::makeShared( base * AngleAxis3{10_degree, Vector3::UnitX()} * @@ -374,38 +375,38 @@ BOOST_DATA_TEST_CASE(IncompatibleZDirection, 30_mm, 100_mm); BOOST_CHECK_THROW( cyl->mergedWith(testContext, *cylRotatedX, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with different radius auto cyl3 = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 200_mm}, 35_mm, 100_mm); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl3, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with bevel auto cyl4 = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 200_mm}, 30_mm, 100_mm, M_PI, 0, M_PI / 8.0); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl4, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); auto cyl5 = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 200_mm}, 30_mm, 100_mm, M_PI, 0, 0, M_PI / 8.0); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl5, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with overlap in z auto cyl6 = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 150_mm}, 30_mm, 100_mm); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl6, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with gap in z auto cyl7 = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 250_mm}, 30_mm, 100_mm); BOOST_CHECK_THROW(cyl->mergedWith(testContext, *cyl7, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); } BOOST_DATA_TEST_CASE(ZDirection, @@ -465,14 +466,14 @@ BOOST_DATA_TEST_CASE(IncompatibleRPhiDirection, 45_degree, a(85_degree)); BOOST_CHECK_THROW( cylPhi->mergedWith(testContext, *cylPhi2, Acts::binRPhi, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with gap in phi auto cylPhi3 = Surface::makeShared(base, 30_mm, 100_mm, 45_degree, a(105_degree)); BOOST_CHECK_THROW( cylPhi->mergedWith(testContext, *cylPhi3, Acts::binRPhi, *logger), - std::invalid_argument); + SurfaceMergingException); // Cylinder with a z shift auto cylPhi4 = Surface::makeShared( @@ -480,7 +481,7 @@ BOOST_DATA_TEST_CASE(IncompatibleRPhiDirection, a(95_degree)); BOOST_CHECK_THROW( cylPhi->mergedWith(testContext, *cylPhi4, Acts::binRPhi, *logger), - std::invalid_argument); + SurfaceMergingException); } BOOST_DATA_TEST_CASE(RPhiDirection, diff --git a/Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp b/Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp index ea5e8285e99..4db74e0af17 100644 --- a/Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp +++ b/Tests/UnitTests/Core/Surfaces/DiscSurfaceTests.cpp @@ -23,6 +23,7 @@ #include "Acts/Surfaces/RadialBounds.hpp" #include "Acts/Surfaces/Surface.hpp" #include "Acts/Surfaces/SurfaceBounds.hpp" +#include "Acts/Surfaces/SurfaceMergingException.hpp" #include "Acts/Tests/CommonHelpers/DetectorElementStub.hpp" #include "Acts/Tests/CommonHelpers/FloatComparisons.hpp" #include "Acts/Utilities/BinningType.hpp" @@ -398,10 +399,10 @@ BOOST_AUTO_TEST_CASE(IncompatibleBounds) { Surface::makeShared(base, 20_mm, 40_mm, 30_mm, 100_mm); BOOST_CHECK_THROW(discRadial->mergedWith(tgContext, *discTrap, binR, *logger), - std::invalid_argument); + SurfaceMergingException); BOOST_CHECK_THROW(discTrap2->mergedWith(tgContext, *discTrap, binR, *logger), - std::invalid_argument); + SurfaceMergingException); } BOOST_DATA_TEST_CASE(IncompatibleRDirection, @@ -423,35 +424,35 @@ BOOST_DATA_TEST_CASE(IncompatibleRDirection, auto discOverlap = makeDisc(base, 90_mm, 150_mm); BOOST_CHECK_THROW( disc->mergedWith(tgContext, *discOverlap, Acts::binR, *logger), - std::invalid_argument); + SurfaceMergingException); // Disc with gap in r auto discGap = makeDisc(base, 110_mm, 150_mm); BOOST_CHECK_THROW(disc->mergedWith(tgContext, *discGap, Acts::binR, *logger), - std::invalid_argument); + SurfaceMergingException); auto discShiftedZ = Surface::makeShared( base * Translation3{Vector3::UnitZ() * 10_mm}, 100_mm, 150_mm); BOOST_CHECK_THROW( disc->mergedWith(tgContext, *discShiftedZ, Acts::binR, *logger), - std::invalid_argument); + SurfaceMergingException); auto discShiftedXy = makeDisc( base * Translation3{Vector3{1_mm, 2_mm, 200_mm}}, 100_mm, 150_mm); BOOST_CHECK_THROW( disc->mergedWith(tgContext, *discShiftedXy, Acts::binZ, *logger), - std::invalid_argument); + SurfaceMergingException); auto discRotatedZ = makeDisc(base * AngleAxis3{10_degree, Vector3::UnitZ()}, 100_mm, 150_mm); BOOST_CHECK_THROW( disc->mergedWith(tgContext, *discRotatedZ, Acts::binR, *logger), - std::invalid_argument); + SurfaceMergingException); auto discRotatedX = makeDisc(base * AngleAxis3{10_degree, Vector3::UnitX()}, 100_mm, 150_mm); BOOST_CHECK_THROW( disc->mergedWith(tgContext, *discRotatedX, Acts::binR, *logger), - std::invalid_argument); + SurfaceMergingException); } BOOST_DATA_TEST_CASE(RDirection, @@ -509,26 +510,26 @@ BOOST_DATA_TEST_CASE(IncompatiblePhiDirection, auto discPhi2 = makeDisc(base, 30_mm, 100_mm, 45_degree, a(85_degree)); BOOST_CHECK_THROW( discPhi->mergedWith(tgContext, *discPhi2, Acts::binPhi, *logger), - std::invalid_argument); + SurfaceMergingException); // Disc with gap in phi auto discPhi3 = makeDisc(base, 30_mm, 100_mm, 45_degree, a(105_degree)); BOOST_CHECK_THROW( discPhi->mergedWith(tgContext, *discPhi3, Acts::binPhi, *logger), - std::invalid_argument); + SurfaceMergingException); // Disc with a z shift auto discPhi4 = makeDisc(base * Translation3{Vector3::UnitZ() * 20_mm}, 30_mm, 100_mm, 45_degree, a(95_degree)); BOOST_CHECK_THROW( discPhi->mergedWith(tgContext, *discPhi4, Acts::binPhi, *logger), - std::invalid_argument); + SurfaceMergingException); // Disc with different r bounds: could be merged in r but not in phi auto discPhi5 = makeDisc(base, 100_mm, 150_mm, 45_degree, a(95_degree)); BOOST_CHECK_THROW( discPhi->mergedWith(tgContext, *discPhi5, Acts::binPhi, *logger), - std::invalid_argument); + SurfaceMergingException); } BOOST_DATA_TEST_CASE(PhiDirection,