Skip to content

Deduplicate world_to_view logic #21715

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.

Sign up for GitHub

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

Open
Breakdown-Dog wants to merge 5 commits into
base: main
Choose a base branch
from
+32 −34
Open

Deduplicate world_to_view logic #21715

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b3a18c
Deduplicate world_to_view logic
Breakdown-Dog Nov 2, 2025
73629fe
fixes
Breakdown-Dog Nov 2, 2025
54ebac0
fixes comment
Breakdown-Dog Nov 2, 2025
152bacd
update as suggested
Breakdown-Dog Nov 9, 2025
f1b5f53
fmt
Breakdown-Dog Nov 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 30 additions & 35 deletions crates/bevy_camera/src/camera.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,21 +489,17 @@ impl Camera {
self.computed.clip_from_view
}

/// Given a position in world space, use the camera to compute the viewport-space coordinates.
///
/// To get the coordinates in Normalized Device Coordinates, you should use
/// [`world_to_ndc`](Self::world_to_ndc).
/// Core conversion logic to compute viewport coordinates
///
/// # Panics
/// This function is shared by `world_to_viewport` and `world_to_viewport_with_depth`
/// to avoid code duplication.
///
/// Will panic if `glam_assert` is enabled and the `camera_transform` contains `NAN`
/// (see [`world_to_ndc`][Self::world_to_ndc]).
#[doc(alias = "world_to_screen")]
pub fn world_to_viewport(
/// Returns a tuple `(viewport_position, ndc_space_coords.z)`.
fn world_to_viewport_core(
&self,
camera_transform: &GlobalTransform,
world_position: Vec3,
) -> Result<Vec2, ViewportConversionError> {
) -> Result<(Vec2, f32), ViewportConversionError> {
let target_rect = self
.logical_viewport_rect()
.ok_or(ViewportConversionError::NoViewportSize)?;
Expand All @@ -524,7 +520,27 @@ impl Camera {
// Once in NDC space, we can discard the z element and map x/y to the viewport rect
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))
Copy link
Contributor

@kfc35 kfc35 Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor Author

@Breakdown-Dog Breakdown-Dog Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Given a position in world space, use the camera to compute the viewport-space coordinates.
///
/// To get the coordinates in Normalized Device Coordinates, you should use
/// [`world_to_ndc`](Self::world_to_ndc).
///
/// # Panics
///
/// Will panic if `glam_assert` is enabled and the `camera_transform` contains `NAN`
/// (see [`world_to_ndc`][Self::world_to_ndc]).
#[doc(alias = "world_to_screen")]
pub fn world_to_viewport(
&self,
camera_transform: &GlobalTransform,
world_position: Vec3,
) -> Result<Vec2, ViewportConversionError> {
Ok(self
.world_to_viewport_core(camera_transform, world_position)?
.0)
}

/// Given a position in world space, use the camera to compute the viewport-space coordinates and depth.
Expand All @@ -542,30 +558,9 @@ impl Camera {
camera_transform: &GlobalTransform,
world_position: Vec3,
) -> Result<Vec3, ViewportConversionError> {
let target_rect = self
.logical_viewport_rect()
.ok_or(ViewportConversionError::NoViewportSize)?;
let mut ndc_space_coords = self
.world_to_ndc(camera_transform, world_position)
.ok_or(ViewportConversionError::InvalidData)?;
// NDC z-values outside of 0 < z < 1 are outside the (implicit) camera frustum and are thus not in viewport-space
if ndc_space_coords.z < 0.0 {
return Err(ViewportConversionError::PastFarPlane);
}
if ndc_space_coords.z > 1.0 {
return Err(ViewportConversionError::PastNearPlane);
}

// Stretching ndc depth to value via near plane and negating result to be in positive room again.
Copy link
Contributor

@kfc35 kfc35 Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to delete this comment as well? If not, you can place it back above let depth …

let depth = -self.depth_ndc_to_view_z(ndc_space_coords.z);

// Flip the Y co-ordinate origin from the bottom to the top.
ndc_space_coords.y = -ndc_space_coords.y;

// Once in NDC space, we can discard the z element and map x/y to the viewport rect
let viewport_position =
(ndc_space_coords.truncate() + Vec2::ONE) / 2.0 * target_rect.size() + target_rect.min;
Ok(viewport_position.extend(depth))
let result = self.world_to_viewport_core(camera_transform, world_position)?;
let depth = -self.depth_ndc_to_view_z(result.1);
Ok(result.0.extend(depth))
}

/// Returns a ray originating from the camera, that passes through everything beyond `viewport_position`.
Expand Down