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

Implement bounding volume intersections #11439

Merged
merged 7 commits into from Jan 22, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Jan 20, 2024

Objective

#10946 added bounding volume types and an IntersectsVolume trait, but didn't actually implement intersections between bounding volumes.

This PR implements AABB-AABB, circle-circle / sphere-sphere, and AABB-circle / AABB-sphere intersections.

Solution

Implement IntersectsVolume for bounding volume pairs. I also added closest_point methods to return the closest point on the surface / inside of bounding volumes. This is used for AABB-circle / AABB-sphere intersections.

@Jondolf Jondolf added C-Enhancement A new feature A-Math Fundamental domain-agnostic mathematical operations labels Jan 20, 2024
@Jondolf Jondolf added this to the 0.13 milestone Jan 20, 2024
Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

For consistency I'd change the < to <=, but other than that I haven't noticed any problems.

crates/bevy_math/src/bounding/bounded2d/mod.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/bounded3d/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@NiseVoid NiseVoid left a comment

Choose a reason for hiding this comment

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

This seems correct. Didn't check the tests tho. And closest_point on sphere/circle appears to be unused. It might be useful, but I also feel like maybe it should be on the primitive types instead.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 20, 2024

For now, I moved the circle/sphere closest_points logic to the primitives (same for AABBs), but kept the methods on the bounding volumes too for consistency and convenience. I could also just remove the circle/sphere version since it's unused, or split it into a different PR, but I thought it'd be nice to have if AABBs have it anyway

Copy link
Contributor

@IQuick143 IQuick143 left a comment

Choose a reason for hiding this comment

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

Seems good.

Although I'm not super convinced about implementing closest point on the primitives, because closest point seems to demand transform information (translation + rotation), which is addressed here by assuming default identity transform.
It's not a huge deal, because the user can use this to implement the general case simply by inversely transforming the point and then transforming the result.

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 20, 2024

FWIW, Parry has "global" and local versions of a ton of methods, like aabb/local_aabb, project_point/project_local_point, support_point/local_support_point, and so on. Both can be useful.

The closest_point methods on the primitives currently correspond the most to project_local_point, but with the shape always treated as "solid" (the returned point can be inside the shape instead of always being at the perimeter/surface). In the future we might want a trait for this and to implement it for all primitives

@Jondolf
Copy link
Contributor Author

Jondolf commented Jan 20, 2024

Here's a video of the bounding volume intersections working: (gray is the bounding volume)

2024-01-20.23-07-29.mp4

I can add this as an example if that'd be useful. Should I do it in this PR or a follow-up?

@NiseVoid
Copy link
Contributor

NiseVoid commented Jan 22, 2024

Should I do it in this PR or a follow-up?

I guess a follow up PR that showcases the intersection tests (including raycasting and later sphere/aabb casting) and bounding types makes sense?

Copy link
Member

@alice-i-cecile alice-i-cecile 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. Great docs, and thanks for including thorough tests.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 22, 2024
Merged via the queue into bevyengine:main with commit 6a3b059 Jan 22, 2024
23 checks passed
@Jondolf Jondolf deleted the bounding-volume-intersections branch January 22, 2024 18:21
@Jondolf Jondolf mentioned this pull request Feb 4, 2024
47 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants