Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: GeoShapes to DetectorVolumes and GeoMaterial Converters #3268
feat: GeoShapes to DetectorVolumes and GeoMaterial Converters #3268
Changes from 34 commits
5c69706
daaf5b8
0700fc7
891280f
15612c9
b8712b4
fa55d0e
656c771
b0c1a09
1660a53
a7fb341
53334ab
6b016ca
cd12571
47547d9
c418f5b
e6c8ac0
2792e65
fe54079
e892533
140cd91
b021387
b7463da
35fbd02
7c7c3c3
8761e4a
6fdc26b
47b7235
a052cfc
1a5afbb
68d51a0
247f90c
787daf5
4a140af
f457684
d5b763a
7608689
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing includes, I sense.
Required for your journey, these includes are:
Add these includes, you must:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Limited shape handling, I sense in this code.
In GeoModelToDetectorVolume.cpp, handle only these shapes, the implementation does:
But many more shapes in the GeoModel universe, there are. A default error throwing for unsupported shapes, I see. Dangerous this path is.
🔗 Analysis chain
Verify shape handling completeness, we must.
Ensure that all GeoModel shapes, handled they are. Search the implementation, we shall.
The search continues, young padawan. More information about shape handling, we must find. The implementation of convertVolume and shape converters, reveal they will.
Hmm, elusive the implementation is. Through different paths, search we must. The Force guides us to look deeper into the GeoModel plugin's implementation files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 2378
Script:
Length of output: 443
Script:
Length of output: 9474
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor the duplicated code, you should.
Duplicate code for calculating
CuboidVolumeBounds
from shape extents, I see. A helper function, create, to improve maintainability and reduce repetition.