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

Add generic traversal #128

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Conversation

finnbear
Copy link
Collaborator

@finnbear finnbear commented Jan 7, 2025

Changes

  • Make traversal (normal and iterator, but not distance-based) generic (breaking change)
    • Normal BVH
    • Flat BVH
    • BoundingHierarchy trait
    • Traverse BVH by ray (existing feature maintained)
    • Traverse BVH by point
    • Traverse BVH by AABB
    • Traverse BVH by ball (2d = circle, 3d = sphere)
  • Fuzz
  • Test
    • Ball::contains
    • Aabb::intersects_aabb
  • Documentation

Questions

  • Would IntersectsAabb be a better name for AabbIntersection?
  • Is Query a good name for the IntersectsAabb generic?

Related

Fixes #42
Fixes #45
Supersedes #43 (credit to @medakk for the central idea of this PR)
Related #71

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 44.79167% with 53 lines in your changes missing coverage. Please review.

Project coverage is 55.18%. Comparing base (e7a594d) to head (fe49a04).

Files with missing lines Patch % Lines
fuzz/fuzz_targets/fuzz.rs 0.00% 52 Missing ⚠️
src/bounding_hierarchy.rs 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   55.60%   55.18%   -0.43%     
==========================================
  Files          14       16       +2     
  Lines        1266     1321      +55     
==========================================
+ Hits          704      729      +25     
- Misses        562      592      +30     

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

@finnbear finnbear marked this pull request as ready for review January 7, 2025 23:46
@svenstaro
Copy link
Owner

Could you attempt to rebase this in such a way that every commit is useful in the history? By that I mean: If you make a commit A and a commit B that fixes A, you should rebase B onto A so that B will not even show up in the history anymore.

Ideally, the commits tell a story so that when I go through your PR commit by commit, I will be able to re-assemble your development/thought process on my end.

@finnbear

This comment was marked as resolved.

@svenstaro
Copy link
Owner

By the way, I really like using git-absorb for this.

@svenstaro
Copy link
Owner

Hm the commits don't look properly rebased. There's a merge commit in there and some other stuff.

@svenstaro
Copy link
Owner

Please also write a CHANGELOG entry for this with a small section on how to migrate this for users and mark it as a breaking change there.

@finnbear

This comment was marked as resolved.

finnbear added a commit to finnbear/bvh that referenced this pull request Jan 8, 2025
@svenstaro
Copy link
Owner

Yup, a minute or two ago, I noticed that and did a second rebase. Can you check again?

Well, there's still a merge commit.

@finnbear

This comment was marked as resolved.

finnbear added a commit to finnbear/bvh that referenced this pull request Jan 8, 2025
@svenstaro
Copy link
Owner

You can still rebase the conflicting commit in your branch so that it will apply cleanly. :)

@finnbear

This comment was marked as resolved.

@svenstaro
Copy link
Owner

svenstaro commented Jan 8, 2025

Ok. Make sure your local master is in sync with the upstream master. Then move to your branch and git rebase master. Solve the conflicts. Your branch should then look like as if it had been created after the latest changes on master which means there's nothing to merge.

It takes a bit of getting used to but in the end it will result in a very clean style of committing. :)

EDIT: Try this: https://simondosda.github.io/posts/2022-01-03-git-rebase-workflow.html

Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

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

Some small things for now, bigger review after commits look right.

src/aabb/aabb_impl.rs Outdated Show resolved Hide resolved
src/ball.rs Outdated Show resolved Hide resolved
src/ball.rs Show resolved Hide resolved
src/ball.rs Outdated Show resolved Hide resolved
src/ball.rs Outdated Show resolved Hide resolved
finnbear added a commit to finnbear/bvh that referenced this pull request Jan 8, 2025
finnbear added a commit to finnbear/bvh that referenced this pull request Jan 8, 2025
@finnbear
Copy link
Collaborator Author

finnbear commented Jan 8, 2025

Then move to your branch and git rebase master. Solve the conflicts.

Thanks for the hints! git rebase seems extremely powerful and I'm glad I now have some understanding of it.

finnbear added a commit to finnbear/bvh that referenced this pull request Jan 8, 2025
@svenstaro
Copy link
Owner

Ideally stuff such as 27b51d7 wouldn't even exist because it essentially just revises another commit which is about to be merged as part of this PR. As such, ideally those new changes to older commits that aren't yet merged would just be manipulated in such a way that they appear to always have been part of the original changeset. This can pretty easily be done using either git rebase --interactive or git add --interactive and then manually choosing which hunks to stage or by using a more automated tool such as git-absorb which will for the most part figure out which commits to put things into.

finnbear added a commit to finnbear/bvh that referenced this pull request Jan 9, 2025
@finnbear
Copy link
Collaborator Author

finnbear commented Jan 9, 2025

Downloaded and ran git absorb on the revisions, rebased, and it looks plausibly correct to me

Some CI checks were cancelled for no apparent reason 👀
image

@svenstaro
Copy link
Owner

You have a commit that renames AabbIntersection to IntersectsAabb but it would be better if you'd rebase it in such a way that it always looks like it was called IntersectsAabb to begin with. :)

finnbear added a commit to finnbear/bvh that referenced this pull request Jan 9, 2025
@finnbear
Copy link
Collaborator Author

finnbear commented Jan 9, 2025

@svenstaro noted, I rebased again. Also figured out out to reorder commits, so the Ball doc improvement is now squashed, too.

Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

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

We're almost there!

src/testbase.rs Outdated Show resolved Hide resolved
src/testbase.rs Outdated Show resolved Hide resolved
src/testbase.rs Outdated Show resolved Hide resolved
src/testbase.rs Outdated Show resolved Hide resolved
src/ball.rs Outdated Show resolved Hide resolved
fuzz/fuzz_targets/fuzz.rs Outdated Show resolved Hide resolved
finnbear added a commit to finnbear/bvh that referenced this pull request Jan 9, 2025
@finnbear
Copy link
Collaborator Author

finnbear commented Jan 9, 2025

Thanks for the review; addressed all comments and absorbed :)

@svenstaro svenstaro merged commit efb3071 into svenstaro:master Jan 9, 2025
11 of 12 checks passed
@svenstaro
Copy link
Owner

Amazing job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Other intersection structures (plane, sphere) More general queries
3 participants