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

Split Ray into Ray2d and Ray3d and simplify plane construction #10856

Merged
merged 14 commits into from
Dec 6, 2023

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Dec 3, 2023

Objective

A better alternative version of #10843.

Currently, Bevy has a single Ray struct for 3D. To allow better interoperability with Bevy's primitive shapes (#10572) and some third party crates (that handle e.g. spatial queries), it would be very useful to have separate versions for 2D and 3D respectively.

Solution

Separate Ray into Ray2d and Ray3d. These new structs also take advantage of the new primitives by using Direction2d/Direction3d for the direction:

pub struct Ray2d {
    pub origin: Vec2,
    pub direction: Direction2d,
}

pub struct Ray3d {
    pub origin: Vec3,
    pub direction: Direction3d,
}

and by using Plane2d/Plane3d in intersect_plane:

impl Ray2d {
    // ...
    pub fn intersect_plane(&self, plane_origin: Vec2, plane: Plane2d) -> Option<f32> {
        // ...
    }
}

Changelog

Added

  • Ray2d and Ray3d
  • Ray2d::new and Ray3d::new constructors
  • Plane2d::new and Plane3d::new constructors

Removed

  • Removed Ray in favor of Ray3d

Changed

  • direction is now a Direction2d/Direction3d instead of a vector, which provides guaranteed normalization
  • intersect_plane now takes a Plane2d/Plane3d instead of just a vector for the plane normal
  • Direction2d and Direction3d now derive Serialize and Deserialize to preserve ray (de)serialization

Migration Guide

Ray has been renamed to Ray3d.

Ray creation

Before:

Ray {
    origin: Vec3::ZERO,
    direction: Vec3::new(0.5, 0.6, 0.2).normalize(),
}

After:

// Option 1:
Ray3d {
    origin: Vec3::ZERO,
    direction: Direction3d::new(Vec3::new(0.5, 0.6, 0.2)).unwrap(),
}

// Option 2:
Ray3d::new(Vec3::ZERO, Vec3::new(0.5, 0.6, 0.2))

Plane intersections

Before:

let result = ray.intersect_plane(Vec2::X, Vec2::Y);

After:

let result = ray.intersect_plane(Vec2::X, Plane2d::new(Vec2::Y));

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Math Fundamental domain-agnostic mathematical operations M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Dec 3, 2023
@Jondolf Jondolf mentioned this pull request Dec 4, 2023
41 tasks
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 really like the split: more consistent and efficient.

I think these From impls should be cut though.

@Jondolf
Copy link
Contributor Author

Jondolf commented Dec 4, 2023

I removed the From impls from the planes, added new constructors instead, and updated the PR description. I think #10857 could be merged first though since the constructors will have to be changed a bit

@Jondolf
Copy link
Contributor Author

Jondolf commented Dec 5, 2023

I updated the PR to match the direction constructor changes. I opted to make the new constructors for rays and planes panic with an expect if the direction is invalid, since I think that's nicer for ergonomics, and you could just not use the constructor if you wanted to handle those cases manually.

If the changes look fine, everything should now be ready :)

@rlidwka
Copy link
Contributor

rlidwka commented Dec 5, 2023

How common is it to convert between Ray2 to Ray3? Do we need helper methods for that like glam's extend() and truncate()?

@Jondolf
Copy link
Contributor Author

Jondolf commented Dec 5, 2023

I don't think it's particularly common, but it'd be nice to have. It would also make sense to implement for other types, like planes, lines, line segments and even rectangle/cuboid. But extending/truncating rays would also require extending/truncating directions, which is probably out of scope for this PR.

@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 Dec 6, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 6, 2023
Merged via the queue into bevyengine:main with commit d9aac88 Dec 6, 2023
23 checks passed
@Jondolf Jondolf deleted the split-ray-dimensions branch December 6, 2023 16:08
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 A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

3 participants