feat: Adding cuboid portal shell #4047
Conversation
WalkthroughA significant transformation in geometry, we witness. New portal shells for cuboid and cylinder volumes, introduced they have been. Abstraction and modularity, the core principles are. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 0
🧹 Nitpick comments (7)
Core/include/Acts/Geometry/CylinderPortalShell.hpp (1)
97-173: Composite shell concept, nicely done.
A no-op inapplyToVolumewell-chosen it is, yet ensure usage known to all is.Core/include/Acts/Geometry/CuboidPortalShell.hpp (1)
104-212: Stacking multiple cuboid shells, a powerful feature.
No-op forapplyToVolumewise is, but consider clarifying future usage, hmm?Core/src/Geometry/CuboidPortalShell.cpp (3)
66-68: Raw pointer from shared pointer, you return.Mindful you should be, of ownership confusion. To keep usage consistent, a direct shared pointer might you prefer.
88-101: Check validity upon applying, you do.Exception thrown if invalid portal is found, wise that is. Consider adding logs for debugging, beneficial it might be.
159-247: Stacking and fusing, large this function is.Refactor into separate smaller methods someday, you might. Complexity reduced, the code will be.
Core/src/Geometry/CuboidVolumeStack.cpp (2)
771-771: Accumulate gap extension, hrrrm.
Take care repeated expansions not to overshoot, you must.
825-825: Expanding gap once more, hmm.
Refactor potential, if expansions become frequent, yes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Core/include/Acts/Geometry/CylinderPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/PortalShell.hpp(1 hunks)Core/src/Geometry/CMakeLists.txt(1 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(2 hunks)Core/src/Geometry/CuboidVolumeStack.cpp(12 hunks)Core/src/Geometry/CylinderPortalShell.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(5 hunks)Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp(6 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: missing_includes
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
🔇 Additional comments (63)
Core/include/Acts/Geometry/PortalShell.hpp (1)
11-12: Hmm, includes you have.
Useful their addition is, yes. Freed from old includes, well-structured your file becomes.Core/include/Acts/Geometry/CylinderPortalShell.hpp (5)
1-10: License block and header guard, good they are.
Conform to project style guidelines, they do.
11-18: Wise includes, these are.
Sufficient coverage for geometry classes, mmh.
21-48: Abstract base class clarity, sense it makes.
Implementation offill(...)flawed it appears not, but concurrency concerns verify you should, young padawan.
50-55: Overload operator, quite handy it is.
For debugging or logging, beneficial it can be.
56-95: Implementation of SingleCylinderPortalShell, neat it is.
Handles portals in an array, straightforward the approach is.Core/include/Acts/Geometry/CuboidPortalShell.hpp (5)
1-10: Top licensing block correct, yes.
Compliance with MPL v2.0 satisfied, continue you may.
11-21: Includes for cuboid geometry, relevant they are.
Extended shaping, well-provided.
24-53: Base class for cuboid shells, robust it appears.
Method signatures consistent are with PortalShellBase. Thorough, is the design.
55-60: Operator<< ensures easy face logging.
Helpful in debugging, rely on it you can.
61-102: SingleCuboidPortalShell, well structured.
A single volume approach, quite direct. Verify correct transformations, you should.Core/src/Geometry/CuboidPortalShell.cpp (18)
29-38: Wise the fill method is, yes.Hmmm, addPortal after fill you do. Efficient and clear, the approach seems. Approve it, the Jedi Council does.
40-64: Check the volume bounds, you correctly do.Strong your validation is. Throw an exception if volume bounds are not cuboid, prudent indeed.
70-72: Direct portal pointer retrieval.Hrrrm. Implementation simple and straightforward it is. Good, it looks.
74-79: Validation strong, your setPortal method has.Check valid pointer, you do. Good is that. Summon no changes, do I.
81-86: Count portals, you do.Incremental and clean, this approach is. A straightforward mechanism, yes.
103-107: Aggregation of validity, I sense.Verify each portal's health, you do. Good path, this is.
109-113: Shared volume name, you label.String conversion, correct it is. Acceptable usage, I find.
115-134: Constructor stacking, a nice approach.Direction properly assigned via switch, handle out-of-scope axis with exception, you do. Good.
136-143: Constructor with custom direction, you have.Mapping direction to axis with transform, clever it is.
145-157: Direction-based axis detection, rely on 1e-4 you do.Hmmm, edge cases might occur. Confirm robust enough for alignment checks, you must.
249-251: Fixed size of six, the Shell has.Straight is this. Approve it, the Council does.
253-255: Portal pointer returned, your method does.Truly direct is this. Good enough, it is.
257-263: Portal pointer from front or back shell, you choose.Dependent on face, wise it is.
265-278: SetPortal logic, consistent across faces it is.Apply to front, back, or all shells. Clear is the approach.
280-285: Validation of each shell, you do.All must be valid, yes. No improvements needed, I see.
287-289: Transform from the front shell, you provide.Adequate design, this appears.
291-301: Readable label, your string composition is.Concise the format is. Acceptable, it is.
303-321: Operator<< for face, a good convenience.Negative or positive XY, YZ, ZX, all covered they are. Approved, it is.
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (3)
12-12: Include of BoundarySurfaceFace, introduced you have.Needed for face definitions, yes. Proceed, you can.
62-72: New enum class Face, well-structured it is.Sync with BoundarySurfaceFace, you have. Good approach, consistent naming.
172-181: boundsFromAxisDirection and facesFromAxisDirection, introduced.Robust these helpers look. Provide clarity in axis usage, they do.
Core/src/Geometry/CuboidVolumeBounds.cpp (2)
Line range hint
172-185: boundsFromAxisDirection, a wise rename from fromAxisDirection.Checks axis direction is valid, very good practice.
187-211: facesFromAxisDirection, you define.Return correct front, back, side faces for each axis, you do. Hmmm, comprehensive it is.
Core/src/Geometry/CylinderPortalShell.cpp (1)
9-9: Header updated, hmm.
Switched from the general PortalShell to the specialized CylinderPortalShell, a clean separation, yes.
But rely on C++20 ranges you still do; confirm your build environment includes them, you must.Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp (7)
20-20: Include specialized shell, wise it is.
Clarity improved by referencing the cylinder file directly.
395-397: SurfaceMergingException tested, mm?
Strong coverage, you show.
443-445: Again test, the merge fails, yes.
Duplicated coverage ensures no regression, it does.
495-497: AxisZ meltdown scenario, tested well you have.
Confidence in code stability, strong it is.
544-546: Expected invalid_argument, hrrm.
Good to confirm misconfiguration yields error.
744-748: Verifying the link is not null, test this does.
Yes, thorough approach displayed, I see.
733-735:⚠️ Potential issueMisapplied BOOST_CHECK_THROW, sense it makes not!
'nullptr' for an exception type is incorrect. An actual exception must you specify.Propose fix, I do:
-BOOST_CHECK_THROW( - shell.portal(InnerCylinder)->getLink(Direction::OppositeNormal()), - nullptr); +BOOST_CHECK_THROW( + shell.portal(InnerCylinder)->getLink(Direction::OppositeNormal()), + std::runtime_error);Likely invalid or redundant comment.
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
1-786: Mmm, comprehensive test coverage for cuboid shells, achieved you have.
Construction, assignment, nested stacks—exercise them all, you do.
Approach robust, it is.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 194-194: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Core/src/Geometry/CuboidVolumeStack.cpp (11)
51-52: Obtaining half-length from axis direction, good approach that is.
Safer and more consistent, indeed.
284-286: Half-length aggregator code, yes.
Maintain clarity in bounding volumes, you do.
328-328: Local bound index for direction, hmm.
Improves readability, it does.
453-456: A new gap volume you build, prudent it is.
Check negative or zero dimension, you must.
548-549: Mapping orthogonal axes, correct the logic seems.
No conflict do I sense.
686-686: Expanding volume meticulously, you do.
Keep the design stable, yes.
704-704: Again retrieving the key, consistent approach.
Good, I find.
781-781: Dimension assignment, yes.
No problem, do I see.
798-801: Gap volume creation repeated, consistent you are.
Validate correct bounds for all cases, ensure you do.
833-836: Creation of second gap, uniform your pattern is.
In harmony, it remains.
849-852: Upper side gap logic, final piece it is.
Sufficient coverage verify, you must.Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp (5)
88-90: Renamed method, well done it is.
Clear the naming scheme has become. Approve, I do.
427-429: Consistent usage, strong the Force is.
Chaining the same pattern throughout, maintain it you shall.
504-506: Uniform approach to directional bounds, good it seems.
Continue supporting different axis directions, you must, with thorough coverage in tests.
765-767: Bound directions, consistent you keep.
Validate boundary corner cases, wise it is.
930-932: Harmonious method calls, seeing I am.
Redundant calls, I do not detect. All is well.Core/src/Geometry/CMakeLists.txt (2)
46-46: Splitting the portal shell, diligent you are.
A modular path to clarity, found you have. Continue, you should.
47-47: Introducing CuboidPortalShell, hamper complexity it does.
Ensuring references remain intact, be mindful you must.Tests/UnitTests/Core/Geometry/CMakeLists.txt (2)
38-38: Adding CylinderPortalShell tests, wise you are.
Coverage in place you have. Approve, I do.
39-39: CuboidPortalShell tests, add them you do.
Essential for confidence in new geometry, yes!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Core/include/Acts/Geometry/CuboidPortalShell.hpp (3)
50-50: Implementation details, calling volume’s fill and transform, you do.
Neat approach, but ensure performance overhead minimal remains when nested shells, you have.Also applies to: 52-52
90-92: Directly returning volume’s transform, you do.
Simplicity, powerful it is. Just confirm that concurrency or external modifications to the transform are properly handled.Need concurrency checks, do you?
155-167: stackShell method, private it is.
Reminded, we must be, that descriptive logging or error checks can aid debugging if stacking goes awry.Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
139-192: PortalAssignment test, thorough it appears.
Use dynamic_cast you do. Avoid you must, if not essential it is. Possibly an alternative, we can consider.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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 (19)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- 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: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: macos
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (12)
Core/include/Acts/Geometry/CuboidPortalShell.hpp (4)
27-27: A well-defined base class, you have introduced.
Implements a clear structure for future expansions, it does. Flexible design, yes.
36-47: Consistent use of pure virtual methods, I see.
Implement them your derived classes must, hmmm. Use references to a face-based enum, both direct and shared pointer retrieval you have. Good clarity, it provides.
63-70: Constructor ensures single-volume management, hmmm.
Wise it is to provide robust checks against invalid volumes. Good that you throw for non-cuboid bounds. Enough logs, do you have?
138-140: No-op applyToVolume, you have.
Composite shell design, yes. Ensure that all sub-shells have applied themselves to their volumes. Important step, it might be.Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (3)
62-72: Enum class Face, introduced you have.
Nicely named, consistent with "BoundarySurfaceFace" it is. Clean, it remains.
172-173: Method renamed boundsFromAxisDirection, clarity, yes.
Break existing code you must not. If used widely, ensure all references updated, yes?
174-182: facesFromAxisDirection method, new it is.
Provides helpful tuple for face determination, hmmm. Good synergy with direction-based logic, yes.Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (5)
53-65: Test of SingleCuboidPortalShell creation, well structured, it is.
Thrown exceptions, you verify. Perfect coverage, hmmm.
332-393: NestedStacks test, a comprehensive scenario, it provides, yes.
Edge cases well addressed they seem to be. Good coverage.
53-53: Unknown macro, the static analyzer complains about.
Worry not. A standard Boost macro, it is. Configuration needed, the tool does.Also applies to: 194-194
743-767: Fill test verifies inter-volume portal creation, hmmm.
Essential coverage, it provides. Reliable linking, mindful we must be.
769-781: RegisterInto test, ensures portal registration, yes.
Six portals, a cuboid does have. The test confirms no missing face, your code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/core/geometry/concepts.md (1)
57-64: Relationships between portal shells, explain we must.Wise addition of cuboid portal shells, this is. Yet clarity for young padawans, we need more of. The relationship between these shells, documented it should be. Like the cylinder shells above, a brief introduction explaining their purpose and hierarchy, beneficial it would be.
Consider adding this introduction before the classes:
+### Cuboid Portal Shells + +The cuboid portal shells provide portal management for cuboid-shaped volumes. The hierarchy consists of: +- `CuboidPortalShell`: Base class for cuboid-shaped portal shells +- `SingleCuboidPortalShell`: Manages portals for a single cuboid volume +- `CuboidStackPortalShell`: Handles portal management for stacked cuboid volumes + :::{doxygenclass} Acts::CuboidPortalShellCore/include/Acts/Geometry/CylinderPortalShell.hpp (2)
50-55: Output operator, well structured it is.
Simple handling of enum faces, you use. Perhaps a small table or switch to map face to string more systematically, you could.
56-96: Clear separation achieved, SingleCylinderPortalShell does.
Constructor, you define nicely, accepting aTrackingVolumereference. Good that you mark methodsfinal, it is. Concurrency in multi-threaded contexts, watch carefully if many threads modify these portals at once, hmmm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/CylinderPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp(1 hunks)Core/src/Geometry/Blueprint.cpp(1 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(1 hunks)Core/src/Geometry/StaticBlueprintNode.cpp(1 hunks)docs/core/geometry/concepts.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp
- Core/src/Geometry/Blueprint.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Geometry/CuboidPortalShell.hpp
🧰 Additional context used
📓 Learnings (1)
Core/src/Geometry/CylinderContainerBlueprintNode.cpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp:28-29
Timestamp: 2024-12-05T20:21:24.253Z
Learning: In `CylinderContainerBlueprintNode`, validation that child volumes are cylinders is enforced by `CylinderVolumeStack`.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
- 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, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (5)
docs/core/geometry/concepts.md (1)
Line range hint
1-4: Address the TODO note, young Padawan must.Incomplete, this documentation remains. A great service to future Jedi, you would do, by completing these teachings. The ways of geometry concepts, documented fully they must be.
Help you with expanding this documentation, shall I? The force is strong with conceptual clarity, it is.
Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp (1)
13-13: Changed your include toCylinderPortalShell, hmmm.Strong with the Force, your code is. Yet verify you must, that references to the removed
PortalShell.hppremain not, lest confusion arise. Harmonious with the newCylinderPortalShellapproach, all code must be.To locate any remaining references to
PortalShell.hpp, run this script:✅ Verification successful
Hmmmm... Valid this change is, young padawan.
The Force reveals that
PortalShell.hpp, still serves its purpose it does, in the foundation of both cylinder and cuboid implementations. Changed toCylinderPortalShell.hppyour include has, because specific cylinder wisdom you seek. Worry not, for balance in the code there is.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to the old portal shell header: ast-grep --pattern $'#include "Acts/Geometry/PortalShell.hpp"'Length of output: 607
Core/include/Acts/Geometry/CylinderPortalShell.hpp (1)
1-48: Foundation strong, your base class is.
Clarity, it has: a well-defined interface for handling cylinder portals. Overridingfill()fromPortalShellBase, you do. Good, this is. Descriptive comments, maintain you must.Core/src/Geometry/StaticBlueprintNode.cpp (1)
11-11: Right the include is, bridging cylinders you now do.
The newly introducedCylinderPortalShell, you require. Good practice it is, yes.Core/src/Geometry/CylinderContainerBlueprintNode.cpp (1)
12-12: Include for CylinderPortalShell, essential for your new logic it is.
Ensures correct creation and interaction with the cylinder shells. Verified, yes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Core/src/Geometry/CuboidPortalShell.cpp (4)
29-38: Improve error handling and logging in fill method, hmm.For robust implementation, consider these suggestions I do:
- Add debug logging for portal filling operations
- Handle edge cases where volume is nullptr
void CuboidPortalShell::fill(TrackingVolume& volume) { + ACTS_VERBOSE("Filling portals for volume: " << volume.volumeName()); for (Face face : {negativeXYPlane, positiveXYPlane, negativeYZPlane, positiveYZPlane, negativeZXPlane, positiveZXPlane}) { const auto& portalAtFace = portalPtr(face); if (portalAtFace != nullptr) { + ACTS_VERBOSE("Filling portal at face: " << face); portalAtFace->fill(volume); volume.addPortal(portalAtFace); + } else { + ACTS_DEBUG("No portal found at face: " << face); } } }
52-56: Improve lambda naming and documentation, we must.The lambda name 'handle' lacks clarity in its purpose. Rename to better reflect its role in portal initialization.
- auto handle = [&](Face face, std::size_t from) { + auto initializePortal = [&](Face face, std::size_t from) { const auto& source = orientedSurfaces.at(from); m_portals.at(toUnderlying(face)) = std::make_shared<Portal>(source.direction, source.surface, *m_volume); };
81-86: Simplify size calculation with std::count_if, we shall.More elegant solution using std::count_if, I sense.
std::size_t SingleCuboidPortalShell::size() const { - std::size_t count = 0; - std::ranges::for_each( - m_portals, [&count](const auto& portal) { count += portal ? 1 : 0; }); - return count; + return std::count_if(m_portals.begin(), m_portals.end(), + [](const auto& portal) { return portal != nullptr; }); }
147-156: Magic numbers in directionToAxis, a path to the dark side they are.Extract the tolerance value to a named constant, improve readability we must.
+ static constexpr double kDirectionTolerance = 1e-4; if ((direction - Vector3::UnitX()).norm() < 1e-4) { return AxisDirection::AxisX; } else if ((direction - Vector3::UnitY()).norm() < 1e-4) { return AxisDirection::AxisY; } else if ((direction - Vector3::UnitZ()).norm() < 1e-4) { return AxisDirection::AxisZ;Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (2)
55-137: Additional test cases, we need.Missing negative test cases for invalid inputs, I see. Add these we must:
- Test with zero-sized bounds
- Test with negative-sized bounds
- Test with invalid transformations
+BOOST_AUTO_TEST_CASE(ConstructionFromInvalidVolume) { + // Test with zero-sized bounds + auto zeroCube = makeVolume(0_mm, 40_mm, 50_mm); + BOOST_CHECK_THROW(SingleCuboidPortalShell{zeroCube}, std::invalid_argument); + + // Test with negative-sized bounds + auto negativeCube = makeVolume(-30_mm, 40_mm, 50_mm); + BOOST_CHECK_THROW(SingleCuboidPortalShell{negativeCube}, std::invalid_argument); +}
326-714: Edge cases in nested stacks, test we should.The nested stacks test, comprehensive it is. Yet, edge cases we must add:
- Test with empty shell list
- Test with single shell
- Test with maximum supported shells
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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 (22)
- GitHub Check: merge-sentinel
- GitHub Check: build_debug
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: unused_files
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: macos
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (1)
Core/src/Geometry/CuboidPortalShell.cpp (1)
303-321: Well implemented, the stream operator is.Clear and complete implementation for all face types, I see.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Core/src/Geometry/TrivialPortalLink.cpp (1)
36-37: Descriptive message, add you could.
Better context in the thrown error helpful would be, especially for debugging. Suggest including details such as position or tolerance in the message, you might. See example below.- throw_assert(m_surface->isOnSurface(gctx, position, BoundaryTolerance::None(), tolerance), - "Trivial portal lookup point should be on surface"); + throw_assert(m_surface->isOnSurface(gctx, position, BoundaryTolerance::None(), tolerance), + "Trivial portal lookup point off surface. Position=" + + std::to_string(position.x()) + "," + + std::to_string(position.y()) + "," + + std::to_string(position.z()));Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (2)
38-49: Document the helper functions, you should.Clear the purpose may be, but documented well these functions should be. Help future Padawans understand their usage, it will.
Add documentation like this, you should:
+/** + * @brief Generates a unique volume index for test volumes + * @return Incrementing index for volume identification + */ std::size_t getVolumeIndex() { static std::size_t i = 1; return i++; } +/** + * @brief Creates a test volume with given cuboid bounds + * @param pars Parameters for CuboidVolumeBounds construction + * @return TrackingVolume instance with unique name + */ auto makeVolume(auto&&... pars) {
742-754: Additional test scenarios, consider you should.Though strong the test suite is, strengthen it further we can. Consider these scenarios, you might:
- Invalid portal assignments
- Boundary conditions for volume dimensions
- Transform validation for rotated volumes
Help you implement these additional test cases, shall I?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/src/Geometry/TrivialPortalLink.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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 (19)
- 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: build_debug
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- 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 / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
🔇 Additional comments (3)
Core/src/Geometry/TrivialPortalLink.cpp (1)
13-13: Include from the Force, you must.
Hmmm, wise to replace debug-only checks with run-time exceptions, it is. Flexible and production-friendly, the error handling becomes.Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (2)
1-33: Proper structure and includes, I see.Well-organized the file header and includes are. Complete and appropriate they appear to be.
55-714: Comprehensive test coverage, achieved you have.Strong with the Force these test cases are. Particularly impressive, the ASCII diagram for nested stacks is. Clear understanding of the geometry it provides.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 194-194: 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 (3)
Core/src/Geometry/CuboidPortalShell.cpp (2)
40-64: Consider caching the bounds cast, we should.Wise the error handling is, but repeated dynamic_cast in orientedSurfaces call, performance impact it may have. Cache the bounds after validation, we must.
SingleCuboidPortalShell::SingleCuboidPortalShell(TrackingVolume& volume) : m_volume{&volume} { if (m_volume->volumeBounds().type() != VolumeBounds::BoundsType::eCuboid) { throw std::invalid_argument("Invalid volume bounds type"); } - const auto& bounds = - dynamic_cast<const CuboidVolumeBounds&>(m_volume->volumeBounds()); + const auto& volumeBounds = m_volume->volumeBounds(); + const auto& bounds = static_cast<const CuboidVolumeBounds&>(volumeBounds);
149-161: Named constant for comparison threshold, define we must.Magic number 1e-4, unclear its purpose is. More descriptive error message, helpful it would be.
+namespace { +constexpr double kAxisAlignmentThreshold = 1e-4; // Radians +} + AxisDirection CuboidStackPortalShell::directionToAxis( const Vector3& direction) { - if ((direction - Vector3::UnitX()).norm() < 1e-4) { + if ((direction - Vector3::UnitX()).norm() < kAxisAlignmentThreshold) { return AxisDirection::AxisX; - } else if ((direction - Vector3::UnitY()).norm() < 1e-4) { + } else if ((direction - Vector3::UnitY()).norm() < kAxisAlignmentThreshold) { return AxisDirection::AxisY; - } else if ((direction - Vector3::UnitZ()).norm() < 1e-4) { + } else if ((direction - Vector3::UnitZ()).norm() < kAxisAlignmentThreshold) { return AxisDirection::AxisZ; } else { throw std::invalid_argument( - "CuboidStackPortalShell: Direction does not coincide with axes"); + "Direction must be aligned with X, Y, or Z axis within " + + std::to_string(kAxisAlignmentThreshold) + " radians"); } }Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
327-715: Comprehensive tests these are, but long they have become.ASCII art diagram, clarity it brings. But split this test into smaller focused cases, we should:
- Basic stacking tests
- Portal resolution tests
- Edge case tests
Example split:
BOOST_AUTO_TEST_CASE(NestedStacks_BasicSetup) { // Test volume creation and basic stacking } BOOST_AUTO_TEST_CASE(NestedStacks_PortalResolution) { // Test portal resolution for each volume } BOOST_AUTO_TEST_CASE(NestedStacks_EdgeCases) { // Test boundary conditions and edge cases }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Core/src/Geometry/TrivialPortalLink.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/src/Geometry/TrivialPortalLink.cpp
- Core/include/Acts/Geometry/CuboidPortalShell.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (5)
Core/src/Geometry/CuboidPortalShell.cpp (3)
29-38: Clean and efficient, this implementation is.Well-structured method with proper null checks and RAII principles, I see. Elegant iteration through faces, it has.
163-251: Break down this method into smaller functions, we must.Too many responsibilities this method has. Split it into focused functions for validation, initialization, and merging, we should.
307-325: Well-crafted stream operator, this is.Modern C++ features it uses wisely. All cases it handles gracefully, including invalid ones.
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (2)
717-755: Well-focused test cases, these are.Single responsibility each test maintains. Clear assertions they have. Pleased with this implementation, I am.
38-51: Thread safety concerns in getVolumeIndex, I sense.Static counter without synchronization, race conditions it may cause in parallel tests. Consider atomic counter or test-specific indices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Core/src/Geometry/CuboidPortalShell.cpp (1)
163-248: 🛠️ Refactor suggestionLong method stackShell is. Split into smaller functions, we must.
Previous wisdom holds true. Too many responsibilities this method has. Extract these operations into separate methods we should:
- validateShells()
- initializeSurfaceDirections()
- sortShellsByDirection()
- mergePortals()
- fusePortals()
🧹 Nitpick comments (4)
Core/src/Geometry/CuboidPortalShell.cpp (4)
29-38: Validate portals before filling, we must.Additional validation for portal validity before filling, add we should. Prevent silent failures in case of invalid portals, this will.
void CuboidPortalShell::fill(TrackingVolume& volume) { for (Face face : {negativeXYPlane, positiveXYPlane, negativeYZPlane, positiveYZPlane, negativeZXPlane, positiveZXPlane}) { const auto& portalAtFace = portalPtr(face); - if (portalAtFace != nullptr) { + if (portalAtFace != nullptr) { + if (!portalAtFace->isValid()) { + throw std::runtime_error("Invalid portal found at face " + + std::to_string(static_cast<int>(face))); + } portalAtFace->fill(volume); volume.addPortal(portalAtFace); } } }
52-56: More descriptive name for lambda function, give we must.Rename 'handle' to 'initializePortalForFace', clearer intent it will show. Additional validation for source surface, add we should.
- auto handle = [&](Face face, std::size_t from) { + auto initializePortalForFace = [&](Face face, std::size_t from) { const auto& source = orientedSurfaces.at(from); + if (!source.surface) { + throw std::runtime_error("Invalid surface for face " + + std::to_string(static_cast<int>(face))); + } m_portals.at(toUnderlying(face)) = std::make_shared<Portal>(source.direction, source.surface, *m_volume); };
149-161: Magic number, a constant become must.Extract the epsilon value 1e-4 into a named constant, better maintainability it will bring.
+ static constexpr double kDirectionEpsilon = 1e-4; if ((direction - Vector3::UnitX()).norm() < 1e-4) { return AxisDirection::AxisX; } else if ((direction - Vector3::UnitY()).norm() < 1e-4) { return AxisDirection::AxisY; } else if ((direction - Vector3::UnitZ()).norm() < 1e-4) { return AxisDirection::AxisZ; }
304-322: Compile-time exhaustiveness check, add we should.Static assertion for switch exhaustiveness, prevent future enum values from being missed it will.
std::ostream& operator<<(std::ostream& os, CuboidPortalShell::Face face) { + static_assert(std::is_enum_v<CuboidPortalShell::Face>, + "Face must be an enum type"); switch (face) { using enum CuboidVolumeBounds::Face; case positiveXYPlane: return os << "positiveXYPlane"; // ... other cases ... default: + assert(false && "Unhandled face enum value"); return os << "Invalid face"; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Geometry/CuboidPortalShell.hpp
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: macos
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: build_debug
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
| CuboidVolumeBounds::Face m_frontFace = negativeYZPlane; | ||
| /// The cuboid face positioned last along the stacking direction | ||
| CuboidVolumeBounds::Face m_backFace = positiveYZPlane; |
There was a problem hiding this comment.
The right hand side is BoundarySurfaceFace, right? Maybe we could initialize them directyly from Face?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (2)
62-72: Consistent naming convention with CylinderVolumeBounds, consider we must.Hmm, in CylinderVolumeBounds, capitalized names like
PositiveDiscwe use. For consistency across the codebase, similar naming convention here adopt we should.Apply this change, you should:
enum class Face : unsigned int { - NegativeXYPlane = BoundarySurfaceFace::negativeFaceXY, - PositiveXYPlane = BoundarySurfaceFace::positiveFaceXY, - NegativeYZPlane = BoundarySurfaceFace::negativeFaceYZ, - PositiveYZPlane = BoundarySurfaceFace::positiveFaceYZ, - NegativeZXPlane = BoundarySurfaceFace::negativeFaceZX, - PositiveZXPlane = BoundarySurfaceFace::positiveFaceZX + NegativeXY = BoundarySurfaceFace::negativeFaceXY, + PositiveXY = BoundarySurfaceFace::positiveFaceXY, + NegativeYZ = BoundarySurfaceFace::negativeFaceYZ, + PositiveYZ = BoundarySurfaceFace::positiveFaceYZ, + NegativeZX = BoundarySurfaceFace::negativeFaceZX, + PositiveZX = BoundarySurfaceFace::positiveFaceZX };
174-182: Strong with the Force, this implementation is.Well-structured and documented, the method is. Yet, enhance the documentation with an example, we could.
Add an example to the documentation, like this:
/// Convert axis direction to a set of corresponding cuboid faces /// in local coordinate convention /// @param direction the axis direction to convert /// @return A tuple of cuboid faces with the following ordering convention: /// (1) negative face orthogonal to the axis direction /// (2) positive face orthogonal to the axis direction /// (3) list of side faces parallel to the axis direction + /// @example For AxisDirection::AxisX, returns: + /// (NegativeYZPlane, PositiveYZPlane, + /// [NegativeXYPlane, PositiveXYPlane, NegativeZXPlane, PositiveZXPlane])Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (4)
1-33: Document the test suite's purpose and scope, you must.Clear documentation for future Padawans, essential it is. Add a brief comment block explaining the test suite's purpose, scope, and the components being tested.
// This file is part of the ACTS project. // // Copyright (C) 2016 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 https://mozilla.org/MPL/2.0/. +/** + * @brief Unit tests for CuboidPortalShell and related classes + * + * Tests the functionality of portal shells for cuboid volumes, including: + * - Construction from volumes + * - Portal assignment and management + * - Nested stacks of portal shells + * - Volume filling and portal registration + */ + #include <boost/test/data/monomorphic/fwd.hpp>
38-49: Document the helper functions, we should.Wisdom in helper functions lies, but documentation they lack. Add comments explaining their purpose and parameters.
+/** + * @brief Generates unique volume indices for test volumes + * @return A unique index for each call + */ std::size_t getVolumeIndex() { static std::size_t i = 1; return i++; } +/** + * @brief Creates a cuboid tracking volume with given parameters + * @param pars Parameters for the CuboidVolumeBounds + * @return A TrackingVolume with unique name and identity transform + */ auto makeVolume(auto&&... pars) {
269-657: Organize test data better, we must.In the NestedStacks test case, complex test data and assertions, I see. Extract test data setup to improve readability and maintainability.
+namespace { +struct TestVolumeConfig { + Vector3 position; + double xHalfLength; + double yHalfLength; + double zHalfLength; + std::string name; +}; + +const std::vector<TestVolumeConfig> nestedStacksConfig = { + {{0, 0, 0}, 30_mm, 100_mm, 200_mm, "vol1"}, + {{0, 0, 300_mm}, 30_mm, 100_mm, 100_mm, "vol2"}, + {{0, 0, 600_mm}, 30_mm, 100_mm, 200_mm, "vol3"}, + {{60_mm, 0, 300_mm}, 30_mm, 100_mm, 500_mm, "vol4"} +}; +} // namespace
685-697: Additional test cases, consider we should.Strong in testing, this code is. Yet, edge cases for volume registration, we could add:
- Test registration with invalid volume bounds
- Test registration with overlapping volumes
- Test registration with empty shell
Generate additional test cases, shall I?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Core/src/Geometry/CuboidVolumeBounds.cpp
- Core/include/Acts/Geometry/CuboidPortalShell.hpp
- Core/src/Geometry/CuboidPortalShell.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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 (3)
- GitHub Check: merge-sentinel
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu
🔇 Additional comments (2)
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (1)
172-172: Approve this change, I do.More descriptive the new name is, clearer its purpose becomes. Wise decision, this renaming was.
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
194-194: False positive from Cppcheck, this warning is.The BOOST_AUTO_TEST_SUITE macro, valid it is. From Boost.Test framework it comes. Ignore this warning, we shall.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 194-194: 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: 1
🧹 Nitpick comments (5)
Core/include/Acts/Geometry/CuboidPortalShell.hpp (2)
61-98: Const correctness improve, we must!Make the volume pointer const, we should, as it should not be modified after construction.
- TrackingVolume* m_volume; + const TrackingVolume* m_volume;
100-153: Document ownership semantics, we must!Clarify in documentation whether the class takes ownership of the shells or expects them to be managed externally, we should.
Core/src/Geometry/CuboidPortalShell.cpp (2)
29-38: Optimize iteration, we can!Use std::ranges::for_each for cleaner and potentially more efficient iteration, we should.
- for (Face face : {NegativeXYPlane, PositiveXYPlane, NegativeYZPlane, - PositiveYZPlane, NegativeZXPlane, PositiveZXPlane}) { - const auto& portalAtFace = portalPtr(face); - if (portalAtFace != nullptr) { - portalAtFace->fill(volume); - volume.addPortal(portalAtFace); - } - } + std::array faces = {NegativeXYPlane, PositiveXYPlane, NegativeYZPlane, + PositiveYZPlane, NegativeZXPlane, PositiveZXPlane}; + std::ranges::for_each(faces, [&](Face face) { + if (const auto& portalAtFace = portalPtr(face)) { + portalAtFace->fill(volume); + volume.addPortal(portalAtFace); + } + });
137-150: Improve error messages, we should!More descriptive error messages, provide we must. Include details about what made the shell invalid.
- throw std::invalid_argument("Invalid shell pointer"); + throw std::invalid_argument("Null shell pointer found in stack construction"); - throw std::invalid_argument("Invalid shell"); + throw std::invalid_argument("One or more shells in stack are invalid");Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
53-137: More edge cases test, we should!Add test cases for:
- Invalid shell configurations
- Zero-sized volumes
- Extreme coordinate values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: 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: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: unused_files
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- 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: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (2)
Core/include/Acts/Geometry/CuboidPortalShell.hpp (1)
24-53: Well designed, this base class is!Clear interface with pure virtual methods for portal management, it provides. SOLID principles, it follows.
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (1)
38-49: Well designed, these helper functions are!Clean and reusable implementation, they have. Good practices, they follow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Core/src/Geometry/CuboidPortalShell.cpp (1)
115-229: 🛠️ Refactor suggestionLong constructor, this is! Split into smaller functions, we must!
Too many responsibilities, this constructor has. Break it down into focused methods, we should:
- validateShells()
- initializeDirection()
- setupFaces()
- mergeSidePortals()
- fuseConnectingPortals()
Easier to understand and maintain, it will be.
🧹 Nitpick comments (3)
Core/src/Geometry/CuboidPortalShell.cpp (3)
40-64: Hmm, robust the constructor is, but safer error handling, we need!Protect against dynamic_cast failure explicitly, we should. Though type check exists, defensive programming, a Jedi's way it is.
const auto& bounds = - dynamic_cast<const CuboidVolumeBounds&>(m_volume->volumeBounds()); + dynamic_cast<const CuboidVolumeBounds*>(&m_volume->volumeBounds()); +if (bounds == nullptr) { + throw std::runtime_error("Failed to cast volume bounds to CuboidVolumeBounds"); +} std::vector<OrientedSurface> orientedSurfaces = - bounds.orientedSurfaces(m_volume->transform()); + bounds->orientedSurfaces(m_volume->transform());
103-107: Remove the unnecessary semicolon after function definition, we must!A disturbance in the Force, this extra semicolon creates.
bool SingleCuboidPortalShell::isValid() const { return std::ranges::all_of(m_portals, [](const auto& portal) { return portal == nullptr || portal->isValid(); }); -}; +}
231-233: Magic number, this is! Define a constant, we should!Hardcoded value 6, mysterious it is. A named constant or computed value, more clarity would bring.
+static constexpr std::size_t NUM_FACES = 6; + std::size_t CuboidStackPortalShell::size() const { - return 6; + return NUM_FACES; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (2)
Core/src/Geometry/CuboidPortalShell.cpp (2)
29-38: Good, this implementation is! Approve it, I do!Clean and efficient, the portal filling logic is. Proper null checks, it has.
285-303: Well implemented, this stream operator is!Clean and complete, the implementation is. All cases, it handles wisely.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Core/src/Geometry/CuboidPortalShell.cpp (1)
115-231: 🛠️ Refactor suggestionToo long, this constructor is! Split it into smaller functions, we must!
Break down the constructor into smaller, focused methods:
- initializeDirections()
- validateShells()
- setupFaces()
- mergePortals()
- fusePortals()
Easier to understand and maintain, it will be.
🧹 Nitpick comments (1)
Core/src/Geometry/CuboidPortalShell.cpp (1)
233-235: Magic number, this is! Named constant or calculation, we need!Replace hardcoded value with a named constant or calculate from m_shells size, you should.
+ static constexpr std::size_t NUM_FACES = 6; std::size_t CuboidStackPortalShell::size() const { - return 6; + return NUM_FACES; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)
🔇 Additional comments (2)
Core/src/Geometry/CuboidPortalShell.cpp (2)
29-38: Approve this implementation, I do!Well-structured and safe, this portal filling method is. Null checks, it performs wisely.
287-305: Well implemented, this stream operator is!All cases it handles, and modern C++20 features it uses wisely.
Adding
CuboidPortalShellclasses for Gen3 planar geometry support.The
CuboidStackPortalShellclass is able to handle arbitrary direction stacking, analogously to theCuboidVolumeStack.Splitting the
PortalShellfile into separatePortalShellBase,CylinderPortalShell,CuboidPortalShell.Adding
facesFromAxisDirectionconvenience method toCuboidVolumeBounds.Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Changes
fromAxisDirection→boundsFromAxisDirection).Testing