[Merged by Bors] - Fixed the frustum-sphere collision and added tests #4035

hayashi-stl · 2022-02-25T07:32:17Z

Objective

Solution

The old code used the formula normal . center + d + radius <= 0 to determine if the sphere with center center and radius radius is outside the plane with normal normal and distance from origin d. This only works if normal is normalized, which is not necessarily the case. Instead, normal and d are both multiplied by some factor that radius isn't multiplied by. So the additional code multiplied radius by that factor.

james7132

This is just a review on the code quality itself, not the correctness of it.

james7132 · 2022-02-25T09:58:46Z

crates/bevy_render/src/primitives/mod.rs

-            if plane.normal_d.dot(sphere.center.extend(1.0)) + sphere.radius <= 0.0 {
+            // The formula `normal . center + d + radius <= 0` relies on `normal` being normalized,
+            // which is not necessarily the case.
+            let factor = (plane.normal_d.truncate().length_squared()


Please split up this statement. It's hard to read in it's current state.

crates/bevy_render/src/primitives/mod.rs

superdump · 2022-02-25T15:12:17Z

I fixed this yesterday and was going to make a PR: superdump@34d134d

I was thinking of making a Plane constructor function that ensures the normal is normalised and the entire normal_d is scaled accordingly. Perhaps you could rework this PR to do that instead? I would rather have the normal be a unit normal and d define the plane according to that unit normal.

superdump

I've made a few suggestions for further changes for how I think the API should be after a bit more thought. Hopefully the suggested changes are all correct and there's not too much to fix elsewhere.

superdump · 2022-02-26T22:03:34Z

crates/bevy_render/src/primitives/mod.rs

@@ -72,14 +72,26 @@ impl Sphere {
    }
 }

-/// A plane defined by a normal and distance value along the normal
+/// A plane defined by a normalized normal and distance value along the normal


Suggested change

/// A plane defined by a normalized normal and distance value along the normal

/// A plane defined by a unit normal and distance from the origin along the normal

superdump · 2022-02-26T22:06:05Z

crates/bevy_render/src/primitives/mod.rs

@@ -72,14 +72,26 @@ impl Sphere {
    }
 }

-/// A plane defined by a normal and distance value along the normal
+/// A plane defined by a normalized normal and distance value along the normal
 /// Any point p is in the plane if n.p = d


Suggested change

/// Any point p is in the plane if n.p = d

/// Any point p is in the plane if n.p + d = 0

superdump · 2022-02-26T22:07:15Z

crates/bevy_render/src/primitives/mod.rs

 /// Any point p is in the plane if n.p = d
-/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side of the plane.
+/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side (inside) of the plane.


Suggested change

/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side (inside) of the plane.

/// For planes defining half-spaces such as for frusta, if n.p + d > 0 then p is on

/// the positive side (inside) of the plane.

superdump · 2022-02-26T22:07:31Z

crates/bevy_render/src/primitives/mod.rs

 /// Any point p is in the plane if n.p = d
-/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side of the plane.
+/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side (inside) of the plane.
 #[derive(Clone, Copy, Debug, Default)]
 pub struct Plane {
    pub normal_d: Vec4,


Suggested change

pub normal_d: Vec4,

normal_d: Vec4,

And then update other code that uses Plane to use Plane::new for construction.

superdump · 2022-02-26T22:08:48Z

crates/bevy_render/src/primitives/mod.rs

+    /// Constructs a `Plane` from a 4D vector whose first 3 components
+    /// are the normal and whose last component is d.
+    /// Ensures that the normal is normalized and d is scaled accordingly
+    /// so it represents the signed distance from the origin.


Suggested change

/// Constructs a `Plane` from a 4D vector whose first 3 components

/// are the normal and whose last component is d.

/// Ensures that the normal is normalized and d is scaled accordingly

/// so it represents the signed distance from the origin.

/// Constructs a `Plane` from a 4D vector whose first 3 components

/// are the normal and whose last component is the distance along the normal

/// from the origin.

/// This constructor ensures that the normal is normalized and the distance is

/// scaled accordingly so it represents the signed distance from the origin.

superdump · 2022-02-26T22:12:47Z

crates/bevy_render/src/primitives/mod.rs

+    /// are the normal and whose last component is d.
+    /// Ensures that the normal is normalized and d is scaled accordingly
+    /// so it represents the signed distance from the origin.
+    fn new(normal_d: Vec4) -> Self {


Suggested change

fn new(normal_d: Vec4) -> Self {

pub fn new(normal_d: Vec4) -> Self {

superdump · 2022-02-26T22:17:20Z

crates/bevy_render/src/primitives/mod.rs

+        Self {
+            normal_d: normal_d * normal_d.xyz().length_recip(),
+        }
+    }


Suggested change

}

}

/// `Plane` unit normal

#[inline]

pub fn normal(&self) -> Vec3 {

self.normal_d.xyz()

}

/// Signed distance from the origin along the unit normal such that n.p + d = 0 for point p in

/// the `Plane`

#[inline]

pub fn d(&self) -> f32 {

self.normal_d.w

}

/// `Plane` unit normal and signed distance from the origin such that n.p + d = 0 for point p

/// in the `Plane`

#[inline]

pub fn normal_d(&self) -> Vec4 {

self.normal_d

}

And then use these in the Frustum intersects sphere/obb functions below.

superdump · 2022-03-02T05:35:06Z

Thanks for writing the tests by the way. With the suggestions, I would likely approve this and try to prioritise it being merged.

mockersf · 2022-03-05T03:27:36Z

crates/bevy_render/src/primitives/mod.rs

+    /// from the origin.
+    /// This constructor ensures that the normal is normalized and the distance is
+    /// scaled accordingly so it represents the signed distance from the origin.
+    pub fn new(normal_d: Vec4) -> Self {


should this one be inline too?

I suppose it could be. I haven’t tested it. I’m just using Cart’s of using online for accessors. But I don’t think we tend to use them for constructors.

it seems the constructor is used a few time during from_view_projection

I've made the suggestion. I'm leaving it as approved as it's too minor to block on imo.

superdump · 2022-03-06T18:19:34Z

crates/bevy_render/src/primitives/mod.rs

+    /// from the origin.
+    /// This constructor ensures that the normal is normalized and the distance is
+    /// scaled accordingly so it represents the signed distance from the origin.
+    pub fn new(normal_d: Vec4) -> Self {


Suggested change

pub fn new(normal_d: Vec4) -> Self {

#[inline]

pub fn new(normal_d: Vec4) -> Self {

mockersf · 2022-03-08T00:28:49Z

bors r+

# Objective Fixes #3744 ## Solution The old code used the formula `normal . center + d + radius <= 0` to determine if the sphere with center `center` and radius `radius` is outside the plane with normal `normal` and distance from origin `d`. This only works if `normal` is normalized, which is not necessarily the case. Instead, `normal` and `d` are both multiplied by some factor that `radius` isn't multiplied by. So the additional code multiplied `radius` by that factor.

bors · 2022-03-08T00:45:54Z

Pull request successfully merged into main.

Build succeeded:

# Objective Fixes bevyengine#3744 ## Solution The old code used the formula `normal . center + d + radius <= 0` to determine if the sphere with center `center` and radius `radius` is outside the plane with normal `normal` and distance from origin `d`. This only works if `normal` is normalized, which is not necessarily the case. Instead, `normal` and `d` are both multiplied by some factor that `radius` isn't multiplied by. So the additional code multiplied `radius` by that factor.

…e#4035)" This reverts commit 206c423. This commit was a breaking change for bevy_inspector_egui and was not actually consequential for typey birb

# Objective Fixes bevyengine#3744 ## Solution The old code used the formula `normal . center + d + radius <= 0` to determine if the sphere with center `center` and radius `radius` is outside the plane with normal `normal` and distance from origin `d`. This only works if `normal` is normalized, which is not necessarily the case. Instead, `normal` and `d` are both multiplied by some factor that `radius` isn't multiplied by. So the additional code multiplied `radius` by that factor.

Fixed the frustum-sphere collision and added tests

16bb5b7

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 25, 2022

Formatting

881db86

james7132 added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Feb 25, 2022

james7132 requested changes Feb 25, 2022

View reviewed changes

Added constructor that normalizes normal

ce787f6

superdump requested changes Feb 26, 2022

View reviewed changes

Applied suggestions

3507530

superdump approved these changes Mar 3, 2022

View reviewed changes

superdump requested a review from mockersf March 3, 2022 22:46

mockersf reviewed Mar 5, 2022

View reviewed changes

superdump reviewed Mar 6, 2022

View reviewed changes

superdump 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 Mar 7, 2022

Inlined constructor of Plane

a46ea9f

mockersf approved these changes Mar 7, 2022

View reviewed changes

bors bot changed the title ~~Fixed the frustum-sphere collision and added tests~~ [Merged by Bors] - Fixed the frustum-sphere collision and added tests Mar 8, 2022

bors bot closed this Mar 8, 2022

hayashi-stl deleted the frustum_fix branch March 11, 2022 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fixed the frustum-sphere collision and added tests #4035

[Merged by Bors] - Fixed the frustum-sphere collision and added tests #4035

hayashi-stl commented Feb 25, 2022

james7132 left a comment

james7132 Feb 25, 2022

superdump commented Feb 25, 2022

superdump left a comment

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump Feb 26, 2022

superdump commented Mar 2, 2022

mockersf Mar 5, 2022

superdump Mar 5, 2022

mockersf Mar 5, 2022

superdump Mar 6, 2022

superdump Mar 6, 2022

mockersf commented Mar 8, 2022

bors bot commented Mar 8, 2022

	/// A plane defined by a normalized normal and distance value along the normal
	/// A plane defined by a unit normal and distance from the origin along the normal

	/// Any point p is in the plane if n.p = d
	/// Any point p is in the plane if n.p + d = 0

	/// For planes defining half-spaces such as for frusta, if n.p > d then p is on the positive side (inside) of the plane.
	/// For planes defining half-spaces such as for frusta, if n.p + d > 0 then p is on
	/// the positive side (inside) of the plane.

	fn new(normal_d: Vec4) -> Self {
	pub fn new(normal_d: Vec4) -> Self {

-    }
+    }
+    /// `Plane` unit normal
+    #[inline]
+    pub fn normal(&self) -> Vec3 {
+        self.normal_d.xyz()
+    }
+    /// Signed distance from the origin along the unit normal such that n.p + d = 0 for point p in
+    /// the `Plane`
+    #[inline]
+    pub fn d(&self) -> f32 {
+        self.normal_d.w
+    }
+    /// `Plane` unit normal and signed distance from the origin such that n.p + d = 0 for point p
+    /// in the `Plane`
+    #[inline]
+    pub fn normal_d(&self) -> Vec4 {
+        self.normal_d
+    }

	pub fn new(normal_d: Vec4) -> Self {
	#[inline]
	pub fn new(normal_d: Vec4) -> Self {

[Merged by Bors] - Fixed the frustum-sphere collision and added tests #4035

[Merged by Bors] - Fixed the frustum-sphere collision and added tests #4035

Conversation

hayashi-stl commented Feb 25, 2022

Objective

Solution

james7132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superdump commented Feb 25, 2022

superdump left a 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

superdump commented Mar 2, 2022

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

mockersf commented Mar 8, 2022

bors bot commented Mar 8, 2022