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

Frustum::from_matrix4 is wrong #125

Closed
flodiebold opened this issue Sep 30, 2014 · 0 comments · Fixed by #187
Closed

Frustum::from_matrix4 is wrong #125

flodiebold opened this issue Sep 30, 2014 · 0 comments · Fixed by #187
Labels

Comments

@flodiebold
Copy link

For example,

let persp = cgmath::PerspectiveFov {
    fovy: cgmath::Deg { s: 90.0f32 },
    aspect: 1.0,
    near: 0.1,
    far: 1000.0
};
let frustum = Frustum::from_matrix4(persp.to_matrix4());

println!("{}", frustum.far);

println!("{}", persp.to_matrix4().mul_v(&Vector4::new(0.0, 0.0, 1.0, 1.0)));
println!("{}", persp.to_matrix4().mul_v(&Vector4::new(0.0, 0.0, -1.0, 1.0)));
println!("{}", persp.to_matrix4().mul_v(&Vector4::new(0.0, 0.0, -999.0, 1.0)));
println!("{}", persp.to_matrix4().mul_v(&Vector4::new(0.0, 0.0, -1001.0, 1.0)));

outputs 0x + 0y + 0.000999z - 0.999999 = 0 for the far plane, i.e. z=1000, when in fact the plane should be z=-1000.

I think this is because Plane::from_vector4 uses the vector's fourth component as D, but the way from_matrix4 uses it, it should be -D. For example, the left clipping plane should have the equation v . (row(3) + row(0)) = 0 (taking the rows from the projection matrix), but since planes are represented with equations of the form A_x + B_y + C*z - D = 0, the D is negated.

I'm not sure whether Plane::from_vector4 is doing what's intended, but either it needs to negate D, or Frustum::from_matrix4 needs to be fixed somehow.

@kvark kvark added the bug label Oct 1, 2014
@ghost ghost closed this as completed in #187 Mar 15, 2015
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants