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.

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

Document when Camera::viewport_to_world and related methods return None #8841

Merged
merged 8 commits into from
Aug 3, 2023
Merged

Document when Camera::viewport_to_world and related methods return None #8841

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2e35ff0
Document when Camera::viewport_to_world and related methods return None
tormeh Jun 14, 2023
0aa192b
fixup: backtick None in comments
tormeh Jun 29, 2023
8fd32f3
fixup?: clarify render target comment
tormeh Jun 29, 2023
f7c871e
fixup: logical_viewport_size doc improvements
tormeh Jul 1, 2023
b2a63bf
fixup: world_to_ndc doc improvements
tormeh Jul 1, 2023
3b0002c
ndc_to_world doc improvements.
tormeh Jul 1, 2023
9e065e7
fixup: implicit frustum note
tormeh Jul 1, 2023
23996f0
Fixup: unbalanced backtics
tormeh Jul 1, 2023
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
32 changes: 31 additions & 1 deletion crates/bevy_render/src/camera/camera.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,14 @@ impl Camera {
/// of the current [`RenderTarget`].
/// For logic that requires the full logical size of the
/// [`RenderTarget`], prefer [`Camera::logical_target_size`].
///
/// Returns `None` if either:
/// - the function is called just after the `Camera` is created, before `camera_system` is executed,
/// - the [`RenderTarget`] isn't correctly set:
/// - it references the [`PrimaryWindow`](RenderTarget::Window) when there is none,
/// - it references a [`Window`](RenderTarget::Window) entity that doesn't exist or doesn't actually have a `Window` component,
/// - it references an [`Image`](RenderTarget::Image) that doesn't exist (invalid handle),
/// - it references a [`TextureView`](RenderTarget::TextureView) that doesn't exist (invalid handle).
#[inline]
pub fn logical_viewport_size(&self) -> Option<Vec2> {
self.viewport
Expand Down Expand Up @@ -218,6 +226,12 @@ impl Camera {
///
/// To get the coordinates in Normalized Device Coordinates, you should use
/// [`world_to_ndc`](Self::world_to_ndc).
///
/// Returns `None` if any of these conditions occur:
/// - The computed coordinates are beyond the near or far plane
Copy link
Member

@Selene-Amanita Selene-Amanita Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On this last point:

I think this reason should be the first in the list, it's the only "unexpected" one, the others are all like "something went wrong", this one is a design choice (to exclude stuff outside the frustum but only for front and back, not left/right or up/down). I think it kinda makes sense otherwise since we discard the z component we have no way to know if it's in the frustum or not, but it should be more "advertised".

Also, technically I feel like despite was is said in the comment bellow, if the "implicit frustum" defined by the projection matrix (the one used to actually render stuff), and the "actual frustum" (the one used to exclude stuff from being considered in the rendering process), are not the same (which, I think, can happen, but you'd have to do some weird stuff, by default they are the same), this uses the "implicit frustum", not the actual one, that maybe should be mentioned?

This technicality also applies to the definition of "near" and "far" planes in viewport_to_world and viewport_to_world_2d.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand this, but I've added a parentheses note

Copy link
Member

@Selene-Amanita Selene-Amanita Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can change the line to "The computed coordinates are beyond the near or far plane defined by the projection,". Or maybe link to https://docs.rs/bevy/latest/bevy/render/camera/enum.Projection.html:

- The computed coordinates are beyond the near or far plane defined by the [`Projection`],

I would also change the comment inside the code bellow to "(implicit, defined by the projection)"

For more details if you're interested, if you look at, for example, Camera3dBundle, you'll notice it has a Projection component, and a Frustum component.

The projection is used to transform all the objects of the world to fit them in a small box in front of the camera, this box will then be "squished" to a rectangle that will make up the image rendered by the camera.

But because everything that won't end up in that box don't need to be transformed, we optimize beforehand which ones need to be. For that there is the frustum, which is a kind of truncated pyramid (in the case of a perspective projection) that defines the limits of what the camera can see. We can check if an object is in the pyramid, if it isn't we don't consider it for rendering at all.

In almost all scenarios, the Frustum is calculated from the Projection, the truncated pyramid will become the rendering box after projection, so the near and far plane of both (where the pyramid is truncated for the Frustum, and the limits of the projection box) are the same.

But in some specific scenarios they can not be, hence why I think it's important to specify it explicitly here: this function returns None if the point will end up in front of or behind the rendering box after projection (but it will still return Some if it's left, right, up or down of the box), but it ignores completely the frustum.

/// - The logical viewport size cannot be computed. See [`logical_viewport_size`](Camera::logical_viewport_size)
/// - The world coordinates cannot be mapped to the Normalized Device Coordinates. See [`world_to_ndc`](Camera::world_to_ndc)
/// May also panic if `glam_assert` is enabled. See [`world_to_ndc`](Camera::world_to_ndc).
#[doc(alias = "world_to_screen")]
pub fn world_to_viewport(
&self,
Expand All @@ -226,7 +240,7 @@ impl Camera {
) -> Option<Vec2> {
let target_size = self.logical_viewport_size()?;
let ndc_space_coords = self.world_to_ndc(camera_transform, world_position)?;
// NDC z-values outside of 0 < z < 1 are outside the camera frustum and are thus not in viewport-space
// 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 || ndc_space_coords.z > 1.0 {
return None;
}
Expand All @@ -246,6 +260,11 @@ impl Camera {
///
/// To get the world space coordinates with Normalized Device Coordinates, you should use
/// [`ndc_to_world`](Self::ndc_to_world).
///
/// Returns `None` if any of these conditions occur:
/// - The logical viewport size cannot be computed. See [`logical_viewport_size`](Camera::logical_viewport_size)
/// - The near or far plane cannot be computed. This can happen if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`.
/// Panics if the projection matrix is null and `glam_assert` is enabled.
pub fn viewport_to_world(
&self,
camera_transform: &GlobalTransform,
Expand Down Expand Up @@ -274,6 +293,11 @@ impl Camera {
///
/// To get the world space coordinates with Normalized Device Coordinates, you should use
/// [`ndc_to_world`](Self::ndc_to_world).
///
/// Returns `None` if any of these conditions occur:
/// - The logical viewport size cannot be computed. See [`logical_viewport_size`](Camera::logical_viewport_size)
/// - The viewport position cannot be mapped to the world. See [`ndc_to_world`](Camera::ndc_to_world)
/// May panic. See [`ndc_to_world`](Camera::ndc_to_world).
pub fn viewport_to_world_2d(
&self,
camera_transform: &GlobalTransform,
Expand All @@ -295,6 +319,9 @@ impl Camera {
/// and between 0.0 and 1.0 on the Z axis.
/// To get the coordinates in the render target's viewport dimensions, you should use
/// [`world_to_viewport`](Self::world_to_viewport).
///
/// Returns `None` if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`.
/// Panics if the `camera_transform` contains `NAN` and the `glam_assert` feature is enabled.
pub fn world_to_ndc(
&self,
camera_transform: &GlobalTransform,
Expand All @@ -315,6 +342,9 @@ impl Camera {
/// and between 0.0 and 1.0 on the Z axis.
/// To get the world space coordinates with the viewport position, you should use
/// [`world_to_viewport`](Self::world_to_viewport).
///
/// Returns `None` if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`.
/// Panics if the projection matrix is null and `glam_assert` is enabled.
Comment on lines +346 to +347
Copy link
Member

@Selene-Amanita Selene-Amanita Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
/// Returns `None` if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`.
/// Panics if the projection matrix is null and `glam_assert` is enabled.
/// Returns `None` if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`,
/// or the projection matrix cannot be inverted.
/// Panics if the projection matrix cannot be inverted and `glam_assert` is enabled.

Same change for: viewport_to_world

Actually I checked the default Mat4 is the identity matrix (the one with 1 in its diagonal and 0 elsewhere), which can be inverted (its inverse is equal to itself), so it would just return invalid values in the case the projection matrix is not set. To be handled in another PR probably not worth mentioning it in a comment.

pub fn ndc_to_world(&self, camera_transform: &GlobalTransform, ndc: Vec3) -> Option<Vec3> {
// Build a transformation matrix to convert from NDC to world space using camera data
let ndc_to_world =
Expand Down