feat: Plane surface merging #4037
Conversation
WalkthroughA new method Changes
Sequence DiagramsequenceDiagram
participant S1 as Surface 1
participant S2 as Surface 2
participant Merger as mergedWith()
participant Result as Merged Surface
S1->>Merger: Provide surface parameters
S2->>Merger: Provide surface parameters
Merger->>Merger: Validate compatibility
Merger-->>Result: Create new surface
Result-->>S1: Return merged surface
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Surfaces/PlaneSurface.hpp(1 hunks)Core/src/Surfaces/PlaneSurface.cpp(2 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Analysis
Core/include/Acts/Surfaces/PlaneSurface.hpp
[error] 230-230: 'BinningValue' has not been declared in PlaneSurface class definition
🪛 GitHub Actions: Builds
Core/include/Acts/Surfaces/PlaneSurface.hpp
[error] 230-230: 'BinningValue' has not been declared in PlaneSurface.hpp
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
[error] 383-383: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (2)
Core/src/Surfaces/PlaneSurface.cpp (1)
235-239:⚠️ Potential issue'BinningValue' declared properly, it must be.
Compilation errors occur because 'BinningValue' undeclared it is. Include the correct header or fully qualify the type, you should.
Run this script to check for missing includes or namespace issues:
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
383-383: Configure Boost.Test properly, you should.Static analysis tool hints at unknown macro. Ensure test module defined and Boost.Test configured correctly, you must.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 383-383: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/src/Surfaces/PlaneSurface.cpp (2)
224-233: Safer bounds validation, implement you should.Check for nullptr before dynamic_cast, wise it would be. Prevent undefined behavior, this will.
-const RectangleBounds* thisBounds = - dynamic_cast<const RectangleBounds*>(&bounds()); -const RectangleBounds* otherBounds = - dynamic_cast<const RectangleBounds*>(&other.bounds()); +const auto* thisBounds = &bounds(); +const auto* otherBounds = &other.bounds(); + +if (thisBounds == nullptr || otherBounds == nullptr) { + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), + "PlaneSurface::merge: bounds must not be null"); +} + +const RectangleBounds* thisRectBounds = dynamic_cast<const RectangleBounds*>(thisBounds); +const RectangleBounds* otherRectBounds = dynamic_cast<const RectangleBounds*>(otherBounds); + +if (thisRectBounds == nullptr || otherRectBounds == nullptr) {
241-308: Improve readability of merging logic, we must.Complex calculations, these are. Better variable names and comments, help future Jedi they will.
Suggestions for improvement:
- Rename variables to be more descriptive:
-double thisHalfMerge +double thisHalfLengthInMergeDir -double otherHalfMerge +double otherHalfLengthInMergeDir -double newMidMerge +double newCenterPositionInMergeDir
- Add comments explaining the geometric calculations:
+ // Calculate the extent of the merged surface in the merging direction double newMaxMerge = std::max(thisMaxMerge, otherMaxMerge); double newMinMerge = std::min(thisMinMerge, otherMinMerge); + // Calculate the half-length and center position of the merged surface double newHalfMerge = (newMaxMerge - newMinMerge) / 2; double newMidMerge = (newMaxMerge + newMinMerge) / 2;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Surfaces/PlaneSurface.hpp(2 hunks)Core/src/Surfaces/PlaneSurface.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Surfaces/PlaneSurface.hpp
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: build_debug
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (4)
Core/src/Surfaces/PlaneSurface.cpp (4)
11-24: Approve the new includes and method signature, I do.Well-structured and appropriate for the task at hand, the changes are. Clear documentation through logging, you have provided.
Also applies to: 197-202
203-208: Wise validation of detector elements, this is.Prevent merging of surfaces with detector elements, you must. Clear error message, you provide.
210-222: Use tolerance when comparing rotations, you must.A previous suggestion, this was. Yet, direct comparison of floating-point numbers, still you do. Numerical instabilities, this may cause.
Apply this diff to include a tolerance in the comparison:
-if (std::abs((otherLocal.rotation().matrix() - RotationMatrix3::Identity()) - .norm())) { +constexpr double rotationTolerance = 1e-9; +if ((otherLocal.rotation().matrix() - RotationMatrix3::Identity()).norm() > rotationTolerance) {
235-239: Valid direction checking, you have implemented.Clear and concise validation of merging direction, this is. Good error message, you provide.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/src/Surfaces/PlaneSurface.cpp (1)
216-223: Make the rotation tolerance configurable, you should.Hardcoded tolerance value for rotation comparison, not ideal it is. Consider making it configurable through the
BoundaryToleranceclass or a dedicated configuration parameter.Apply this diff to make the tolerance configurable:
- if ((otherLocal.rotation().matrix() - RotationMatrix3::Identity()).norm() > - tolerance) { + const auto& boundaryTolerance = BoundaryTolerance::None(); + if ((otherLocal.rotation().matrix() - RotationMatrix3::Identity()).norm() > + boundaryTolerance.rotationTolerance(tolerance)) {Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
510-549: Add test cases for edge conditions, you should.Good coverage of basic merging scenarios, the test has. But missing test cases for edge conditions, it is:
- Surfaces touching at exactly the tolerance value
- Surfaces with minimal overlap
- Surfaces with maximum allowed size difference
Assistance in generating additional test cases, offer I do. Want me to help implement these test cases, do you?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Surfaces/PlaneSurface.hpp(1 hunks)Core/src/Surfaces/PlaneSurface.cpp(2 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Surfaces/PlaneSurface.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: unused_files
- GitHub Check: missing_includes
🔇 Additional comments (2)
Core/src/Surfaces/PlaneSurface.cpp (1)
213-214: Address the tolerance value uncertainty, you must.Uncertain about the tolerance value, the TODO comment is. Investigate and document the rationale for using
s_onSurfaceTolerance, we should.Run this script to find other tolerance usages in the codebase:
✅ Verification successful
Appropriate the tolerance value is, young Padawan.
The Force reveals that
s_onSurfaceTolerancevalue of 0.1 micrometers (1e-4 mm) is well-balanced it is:
- Precise enough for surface geometry operations
- Consistent with physical detector resolutions
- Validated through extensive test coverage
Remove the TODO comment, you should. Trust in this tolerance value, we must.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other usages of s_onSurfaceTolerance in rotation comparisons # Test: Search for rotation comparison with s_onSurfaceTolerance rg -A 5 'rotation.*s_onSurfaceTolerance|s_onSurfaceTolerance.*rotation'Length of output: 73
Script:
#!/bin/bash # Find definition and all usages of s_onSurfaceTolerance echo "=== Searching for s_onSurfaceTolerance definition and usage ===" rg "s_onSurfaceTolerance" -B 2 -A 2 echo -e "\n=== Searching for other tolerance constants ===" rg "constexpr.*tolerance" -B 1 -A 1 echo -e "\n=== Searching for Surface-related tolerance definitions ===" rg "Surface.*tolerance|tolerance.*Surface" -B 1 -A 1Length of output: 66199
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
382-382: Include Boost.Test headers correctly, you must.Unknown macro
BOOST_AUTO_TEST_SUITEdetected by static analysis tools, it is. Already included<boost/test/unit_test.hpp>at line 11, we have. False positive from static analysis, this is.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
477-491: Additional test coverage, suggest I do.Test Y direction merging for surfaces with different bounds, we should. More complete test coverage, this would provide.
Apply this diff to add Y direction test:
BOOST_CHECK_THROW(plane->mergedWith(*planeOther, Acts::AxisDirection::AxisX), SurfaceMergingException); +BOOST_CHECK_THROW(plane->mergedWith(*planeOther, Acts::AxisDirection::AxisY), + SurfaceMergingException);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/src/Surfaces/PlaneSurface.cpp(2 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: merge-sentinel
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / build_exatrkx_cpu
🔇 Additional comments (6)
Core/src/Surfaces/PlaneSurface.cpp (2)
196-309: Impressive, the implementation is. Approve it, I do.A well-structured implementation of surface merging, this is. Thorough validation checks and clear merging logic, it has. The Force is strong with this code.
Key features, I observe:
- Detector element association check
- Transform existence validation
- Relative rotation verification
- Bounds compatibility validation
- Surface alignment confirmation
213-214: Verify the tolerance value, you must.Hmm, a TODO comment about tolerance, I see. The
s_onSurfaceTolerancevalue, validate we must, to ensure proper surface alignment detection, we do.Run this script to find the definition and usage of the tolerance value:
✅ Verification successful
Hmm, good tolerance value this is, young padawan.
A well-chosen value of 1e-4, s_onSurfaceTolerance is. Through extensive testing and widespread use in the codebase, proven its worth has been. For geometric operations and surface alignment, appropriate this tolerance is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find the definition and usage of s_onSurfaceTolerance # Search for the definition rg -A 5 $'s_onSurfaceTolerance.*=' # Search for other usages rg -A 5 $'s_onSurfaceTolerance'Length of output: 64967
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (4)
391-413: Well-crafted test case, this is.Thorough validation of misaligned surface handling, it provides. Both X and Y directions, it tests. Both forward and reverse merging, it verifies.
415-444: Comprehensive test case for orthogonal misalignment, this is.Thorough validation of orthogonally misaligned surfaces, it provides. X, Y, and Z translations, it tests. Both forward and reverse merging, it verifies.
446-475: Strong with angular misalignment testing, this test case is.Thorough validation of surfaces with angular misalignment, it provides. Rotations around X, Y, and Z axes, it tests. Both forward and reverse merging, it verifies.
493-531: Most impressive test case for successful merging, this is.Thorough validation of successful merging scenarios, it provides:
- X and Y direction merging, it tests
- Forward and reverse merging, it verifies
- Bounds equality, it confirms
- Merge direction indicator, it validates
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (1)
494-542: Additional assertions for merged surface properties, needed they are.Test verifies successful merging scenarios, it does. But more assertions for the merged surface properties, beneficial they would be.
Add these assertions to strengthen the test:
BOOST_CHECK_EQUAL(planeXMerged->bounds(), *expectedBoundsX); BOOST_CHECK_EQUAL(planeXMerged->center(gctx), base * Vector3::UnitX() * 1); + BOOST_CHECK(planeXMerged->transform(gctx).rotation() == base.rotation()); + BOOST_CHECK_EQUAL(planeXMerged->normal(gctx), plane->normal(gctx)); auto expectedBoundsY = std::make_shared<const RectangleBounds>(1, 4); auto [planeYMerged, reversedY] = plane->mergedWith(*planeY, Acts::AxisDirection::AxisY, *logger); BOOST_REQUIRE_NE(planeYMerged, nullptr); BOOST_CHECK(!reversedY); BOOST_CHECK_EQUAL(planeYMerged->bounds(), *expectedBoundsY); BOOST_CHECK_EQUAL(planeYMerged->center(gctx), base * Vector3::UnitY() * 2); + BOOST_CHECK(planeYMerged->transform(gctx).rotation() == base.rotation()); + BOOST_CHECK_EQUAL(planeYMerged->normal(gctx), plane->normal(gctx));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Surfaces/PlaneSurface.hpp(2 hunks)Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Surfaces/PlaneSurface.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (5)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (5)
391-413: Well-structured test for surface overlap detection, this is.Test verifies that overlapping surfaces cannot be merged, it does. Both directions and both merge orders, it checks. Thorough testing, I sense.
415-444: Comprehensive test for misalignment detection, hmm.Test verifies that surfaces with incorrect positioning cannot be merged, it does. All axes and both merge orders, it checks. Good coverage, this provides.
446-475: Thorough test for rotational misalignment, this is.Test verifies that rotated surfaces cannot be merged, it does. Different rotation axes and both merge orders, it checks. Well-structured test cases, these are.
477-492: Test for incompatible bounds, I see.Test verifies that surfaces with different bound sizes cannot be merged, it does. Single direction sufficient it is, as symmetry exists.
382-390:⚠️ Potential issueMissing Boost.Test header, I sense.
Include the proper header for Boost.Test macros, you must.
Apply this diff to fix the issue:
+#include <boost/test/unit_test.hpp>Likely invalid or redundant comment.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (2)
384-384: Consider logger naming improvement, we should.More specific logger name than "UnitTests" for better log filtering, suggest I do. Match it with test suite name "PlaneSurfaceMerging", we could.
-auto logger = Acts::getDefaultLogger("UnitTests", Acts::Logging::VERBOSE); +auto logger = Acts::getDefaultLogger("PlaneSurfaceMerging", Acts::Logging::VERBOSE);
494-542: Success cases testing, thorough and complete they are.Test cases for successful merging in both X and Y directions with:
- Proper bounds validation
- Center position verification
- Reversal flag checking
- Bidirectional merging validation
Additional assertions for merged surface properties, suggest I do.
Add these assertions to strengthen the test:
BOOST_CHECK_EQUAL(planeXMerged->center(gctx), base * Vector3::UnitX() * 1); +BOOST_CHECK(planeXMerged->transform(gctx).rotation() == base.rotation()); +BOOST_CHECK_EQUAL(planeXMerged->normal(gctx), plane->normal(gctx)); BOOST_CHECK_EQUAL(planeYMerged->center(gctx), base * Vector3::UnitY() * 2); +BOOST_CHECK(planeYMerged->transform(gctx).rotation() == base.rotation()); +BOOST_CHECK_EQUAL(planeYMerged->normal(gctx), plane->normal(gctx));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp
[error] 382-382: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
Tests/UnitTests/Core/Surfaces/PlaneSurfaceTests.cpp (5)
9-9: Approve header inclusion, I do.Fixed the missing Boost.Test header issue from previous review, you have. Pleased with this correction, I am.
391-413: Well-structured test for surface overlap detection, this is!Test cases for both X and Y directions with bidirectional checks, comprehensive they are. Clear validation of overlap detection, it provides.
415-444: Thorough misalignment testing, observe I do.Test cases for shifts in X, Y, and Z directions with bidirectional checks, complete they are. Good coverage of alignment requirements, this provides.
446-475: Rotation testing, comprehensive it is.Test cases for rotations around different axes, thorough they are. Clear validation of rotation constraints, this provides.
477-492: Bounds validation testing, well-implemented it is.Test case for different bounds sizes, proper validation it provides. Clear assertion of bounds compatibility requirements, this ensures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Core/src/Surfaces/PlaneSurface.cpp (2)
236-240: Optimize direction validation using switch statement, we could.More efficient validation using switch statement, achieve we can.
- if (direction != AxisDirection::AxisX && direction != AxisDirection::AxisY) { - throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), - "PlaneSurface::merge: invalid direction " + - axisDirectionName(direction)); - } + switch (direction) { + case AxisDirection::AxisX: + case AxisDirection::AxisY: + break; + default: + throw SurfaceMergingException(getSharedPtr(), other.getSharedPtr(), + "PlaneSurface::merge: invalid direction " + + axisDirectionName(direction)); + }
242-291: Extract helper methods for better organization, we should.Long and complex this segment is. Extract validation logic into helper methods, improve readability it would.
+ private: + void validateNonMergingDimensions(const RectangleBounds* thisBounds, + const RectangleBounds* otherBounds, + bool mergeX, + double tolerance) const { + double thisHalfNonMerge = + mergeX ? thisBounds->halfLengthY() : thisBounds->halfLengthX(); + double otherHalfNonMerge = + mergeX ? otherBounds->halfLengthY() : otherBounds->halfLengthX(); + + if (std::abs(thisHalfNonMerge - otherHalfNonMerge) > tolerance) { + throw SurfaceMergingException( + getSharedPtr(), other.getSharedPtr(), + "PlaneSurface::merge: surfaces have different non-merging lengths"); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Surfaces/PlaneSurface.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: missing_includes
🔇 Additional comments (5)
Core/src/Surfaces/PlaneSurface.cpp (5)
197-208: Approve the initial validation, I do!Wise validation of detector elements, this is. Prevent unexpected behavior in detector-associated surfaces, it does.
214-215: Address the TODO comment about tolerance, you must.Uncertainty about tolerance value, expressed in TODO comment it is. Document the reasoning for chosen tolerance value or adjust it based on geometric considerations, you should.
217-224: Use tolerance when comparing rotations, you must.Directly comparing floating-point numbers, wise it is not. Introduce a small tolerance to account for numerical inaccuracies, you should.
226-234: Clear and precise bounds validation, this is!Restrict merging to rectangle bounds only, it does. Clear error messages for unsupported cases, it provides.
293-308: Elegant final surface creation, this is!Clear and efficient implementation for creating merged surface, you have achieved. Well-chosen variable names and helper functions, you have used.
|
Adding
mergedWithmethod for thePlaneSurface.The logic is similar to that of the
CylinderSurface-- to be merged two surfaces have to touch each other along the merging direction bound and same-sized aligned bounds along the direction that is orthogonal to the merging direction.The main difference with the
CylinderSurfaceimplementation is in the requirement of relative rotation matrix to be exactly identity.The convention for the binning values is defined by the local surface coordinates.
Summary by CodeRabbit
Release Notes
New Features
Tests
Bug Fixes
Documentation