separating finite and infinite 3d planes #12426

andristarr · 2024-03-11T22:07:29Z

Objective

Solution

Removing the plane3d and adding rect3d primitive mesh

NthTensor · 2024-03-17T02:07:52Z

Nice work! Need to rename the existing Rectangle primitive to Rect2d I think. Thoughts on adding Rotation2d to Rect3d to rotate the plane around it's normal? With transforms it's in for a penny in for a pound.

andristarr · 2024-03-17T20:45:16Z

So in the end the decision was to use Plane3d for a finite and InfinitePlane3d for an infinite plane, so all the confusing naming convention with the rectagnle is gone now :)

NthTensor · 2024-03-21T02:45:05Z

crates/bevy_math/src/primitives/dim3.rs

-        let normal = Dir3::new((b - a).cross(c - a))
-            .expect("plane must be defined by three finite points that don't lie on the same line");
+        let normal = Dir3::new((b - a).cross(c - a)).expect(
+            "infinite plane must be defined by three finite points that don't lie on the same line",


Use of "infinite" in error for finite plane.

Good catch, this is fixed now.

andristarr · 2024-03-22T14:13:07Z

This should be ready for final review

andristarr · 2024-03-28T18:00:11Z

I suspect this is the base of the upcoming work to have infinite plane meshes?

NthTensor · 2024-03-28T18:12:27Z

Yes, I'll try to review this soon. I've had less time for reviews lately.

NthTensor · 2024-04-07T01:25:29Z

crates/bevy_math/src/bounding/bounded3d/primitive_impls.rs

@@ -21,7 +21,7 @@ impl Bounded3d for Sphere {
    }
 }

-impl Bounded3d for Plane3d {
+impl Bounded3d for InfinitePlane3d {


This seems wrong. Why would an infinite plane have bounds?

Semantically I agree, but it can still be useful to be able to compute the AABB for collision detection purposes. You would still want planes (and half-spaces) to be included in BVHs for example. Or you could have a Shape3d trait that requires Bounded3d.

For reference, Parry's HalfSpace has AABB methods

I'll grant that, and it makes more sense looking at the actual impl. I consider this resolved.

NthTensor · 2024-04-07T01:29:15Z

crates/bevy_math/src/primitives/dim3.rs

+/// A bounded plane in 3D space. It forms a surface starting from the origin with a defined height and width.
 #[derive(Clone, Copy, Debug, PartialEq)]
 #[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))]
 pub struct Plane3d {


I would be concerned about changing the semantics of a public type like this, but this just changes it back to what it was before right?

The original Plane in bevy_render was bounded (but had no configurable normal and was always facing +Y), but the Plane3d primitive was always unbounded.

If we want to split it into a finite plane and and an infinite plane though, then I think this semantic change is kinda unavoidable, unless we instead name this something like FinitePlane3d or BoundedPlane3d, but that might be less user-friendly

Agreed. We just need to highlight it really clearly in the migration notes.

I think there is no code change required by this one then, correct?

NthTensor · 2024-04-07T01:35:33Z

crates/bevy_math/src/primitives/dim3.rs

+    pub fn new(normal: Vec3) -> Self {
+        Self {
+            normal: Dir3::new(normal).expect("normal must be nonzero and finite"),
+        }
+    }


Suggested change

pub fn new(normal: Vec3) -> Self {

Self {

normal: Dir3::new(normal).expect("normal must be nonzero and finite"),

}

}

pub fn new(normal: TryInto<Dir3>) -> Self {

Self {

normal: normal.try_into().expect("normal must be nonzero and finite"),

}

}

Sorry, the suggested change here produces an error, I am curious what is the driver behind this suggestion?

I think other methods also expect a Vec3 so I wouldn't change just one's signature.

If you have a Dir3 this will do a redundant conversion to and from a Vec. It looks to me like this is the only place a vector is taken as a parameter and immediately converted into a Dir3.

It's fine either way. What's the error?

Ah, it should be impl TryInto<Dir3>. Well anyway it's not super important.

Suggested change

pub fn new(normal: Vec3) -> Self {

Self {

normal: Dir3::new(normal).expect("normal must be nonzero and finite"),

}

}

pub fn new(normal: impl TryInto<Dir3>) -> Self {

Self {

normal: Dir3::new(normal).expect("normal must be nonzero and finite"),

}

}

@NthTensor Sorry, I missed your comments here. Thank you for the explanation!

@alice-i-cecile This is now addressed!

NthTensor · 2024-04-07T01:36:51Z

crates/bevy_render/src/mesh/primitives/dim3/plane.rs

@@ -7,39 +7,32 @@ use crate::{
 };

 /// A builder used for creating a [`Mesh`] with a [`Plane3d`] shape.
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, Default)]
 pub struct PlaneMeshBuilder {


If this just wraps Plane3d, can't we remove it?

Right now, we could, but we should probably add subdivisions back in a follow-up, so it'll be needed for that

Sounds good.

NthTensor

Thanks for your patience, I know this took me a while to get to. There's a few things I think we need to fix and double check before this is ready for merge, but it's a big improvement already, and it's in very good shape generally.

Great job!

andristarr · 2024-04-08T16:55:29Z

Thanks for reviewing! I have resolved the merge conflict as well, just awaiting your thoughts on my replies! :)

NthTensor

Looks like my feedback last week wasn't super helpful. Sorry about that, and thanks for the clarifications.

Happy to approve this now, looks good.

andristarr · 2024-04-15T14:24:25Z

This should be ready for merge @alice-i-cecile

NthTensor · 2024-04-15T14:30:00Z

I think we need another approval first. Maybe @Jondolf can give it a look?

alice-i-cecile

I think the TryInto API is meaningfully nicer: I've left a corrected suggestion on how to do this. Once that's done though I'm happy to merge :)

NthTensor · 2024-04-18T13:57:16Z

I'm going to go ahead and mark this as ready for final review, given Alice's conditional approval has been addressed.

github-actions · 2024-07-06T02:00:28Z

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

Addresses Bevy #12426 (bevyengine/bevy#12426)

andristarr force-pushed the adding-rect3d-primitive branch 5 times, most recently from 8777395 to a2db276 Compare March 13, 2024 14:13

TrialDragon 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 labels Mar 15, 2024

andristarr force-pushed the adding-rect3d-primitive branch 8 times, most recently from c74af27 to 4bba2a5 Compare March 17, 2024 20:34

andristarr marked this pull request as ready for review March 17, 2024 20:45

andristarr changed the title ~~adding the rect3d primitive~~ separating finite and infinite 3d planes Mar 17, 2024

NthTensor reviewed Mar 21, 2024

View reviewed changes

Separating bounded and infinite planes

58727bb

andristarr force-pushed the adding-rect3d-primitive branch from 4bba2a5 to 58727bb Compare March 21, 2024 17:13

andristarr added 2 commits March 21, 2024 19:12

Merge branch 'main' into adding-rect3d-primitive

224a4a2

Merge branch 'main' into adding-rect3d-primitive

5998ffd

Merge branch 'main' into adding-rect3d-primitive

cd8d40f

NthTensor reviewed Apr 7, 2024

View reviewed changes

NthTensor suggested changes Apr 7, 2024

View reviewed changes

andristarr added 2 commits April 8, 2024 18:39

New plane now expect a TryInto<Dir3>

0861253

Merge branch 'main' into adding-rect3d-primitive

d241852

andristarr force-pushed the adding-rect3d-primitive branch 2 times, most recently from ece914c to e8e3c67 Compare April 8, 2024 17:13

InfinitePlane3d new uses Vec3 now

1c0d228

andristarr force-pushed the adding-rect3d-primitive branch from e8e3c67 to 1c0d228 Compare April 8, 2024 17:19

NthTensor approved these changes Apr 14, 2024

View reviewed changes

alice-i-cecile requested changes Apr 15, 2024

View reviewed changes

alice-i-cecile added this to the 0.14 milestone Apr 15, 2024

The ctor for InfinitePlane3d consumes a TryInto<Dir3>

640d679

NthTensor 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 Apr 18, 2024

alice-i-cecile approved these changes Apr 18, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Apr 18, 2024

Merged via the queue into bevyengine:main with commit 2b3e334 Apr 18, 2024
28 checks passed

jf908 mentioned this pull request Jul 5, 2024

Migration guide: Missing Plane3d -> InfinitePlane3d bevyengine/bevy-website#1536

Closed

BD103 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 6, 2024

zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jul 7, 2024

Add half_size arg to Plane3d::new calls instead of PlaneMeshBuilder

67e5b53

Addresses Bevy #12426 (bevyengine/bevy#12426)

zhaop mentioned this pull request Jul 7, 2024

Upgrade to Bevy 0.14 & upgrade egui-gizmo dependency jakobhellermann/bevy_editor_pls#110

Merged

5 tasks

jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 2024

Add half_size arg to Plane3d::new calls instead of PlaneMeshBuilder

729d92f

Addresses Bevy #12426 (bevyengine/bevy#12426)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separating finite and infinite 3d planes #12426

separating finite and infinite 3d planes #12426

andristarr commented Mar 11, 2024

NthTensor commented Mar 17, 2024 •

edited

Loading

andristarr commented Mar 17, 2024

NthTensor Mar 21, 2024

andristarr Mar 21, 2024

andristarr commented Mar 22, 2024

andristarr commented Mar 28, 2024

NthTensor commented Mar 28, 2024

NthTensor Apr 7, 2024 •

edited

Loading

Jondolf Apr 7, 2024

NthTensor Apr 7, 2024

NthTensor Apr 7, 2024

Jondolf Apr 7, 2024

NthTensor Apr 7, 2024

andristarr Apr 8, 2024 •

edited

Loading

NthTensor Apr 14, 2024

NthTensor Apr 7, 2024

andristarr Apr 8, 2024

NthTensor Apr 14, 2024

NthTensor Apr 14, 2024

alice-i-cecile Apr 15, 2024

andristarr Apr 16, 2024

andristarr Apr 16, 2024

NthTensor Apr 7, 2024

Jondolf Apr 7, 2024

NthTensor Apr 7, 2024

NthTensor left a comment

andristarr commented Apr 8, 2024

NthTensor left a comment

andristarr commented Apr 15, 2024

NthTensor commented Apr 15, 2024

alice-i-cecile left a comment

NthTensor commented Apr 18, 2024

github-actions bot commented Jul 6, 2024

separating finite and infinite 3d planes #12426

separating finite and infinite 3d planes #12426

Conversation

andristarr commented Mar 11, 2024

Objective

Solution

NthTensor commented Mar 17, 2024 • edited Loading

andristarr commented Mar 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andristarr commented Mar 22, 2024

andristarr commented Mar 28, 2024

NthTensor commented Mar 28, 2024

NthTensor Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andristarr Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

andristarr commented Apr 8, 2024

NthTensor left a comment

Choose a reason for hiding this comment

andristarr commented Apr 15, 2024

NthTensor commented Apr 15, 2024

alice-i-cecile left a comment

Choose a reason for hiding this comment

NthTensor commented Apr 18, 2024

github-actions bot commented Jul 6, 2024

NthTensor commented Mar 17, 2024 •

edited

Loading

NthTensor Apr 7, 2024 •

edited

Loading

andristarr Apr 8, 2024 •

edited

Loading