refactor: ProtoLayer can (optionally) hold mutable surfaces
#4030
Conversation
In the Gen3 building context, using mutable pointers makes more sense since we store as much as possible internally as mutable. This is implemented in a API backwards compatible way, because the Gen1 building assumes the opposite.
WalkthroughIn the realm of geometry, a transformation most profound has occurred! The Changes
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: 2
🧹 Nitpick comments (7)
Core/src/Geometry/ProtoLayer.cpp (3)
25-27: Clarify the comment, we should.The comment "To prevent problematic isInsidePolygon check for straw surfaces with only one lseg" may benefit from rephrasing for clarity. Consider explaining why
lsegis set to2forStrawsurfaces.
46-64: Consistent use of parameter names, important it is.The parameter
addenvis misspelled in comments as "enevlope." Correct the spelling for consistency and readability.- /// @param addenv The steering if enevlope is added or not + /// @param addenv The steering if envelope is added or not
68-70: Avoid unnecessary copies, we should.Passing
Extent& extentby non-const reference is acceptable, but ensure no unnecessary copies occur when using it.Core/include/Acts/Geometry/ProtoLayer.hpp (3)
28-29: Plan for deprecation, we must.The comment suggests that
ProtoLayerBasewill go away once Gen1 geometry is removed. Ensure there is a clear plan and timeline for this deprecation to avoid future technical debt.
40-63: Enhance documentation, beneficial it would be.Provide more detailed descriptions for the methods
min,max,medium, andrange. Explain the purpose and usage of theaddenvparameter in greater detail.
95-110: Simplify the constructor logic, we can.The conditional compilation using
if constexprimproves efficiency. However, consider unifying the logic to reduce code duplication.Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp (1)
81-88: Typedef for clarity, consider we should.Improving readability by creating a type alias for the complex
std::shared_ptrtype may help.using SharedSurfacePtr = std::shared_ptr<std::conditional_t<IsConst, const Surface, Surface>>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Geometry/ProtoLayer.hpp(4 hunks)Core/src/Geometry/ProtoLayer.cpp(1 hunks)Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: build_debug
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (3)
Core/src/Geometry/ProtoLayer.cpp (1)
21-41: Proper encapsulation ofmeasureImpl, you have achieved.The refactoring of
measureintomeasureImplenhances modularity and reusability. Well done, it is.Tests/UnitTests/Core/Geometry/ProtoLayerTests.cpp (2)
39-42: Commendable use of templates, it is.Templating
testProtoLayerallows testing of both const and non-const versions efficiently. Excellent approach, you have employed.
168-174: Ensure both test cases cover all scenarios, we must.While you test both const and non-const versions, verify that all code paths and edge cases are thoroughly tested in both.
| for (const auto& dT : {-0.5 * thickness, 0.5 * thickness}) { | ||
| Transform3 dtransform = transform * Translation3{dT * sfNormal}; | ||
| extent.extend(sfPolyhedron.extent(dtransform)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure thread safety, we must.
If this code is executed in a multi-threaded context, modifying extent without proper synchronization may lead to data races.
Propose adding synchronization mechanisms if necessary.
+ // Ensure thread safety
+ std::lock_guard<std::mutex> lock(extentMutex);
extent.extend(sfPolyhedron.extent(dtransform));Committable suggestion skipped: line range outside the PR's diff.
| void add(const GeometryContext& gctx, SurfaceType& surface) { | ||
| m_surfaces.push_back(&surface); | ||
| std::vector<const Surface*> constSurfaces; | ||
| if constexpr (!IsConst) { | ||
| constSurfaces.reserve(m_surfaces.size()); | ||
| for (auto* sf : m_surfaces) { | ||
| constSurfaces.push_back(sf); | ||
| } | ||
| measureImpl(gctx, constSurfaces, extent, transform); | ||
| } else { | ||
| measureImpl(gctx, m_surfaces, extent, transform); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reevaluate the need for multiple measureImpl calls, we should.
Each time add is called, measureImpl recalculates the entire extent. For performance optimization, consider updating the extent incrementally or deferring the full recalculation until necessary.
[performance]
|
In the Gen3 building context, using mutable pointers makes more sense since we store as much as possible internally as mutable.
This is implemented in a API backwards compatible way, because the Gen1 building assumes the opposite.
PLEASE DESCRIBE YOUR CHANGES.
THIS MESSAGE ENDS UP AS THE COMMIT MESSAGE.
DO NOT USE @-MENTIONS HERE!
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Refactor
ProtoLayerclass to improve modularity and flexibilityNew Features
Tests
ProtoLayerscenarios