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

Error in frustum math. #3744

Closed
ixru opened this issue Jan 22, 2022 · 5 comments
Closed

Error in frustum math. #3744

ixru opened this issue Jan 22, 2022 · 5 comments
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@ixru
Copy link

ixru commented Jan 22, 2022

Bevy version

0.6.0

What you did

When trying to use the new frustum from 0.6 I noticed it being too inclusive when using the intersects_sphere method, causing some flickering when loading new meshes within it.
Looking at the code I think I see an error in how the radius is handled. Adding radius directly to the normalized dot product causes it to add multiples of the vector's original length instead of the radius * length of normalized normal vector I assume it is meant to use.

@ixru ixru added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jan 22, 2022
@alice-i-cecile alice-i-cecile added A-Math Fundamental domain-agnostic mathematical operations S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! and removed S-Needs-Triage This issue needs to be labelled labels Jan 23, 2022
@superdump superdump added the A-Rendering Drawing game state to the screen label Feb 1, 2022
@superdump
Copy link
Contributor

I think it is correct. The frustum plane normals are inward facing and normalised. Given the equation for points in a plane ax + by + cz + d = 0, a,b,c is the unit normal, and d is the d value. Given a point p, the shortest distance from p to the plane is along the normal to the plane. How far turns out to be (a,b,c,d) dot (px, py, pz, 1) as I recall, and given inward facing normals, if this value is positive, the point is inside and if it’s negative the point is outside. For intersecting with a sphere we want to expand the frustum plane out by the radius which means the dot product now needs to be <= -radius or dot product + radius <= 0. I got most of this from Eric Lengyel’s Foundations of Game Engine Development Volume 2: Rendering. Maybe there is a bug in there. I’m not 100% certain.

Could you give more details about the flickering bug and either provide a minimal example that reproduces it or give instructions?

@ixru
Copy link
Author

ixru commented Feb 2, 2022

Sorry for the bad description. An example of how I think it works:
Given the default far value of 1000 and looking down the z-axis [0,0,-1] the far plane is defined by plane = [0, 0, 1, 1000]
Normalized that gives something approximate to plane_n = [0, 0, 0.0009, 0.999].
Choosing a point far outside the plane point = [0,0,-2000].
Now plane_n * point.extend(1) ~= -1 or plane * point.extend(1) = -1000.
A radius of 10 will then give -1 + 10 = 9 which is inside the frustum, or the point actually at [0, 0, 8000].
While the non-normalized one -1000 + 10 = -990 is still outside. This is what I meant by the radius adding multiples when the plane is normalized.

@ixru
Copy link
Author

ixru commented Feb 2, 2022

use bevy::{
    prelude::*,
    render::primitives::{Frustum, Sphere},
};

fn main() {
    App::new()
        .add_startup_system(spawn_camera)
        .add_system(test_frustum)
        .run();
}

fn test_frustum(frustum_query: Query<&Frustum>) {
    let frustum = frustum_query.single();
    let sphere = Sphere {
        center: Vec3::new(0.0, 0.0, -2000.0),
        radius: 10.0,
    };
    // frustum.intersects_sphere(&sphere), only far plane
    let inside = frustum.planes[5].normal_d.dot(sphere.center.extend(1.0)) + sphere.radius > 0.0;
    // Prints true, but should be false
    dbg!(inside);
}

fn spawn_camera(mut commands: Commands) {
    commands.spawn_bundle(PerspectiveCameraBundle::default());
}

@superdump
Copy link
Contributor

Thanks. In my re-reading I had missed that the Vec4s were being normalized including all 4 components. I understand the problem now.

@superdump superdump added this to the Bevy 0.6.1 milestone Feb 2, 2022
@superdump
Copy link
Contributor

I'll add this to the rendering project board. Given no one has noticed this so far suggests it's not that bad of an impact in practice, and to fix it I would like to add a bunch of tests for these intersection checks, and that will take a bit of time.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.6.1 milestone Feb 10, 2022
@bors bors bot closed this as completed in e3a3b5b Mar 8, 2022
rparrett pushed a commit to rparrett/bevy that referenced this issue Mar 15, 2022
# 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.
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
# 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.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# 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.
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-Bug An unexpected or incorrect behavior S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants