Document when Camera::viewport_to_world and related methods return None

[tormeh]

Objective

Fixes #7171

Solution

• Add documentation for when Camera::viewport_to_world and related methods return None

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[tormeh]

This is my first Bevy PR and I'm a total rendering noob, so I hope I got it right 😅

[alice-i-cecile]

Good! I'm not 100% convinced that this is exhaustive, but I couldn't find any other failure cases myself when examining the code.

Probably an argument for using a result here for better debugging, but that should be done in a later PR.

[tormeh]

Good! I'm not 100% convinced that this is exhaustive, but I couldn't find any other failure cases myself when examining the code.

Probably an argument for using a result here for better debugging, but that should be done in a later PR.

I don't have merge privileges, btw. Someone else will have to merge

[alice-i-cecile]

Yep, I'm one of the folks with merge rights. Once we have a second approval (from anyone), this PR will be marked with Ready-For-Final-Review and we'll merge it in :)

[rparrett]

I think this might not be entirely correct.

To me, it looks like this would return None only if there is no render target, or if the projection matrix / render target size have not yet been computed by camera_system.

[Selene-Amanita]

I agree with that, the "render target" cannot not be set, it's not an option, but the "computed render target", Camera.computed.target_info, can not be set if the camera was just created or something.

[rparrett]

Ah, so I suppose it's just a matter of perspective. I am imagining reading this as a user with no knowledge of that internal implementation detail and it seems a bit confusing as-is from that angle.

As a user, I can set a custom viewport or not, and that doesn't really matter here.

As a user, the render target is not optional (but perhaps I can set it to something invalid? or a reference may break, or whatever)

As a user, I might attempt to use these functions before computed.whatever is computed by camera_system.

I personally think that we should use this sort of user-facing language, or if we're talking about specific optional properties, maybe throw some backticks around them to make that more clear.

[Selene-Amanita]

I agree.

Actually, no matter if the viewport is set or not you need Camera.computed.target_info, so it doesn't matter at all if there is a viewport.

Inside camera_system, Camera.computed.target_info is set using get_render_target_info, which can return None for four reasons I think.

The comment here should be something like this:

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

[tormeh]

Made a draft for this based on @Selene-Amanita's comment

[Selene-Amanita]

I investigated a bit more and edited my message, sorry @tormeh I saw that you already added my previous suggestion ^^'

[Selene-Amanita]

I think for world_to_viewport (and others) you can put a reference instead of copying the list, like

The logical viewport size cannot be computed, see [logical_viewport_size](Camera::logical_viewport_size)

[tormeh]

Done

[Selene-Amanita]

I don't know how to comment a line that isn't edited in the PR but I think it could be good to have more cross-reference in some places, for example NormalizedRenderTarget's doc mentions "render target" but doesn't point directly to RenderTarget (like this:)

[`RenderTarget`]

Same for the doc of its variants, and the variants of RenderTarget, and probably other stuff in this file.

This PR can be merged without it but I think it'll be a nice bonus while we're at it.

[Selene-Amanita]

1. "the projection matrix" is something computed here, like the "computed render target" in logical_viewport_size, it comes from CameraProjection.

• It can not "be set", to be set it has exactly the same requirements to be set as logical_viewport_size (it's set at the same place as target_info, and uses logical_viewport_size for computation, then uses get_projection_matrix from the CameraProjection which should never fail I think). If it is not set it would probably be a matrix full of zero, and this function would return something wrong. This should probably be mentioned, and maybe even fixed (I feel like if it's not set this function should return None). You can put a reference here like this: Document when Camera::viewport_to_world and related methods return None #8841 (comment)
• It can be "invalid" and eventually return None if it makes the result have NAN, see point 3.

2. For the camera transform, I think computing the matrix never fails even if the Transform has stuff like NaN, but if it can't be inverted (not sure when that happens in that case, I think a Matrix generated from a transform could always be inverted, but maybe with NaN, see point 3?) it will Panic, not return None, when the glam_assert feature is set.
   If glam_assert is not set and the inversion "fails" silently, I think the world_to_ndc variable would have some NaN inside and the method will ultimately return None.

3. "invalid" here I think is the same as point 1 and 2, it means that it contains some NAN, and maybe INFINITY or NEG_INFINITY (?)

4. I think the function could fail if the resulting coordinate is "too big"? Not sure about that it may just return stuff with INFINITY not NAN

I would maybe word the comment like this :

Returns `None` if the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`.

Returns an invalid value if the conditions for  [`logical_viewport_size`](Camera::logical_viewport_size) to return `Some ` are not met.

Panics if the `camera_transform` contains `NAN` and the `glam_assert feature is enabled.

Or put this at the top of the function (as a quick fix, projection_matrix should probably be an Option tbh, or computed shoud be an Option) :

// `self.computed.projection_matrix` is set only if `self.computed.target_info` is set,
// otherwise it's a "zero" matrix and should not be used for computation.
if self.computed.target_info.is_none {
    return None;
}

Or even get rid of the panic entirely (? if people don't want a panic they can just not use glam_assert, the end of the function would return NAN anyway) with:

if self.computed.target_info.is_none || self.computed.projection_matrix.is_nan() {
    return None;
}

Actually a projection matrix should never be a 0 matrix so this can be used instead:

// The projection matrix can be not set if the conditions for `logical_viewport_size` to return some are not met
if self.computed.projection_matrix == Mat4::ZERO || self.computed.projection_matrix.is_nan() {
    return None;
}

And then the comment would be like this:

Returns `None` if either:
- the `camera_transform`, the `world_position`, or the projection matrix defined by [`CameraProjection`] contain `NAN`,
- the logical viewport size cannot be computed, see [logical_viewport_size](Camera::logical_viewport_size).

[tormeh]

Pushed a potential fix. I want to restrict the scope of this PR to not involve any code changes, so haven't done any of that

[Selene-Amanita]

Fair ^^

We (you, me, someone else, idk) can make another PR later for that.

I'll review more thoroughly later but at a glance it seems good to me.

[Selene-Amanita]

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.

[tormeh]

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

[Selene-Amanita]

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.

[Selene-Amanita]

Same as https://github.com/bevyengine/bevy/pull/8841/files#r1246420948 except here if the Matrix is not set it would panic or put invalid values (depending on glam_assert) too because the 0 matrix can't be inverted.

The "quick fix" to return None early could be added for this function too.

[tormeh]

Have also revised this

[Selene-Amanita]

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.

[Selene-Amanita]

After #8841 (comment) and #8841 (comment) are handled this PR looks good to me.

It needs to be followed by another PR to handle the case where the projection Matrix isn't defined yet, though.

[tormeh]

After #8841 (comment) and #8841 (comment) are handled this PR looks good to me.

Done. Also rebased on main. I suppose you squash it on merge?

[Selene-Amanita]

Looks good to me!

@tormeh Just so you know, the reason CI fails is because there is a new clippy lint in Rust 1.71.0, released today, it has nothing to do with your code, you need to wait for this PR to be merged and then merge to your branch: #9144

[tormeh]

Looks good to me!

@tormeh Just so you know, the reason CI fails is because there is a new clippy lint in Rust 1.71.0, released today, it has nothing to do with your code, you need to wait for this PR to be merged and then merge to your branch: #9144

Wow, you guys are responsive! Anyway, yeah I figured. Thanks for telling me, though.

[alice-i-cecile]

Thanks @tormeh :D Yeah, this will get squashed on merge, which is very convenient for PR authors.

[mockersf]

This PR needs to be rebased before it can be merged