Skip to content
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: adapt visitor concept #2901

Merged
merged 12 commits into from
Jan 29, 2024

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Jan 26, 2024

This PR enhances the visitor pattern for the Acts geometry.

It adds a non-const visitor pattern which will allow to also use the visitor pattern to set the material to the new detector geometry and will replace this IMaterialProvider mechanism and will rely on the same recursive visiting.

The unit tests show how this can be used to assign material to a pre-built geometry instance.

Note: The new visitSurfaces method of the TrackingGeometry will now visit all reachable surfaces, and not only the sensitive ones. This allows it to e.g. extract material surfaces and will remove a lot of duplicated code in SurfaceMaterialMapper and VolumeMaterialMapper that manually walk through the TrackingGeometry hierarchy.

@asalzburger asalzburger added this to the next milestone Jan 26, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Jan 26, 2024
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (7d596d1) 48.92% compared to head (42fcb7f) 48.93%.
Report is 1 commits behind head on main.

❗ Current head 42fcb7f differs from pull request most recent head ab6dd0d. Consider uploading reports for the commit ab6dd0d to get more accurate results

Files Patch % Lines
Core/include/Acts/Detector/DetectorVolume.hpp 42.10% 1 Missing and 10 partials ⚠️
Core/include/Acts/Geometry/TrackingVolume.hpp 52.63% 0 Missing and 9 partials ⚠️
Core/include/Acts/Detector/Detector.hpp 50.00% 0 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
+ Coverage   48.92%   48.93%   +0.01%     
==========================================
  Files         496      496              
  Lines       28849    28899      +50     
  Branches    13677    13705      +28     
==========================================
+ Hits        14113    14142      +29     
+ Misses       4874     4872       -2     
- Partials     9862     9885      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulgessinger
Copy link
Member

If this is breaking we need to hold this until v33.

@paulgessinger paulgessinger modified the milestones: next, v33.0.0 Jan 26, 2024
@paulgessinger
Copy link
Member

I'm happy with this, but we'll really have to hold this until the next major version I think.

@asalzburger
Copy link
Contributor Author

Hey @paulgessinger - I have done as you suggested, it should now not be breaking change anymore.

@asalzburger asalzburger changed the title feat!: adapt visitor concept feat: adapt visitor concept Jan 29, 2024
@kodiakhq kodiakhq bot merged commit 5166de1 into acts-project:main Jan 29, 2024
53 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Jan 29, 2024
@paulgessinger paulgessinger modified the milestones: v33.0.0, next, v32.1.0 Feb 2, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
This PR enhances the visitor pattern for the Acts geometry.

It adds a non-const visitor pattern which will allow  to also use the visitor pattern to set the material to the new detector geometry and will replace this `IMaterialProvider` mechanism and will rely on the same recursive visiting.

The unit tests show how this can be used to assign material to a pre-built geometry instance.

Note: The new `visitSurfaces` method of the `TrackingGeometry` will now visit all reachable surfaces, and not only the sensitive ones. This allows it to e.g. extract material surfaces and will remove a lot of duplicated code in `SurfaceMaterialMapper` and `VolumeMaterialMapper` that manually walk through the `TrackingGeometry` hierarchy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants