-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve Frustum struct readability #21508
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
base: main
Are you sure you want to change the base?
Improve Frustum struct readability #21508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes this section much more readable!
crates/bevy_camera/src/primitives.rs
Outdated
let row2 = clip_from_world.row(2); | ||
let row3 = clip_from_world.row(3); | ||
|
||
half_spaces[Self::LEFT_PLANE_IDX] = HalfSpace::new(row3 + row0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section could also be written as a simple array instead of assigning each element individually.
i.e. Self { half_spaces: [HalfSpace::new(row3 + row0), ..] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you said makes sense. We don't even need to declare a mut half_space in advance and can directly return the value. However, some constants seem a bit redundant—I don't see where these constants are needed externally either.
crates/bevy_camera/src/primitives.rs
Outdated
const NEAR_PLANE_IDX: usize = 4; | ||
const FAR_PLANE_IDX: usize = 5; | ||
|
||
const INACTIVE_HALF_SPACE: Vec4 = Vec4::new(0.0, 0.0, 0.0, f32::MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied from the original code, but I'm wondering if it's correct to have a halfspace of NaNs here?
crates/bevy_camera/src/primitives.rs
Outdated
const LEFT_PLANE_IDX: usize = 0; | ||
const RIGHT_PLANE_IDX: usize = 1; | ||
const BOTTOM_PLANE_IDX: usize = 2; | ||
const TOP_PLANE_IDX: usize = 3; | ||
const NEAR_PLANE_IDX: usize = 4; | ||
const FAR_PLANE_IDX: usize = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be pub
, since half_spaces
is pub
as well, and is used outside of this module.
Objective
Testing