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: Add envelope to CuboidVolumeBuilder #2542

Merged

Conversation

andiwand
Copy link
Contributor

I noticed that the CuboidVolumeBuilder is not generating any envelope around the generated layer which results in overlapping surfaces which does not play nicely with navigation.

discovered in #2532

@andiwand andiwand added this to the next milestone Oct 12, 2023
@github-actions github-actions bot added the Component - Core Affects the Core module label Oct 12, 2023
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #2542 (053c906) into main (16e44c0) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 37.50%.

❗ Current head 053c906 differs from pull request most recent head 3cc6ac4. Consider uploading reports for the commit 3cc6ac4 to get more accurate results

@@            Coverage Diff             @@
##             main    #2542      +/-   ##
==========================================
- Coverage   49.77%   49.76%   -0.01%     
==========================================
  Files         466      466              
  Lines       26311    26318       +7     
  Branches    12093    12095       +2     
==========================================
+ Hits        13096    13097       +1     
- Misses       4616     4621       +5     
- Partials     8599     8600       +1     
Files Coverage Δ
Core/src/Geometry/CuboidVolumeBuilder.cpp 42.53% <100.00%> (+1.31%) ⬆️
Core/src/Geometry/LayerCreator.cpp 23.07% <0.00%> (-0.43%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@felix-russo felix-russo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question for my understanding

Core/src/Geometry/CuboidVolumeBuilder.cpp Show resolved Hide resolved
paulgessinger
paulgessinger previously approved these changes Oct 13, 2023
@paulgessinger
Copy link
Member

Actually, I just now remembered #1751. So shouldn't this in principle already be configurable (but not correctly configured)?

@andiwand
Copy link
Contributor Author

Actually, I just now remembered #1751. So shouldn't this in principle already be configurable (but not correctly configured)?

right it is configurable but then we default it in the volume builder which results in 0 envelope again

@paulgessinger
Copy link
Member

@andiwand Ah, and this is only if the cuboid builder auto-builds a single layer, without an explicit layer configuration.

@kodiakhq kodiakhq bot merged commit 98f36a0 into acts-project:main Oct 13, 2023
57 checks passed
@andiwand andiwand deleted the fix-cuboid-volume-builder-envelope branch October 13, 2023 12:50
@paulgessinger paulgessinger modified the milestones: next, v30.3.0 Oct 25, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants