feat: Adding planar portal link #4044
Conversation
WalkthroughChanges to geometry and grid portal linking, we have! Modifications to handle Changes
Sequence DiagramsequenceDiagram
participant GridPortalLink
participant PlaneSurface
participant RectangleBounds
GridPortalLink->>PlaneSurface: checkConsistency()
PlaneSurface->>RectangleBounds: validate bounds
GridPortalLink->>GridPortalLink: extendTo2dImpl
GridPortalLink->>PlaneSurface: retrieve surface properties
GridPortalLink-->>GridPortalLink: create grid
Possibly related PRs
Suggested Labels
Suggested Reviewers
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: 0
🧹 Nitpick comments (1)
Tests/UnitTests/Core/Geometry/PortalLinkTests.cpp (1)
2374-2378: Redundant debug output calls, I detect.Multiple calls to
printContents()you have. In test code, these should be guarded or removed unless needed for debugging.Consider this change:
- grid1X->printContents(std::cout); - grid2->printContents(std::cout); - grid3->printContents(std::cout); - mergedX->printContents(std::cout); - mergedY->printContents(std::cout); + #ifdef ACTS_VERBOSE_DEBUG + grid1X->printContents(std::cout); + grid2->printContents(std::cout); + grid3->printContents(std::cout); + mergedX->printContents(std::cout); + mergedY->printContents(std::cout); + #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Core/include/Acts/Geometry/GridPortalLink.hpp(8 hunks)Core/src/Geometry/CompositePortalLink.cpp(3 hunks)Core/src/Geometry/GridPortalLink.cpp(4 hunks)Core/src/Geometry/GridPortalLinkMerging.cpp(3 hunks)Core/src/Geometry/PortalLinkBase.cpp(2 hunks)Tests/UnitTests/Core/Geometry/PortalLinkTests.cpp(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: merge-sentinel
- GitHub Check: linux_physmon
- GitHub Check: linux_examples_test
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: build_debug
🔇 Additional comments (22)
Core/src/Geometry/CompositePortalLink.cpp (3)
18-18: Necessary includes added, appropriate they are.Includes for
RectangleBounds,AxisDefinitions, and<cstddef>added you have. Essential for PlaneSurface handling, they are.Also applies to: 21-21, 24-24
54-56: Merging logic for PlaneSurfaces, correctly implemented it is.In the
mergedSurfacefunction, support for merging PlaneSurfaces, added you have. Properly handling the merge, the code is.
297-345: Support for PlaneSurface in grid creation, wisely added you have.Implementation of grid creation for PlaneSurfaces, this adds. Checks for binning direction and edge calculations, correctly included they are. Enhance functionality, this does.
Core/src/Geometry/GridPortalLink.cpp (4)
13-14: Additional includes for PlaneSurface support, added they are.Includes for
RectangleBounds,AxisDefinitions, and<iostream>necessary they are. Ensure proper functionality, they will.Also applies to: 16-16
72-84: Handling of PlaneSurface inmakemethod, appropriate it is.Logic for PlaneSurface in
GridPortalLink::make, added you have. Validating binning directions and setting up axes, correctly you are.
218-250: Consistency check for PlaneSurface, well implemented it is.Method
checkConsistencyfor PlaneSurface, added you have. Ensures grid matches bounds of PlaneSurface, this does. Important for correctness, it is.
469-505: Extension to 2D grid for PlaneSurface, correctly added it is.In
extendTo2dImpl, handling of PlaneSurface to extend to 2D grid, implemented you have. Logic for axes and grid filling, proper it is.Core/src/Geometry/PortalLinkBase.cpp (2)
17-17: Include forRectangleBounds, necessary it is.For PlaneSurface support, required this include is.
60-71: Preconditions for merging PlaneSurfaces, added you have.In
checkMergePreconditions, support for PlaneSurfaces, included it is. Assertions for surface types and bounds, correctly added they are.Core/src/Geometry/GridPortalLinkMerging.cpp (3)
260-261: Hmm, good implementation this is!Consistent with other surface types, the merging of plane surfaces is. Follow the established pattern, it does.
459-462: Wise addition to the merging logic, this is!Handle plane surfaces with proper axes, it does. Consistent with the surface's coordinate system, it remains.
446-446: Hmm, proper type checking, added it has been!Follow the established pattern of surface type detection, it does.
Core/include/Acts/Geometry/GridPortalLink.hpp (6)
70-72: Strong with validation, this code is!Check the binning direction for plane surfaces, it does. Throw proper exceptions when invalid directions are used, it will.
361-364: Hmm, consistency check for plane surfaces, declared it has been!Follow the ways of other surface types, it does. Essential for grid configuration validation, this is.
382-389: Well documented and consistent, this implementation is!Follow the path of other surface types in grid extension, it does.
471-481: Strong with the Force, this constructor implementation is!Handle plane surfaces with proper projections and error checks, it does. Follow the ways of other surface types, it does.
521-523: Hmm, wise addition to grid extension, this is!Handle plane surfaces in dimension extension, it does. Follow the established patterns, it does.
573-574: Improved error handling, this change brings!Use exceptions instead of assertions, it now does. More robust at runtime, this makes the code.
Tests/UnitTests/Core/Geometry/PortalLinkTests.cpp (4)
470-575: Comprehensive test coverage for 1D plane surface merging, I see!Well structured test cases for merging plane surfaces in both X and Y directions, these are. Good validation of axis bounds and bin counts, you have included.
2447-2590: Thorough bin filling tests for plane surfaces, you have written!Clever use of checkerboard pattern to verify correct bin assignments and volume resolution. The force is strong with these tests.
Line range hint
2844-3115: Well-crafted portal merging tests for plane surfaces, these are!Good coverage of both X and Y directions with proper volume transformations and boundary checks. Strong test cases for the portal linking system, you have created.
2366-2372:⚠️ Potential issueA bug in the test assertions, I sense.
The test checks
mergedYbut usesmergedPtrXto get the pointer. Incorrect this is!Apply this fix, you must:
- const auto* mergedY = dynamic_cast<const merged_type*>(mergedPtrX.get()); + const auto* mergedY = dynamic_cast<const merged_type*>(mergedPtrY.get());Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/src/Geometry/CompositePortalLink.cpp (1)
305-345: Additional validation checks, we should consider.Hmm, missing some checks, this implementation is:
- Validate consistent surface orientations, we must
- Check for overlapping bounds between adjacent surfaces, we should
- Verify alignment of surfaces along the binning direction, important it is
Add these checks before edge calculation, you should:
bool dirX = m_direction == AxisDirection::AxisX; +// Validate surface orientations and alignment +const auto& refTransform = trivialLinks[0]->surface().transform(gctx); +const Vector3 refNormal = refTransform.rotation().col(2); +for (const auto* link : trivialLinks) { + const auto& transform = link->surface().transform(gctx); + const Vector3 normal = transform.rotation().col(2); + if (!normal.isApprox(refNormal)) { + throw std::runtime_error{"Inconsistent surface orientations in plane grid"}; + } +} + std::vector<double> edges;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/src/Geometry/CompositePortalLink.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unused_files
- GitHub Check: missing_includes
- 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: build_debug
🔇 Additional comments (2)
Core/src/Geometry/CompositePortalLink.cpp (2)
18-18: Approve these changes, I do!Wisely added, the new includes are. Elegant and consistent with existing patterns, the PlaneSurface merging implementation is. The Force is strong with this code!
Also applies to: 21-21, 54-56
297-303: Wise direction validation, you have implemented!Clear and precise, the error handling for unsupported binning directions is. Strong boundaries, a Jedi must maintain!
|
acts-project-service
added
the
Breaks Athena build
Extending the
PortalLinkBase,GridPortalLink, andCompositePortalLinkclasses to supportPlaneSurface.Summary by CodeRabbit
New Features
PlaneSurfacein grid portal link functionality.Bug Fixes
Tests