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

fix: Return nullptr if outside tracking geometry in TrackingGeometry::lowestTrackingVolume #3481

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 6, 2024

Currently TrackingGeometry::lowestTrackingVolume will always return at least the world volume even though the given position is not inside the world volume. In this PR I change the behavior to return nullptr in case we are outside the tracking geometry.

Same is done for TrackingVolume::lowestTrackingVolume. The rest are necessary fixes and downstream changes to make this work.

blocked by

@andiwand andiwand added this to the next milestone Aug 6, 2024
@andiwand andiwand requested a review from AJPfleger August 6, 2024 09:03
@github-actions github-actions bot added the Component - Core Affects the Core module label Aug 6, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Aug 6, 2024

this also fails the gx2f unit tests 🙃

09:08:42    Navigator      VERBOSE   No Volume | Initialization.
09:08:42    Navigator      VERBOSE   No Volume | Current surface set to start surface undefined
09:08:42    Navigator      VERBOSE   No Volume | Slow start initialization through search.
09:08:42    Navigator      VERBOSE   No Volume | Starting from position (750.0221, -749.9779, -0.1743) and direction (-0.2816, 0.9595, 0.0001)
unknown location(0): fatal error: in "Gx2fTest/UpdatePushedToNewVolume": memory access violation at address: 0x1a8: no mapping at fault address

I guess we are pushing out of the world volume @AJPfleger

Copy link

github-actions bot commented Aug 6, 2024

📊: Physics performance monitoring for 5dd2516

Full contents

physmon summary

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Aug 6, 2024
@andiwand andiwand marked this pull request as ready for review August 6, 2024 16:14
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Aug 6, 2024
@andiwand andiwand requested a review from AJPfleger August 6, 2024 16:34
AJPfleger
AJPfleger previously approved these changes Aug 6, 2024
@acts-project-service acts-project-service added Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Aug 6, 2024
@andiwand andiwand marked this pull request as draft August 6, 2024 20:14
@andiwand
Copy link
Contributor Author

andiwand commented Aug 6, 2024

Looks like Athena is also relying on the faulty behavior

@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Aug 7, 2024
@andiwand andiwand marked this pull request as draft August 30, 2024 08:45
kodiakhq bot pushed a commit that referenced this pull request Aug 31, 2024
This does not play nice with #3481 and I think the initial assumption changed. As long as the start surface is not part of the tracking geometry the navigator will decide on the volume by looking up through the tracking geometry. Change of volume would only be a problem if we start on a surface from the tracking geometry outside its bounds.
@andiwand andiwand removed the Fails Athena tests This PR causes a failure in the Athena tests label Aug 31, 2024
@andiwand andiwand marked this pull request as ready for review August 31, 2024 07:40
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 31, 2024
@kodiakhq kodiakhq bot merged commit c962d5a into acts-project:main Aug 31, 2024
41 checks passed
Copy link

sonarcloud bot commented Aug 31, 2024

@andiwand andiwand deleted the fix-lowestTrackingVolume-allow-nullptr branch August 31, 2024 22:35
kodiakhq bot pushed a commit that referenced this pull request Sep 1, 2024
…3442)

I think this is one of the big navigation pitfalls. If we end up in the wrong volume the navigation is practically lost. This is too expensive to check in production so I opted for an `assert`.

Not sure if that is a great solution since users might try to measure performance with non release build by accident. But that was the only way I could think of.

blocked by
- #3521
- #3481
@andiwand andiwand modified the milestones: next, v36.3.0 Sep 2, 2024
paulgessinger added a commit that referenced this pull request Sep 19, 2024
…ingGeometry::lowestTrackingVolume` (#3481)"

This reverts commit c962d5a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants