Deduplicate world_to_view logic #21715
Conversation
mnmaita
added
C-Code-Quality
| return Err(ViewportConversionError::PastNearPlane); | ||
| } | ||
|
|
||
| // Stretching ndc depth to value via near plane and negating result to be in positive room again. |
There was a problem hiding this comment.
Did you intend to delete this comment as well? If not, you can place it back above let depth …
|
Beyond the deletion of the comment, this LGTM, with the caveat that this is my first pull request I am reviewing in the codebase and I’m also new to Rust |
crates/bevy_camera/src/camera.rs
Outdated
| let viewport_position = | ||
| (ndc_space_coords.truncate() + Vec2::ONE) / 2.0 * target_rect.size() + target_rect.min; | ||
| Ok(viewport_position) | ||
| Ok((viewport_position, ndc_space_coords.z)) |
There was a problem hiding this comment.
Does truncate() when creating viewport_position mutate ndc_space_coords at all, specifically the z-component? if it does, the value for z might not be what is expected here, and it might be safer for this function to return depth (since depth in the previous logic seemed to be created before viewport_position)
(New here so take this with a grain of salt!)
There was a problem hiding this comment.
The z component’s value isn’t changed, but I understand why it’s confusing.
The truncate() function consumes the entire ndc_space_coords struct. The reason it still works is due to Rust’s Non-Lexicographical Lifetimes (NLL). The compiler is smart enough to recognize that even though the whole struct is moved, the z field was never actually needed by truncate(), so it remains valid for the final Ok return.
However, I completely agree with your sentiment. Relying on this advanced compiler behavior is not ideal practice. It makes the code less clear and more fragile.
3 participants
Objective
Solution
Testing
cargo testand all the tests pass.