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 RayTest2d and RayTest3d #11310

Merged
merged 13 commits into from
Jan 28, 2024
Merged

Conversation

NiseVoid
Copy link
Contributor

Objective

Implement a raycast intersection test for bounding volumes

Solution

  • Implement RayTest2d and RayTest3d types

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Jan 12, 2024
Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

This is more of a physics thing, and Bevy doesn't have a physics library yet. So I don't really think it's the right time to add something like this.

@NiseVoid
Copy link
Contributor Author

NiseVoid commented Jan 12, 2024

This is more of a physics thing, and Bevy doesn't have a physics library yet. So I don't really think it's the right time to add something like this.

I can understand the confusion, but intersection tests aren't really specific to physics. Some examples would be raycasting to find the entity under your cursor, or a raytracing renderer. Other intersection tests can be even more common, like intersections between bounding volumes which can be used to find all nearby entities for enemy AI or a visibility system.

crates/bevy_math/src/bounding/raytest3d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Show resolved Hide resolved
@NiseVoid
Copy link
Contributor Author

NiseVoid commented Jan 27, 2024

out.mp4

Made a modified version of Jondolf's intersection test example to test it, it seems to work now, even in some of the edge cases (like things being behind the ray origin, or the ray origin being inside the volume).

@NiseVoid NiseVoid marked this pull request as ready for review January 27, 2024 13:48
@IQuick143
Copy link
Contributor

It's weird that on the bottom AABB in the video, the hit seems to occasionally produce slightly wrong results (they're not on the surface of the AABB). All the other collisions seem correct and I couldn't find a mistake in the code.
Maybe there is just a frame delay? Probably not a big deal, just noticed it's odd.

crates/bevy_math/src/bounding/raytest2d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest2d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest2d.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest3d.rs Outdated Show resolved Hide resolved
@Jondolf
Copy link
Contributor

Jondolf commented Jan 27, 2024

Noticed the same as @IQuick143, but for the middle bounding circles, at around 13 seconds. It seems to be wrong for multiple frames

kuva

@NiseVoid
Copy link
Contributor Author

Noticed the same as @IQuick143, but for the middle bounding circles, at around 13 seconds. It seems to be wrong for multiple frames

In this case the points are not actually wrong, the left one touches the surface of the left sphere, the right one is inside the right sphere so it stays at the origin.

There is however some weirdness going on with the bottom AABB, which I think might just be an error in the example (the code isn't written in such a way that each volume is handled by the exact same code)

let offset = self.ray.origin - sphere.center;
let projected = offset.dot(*self.ray.direction);
let closest_point = offset - projected * *self.ray.direction;
let distance_squared = sphere.radius().powi(2) - closest_point.length_squared();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this value is actually a squared distance, though I can't figure out a better name for it.

Copy link
Contributor Author

@NiseVoid NiseVoid Jan 27, 2024

Choose a reason for hiding this comment

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

Yea, distance is a bit of a weird term ... It's the distance from the outside surface to the projected point. It's like a reversed signed distance 🙃

crates/bevy_math/src/bounding/raytest2d.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Jondolf Jondolf 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 to me now!

@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 28, 2024
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.

I like the code, but this definitely needs a test suite before I'm happy with merging it. Problems in this type of code are really nasty to detect and isolate downstream otherwise.

@alice-i-cecile alice-i-cecile removed 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 28, 2024
@IQuick143
Copy link
Contributor

I guess for tests we could do

  1. a set of [BoundingVolume, ray_origin, ray_direction] which should intersect, and then test:
    A) an intersection happened
    B) the computed point works out to lie on the surface of the volume
    C) the same intersection test with a negated ray direction fails
    [D) try to move the origin onto the other side of the volume and test a ray going the other way, and see that the two intersection points lie closer to their respective ray origins than the other one (this should test that the ray did in fact hit the front of the volume rather than the back)]
  2. a set of cases where the ray_origin is within the volume which should intersect and return 0 as the intersection time
  3. a set of cases that should miss (idk how specifically)

I think that would effectively cover most cases.

Idk if point D) is a bit too much, if @NiseVoid wants, I could try implementing the tests myself.

@NiseVoid
Copy link
Contributor Author

I'll add some tests as described by @IQuick143, that seems like a fairly decent set of cases. Adding an example in a later later PR for intersection tests would probably also help with the debugging process if users hit any weird issues

crates/bevy_math/src/bounding/raytest3d.rs Show resolved Hide resolved
crates/bevy_math/src/bounding/raytest2d.rs Outdated Show resolved Hide resolved
Co-authored-by: IQuick 143 <[email protected]>
@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 28, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 28, 2024
Merged via the queue into bevyengine:main with commit 8851532 Jan 28, 2024
22 checks passed
@Jondolf Jondolf mentioned this pull request Feb 4, 2024
46 tasks
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective

Implement a raycast intersection test for bounding volumes

## Solution

- Implement RayTest2d and RayTest3d types

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: IQuick 143 <[email protected]>
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-Feature A new feature, making something new possible 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.

5 participants