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.

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

Fix Camera3D::project_position() when depth == zfar #98489

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Flarkk
Copy link
Contributor

@Flarkk Flarkk commented Oct 24, 2024

Fixes #95786.
Edit : Fixes #46010

This PR computes the viewport half-extents directly by finding the intersection of the camera top and right planes with a camera-facing plane positioned at the user provided depth.

Without this PR, a second projection matrix was built from the camera projection by overriding its near distance with the user provided depth (Projection cm = _get_camera_projection(p_z_depth);).
When p_z_depth equals the camera far distance, the near and far distances become equal and :

This PR also updates unit tests of class Camera3D to add the case where the provided depth = zfar.

Note :

After this PR, Camera3D::_get_camera_projection(real_t p_near) is now exclusively called with its own camera's near value across the code (this PR removes the one and only case it was called with an user provided p_near).

It could be worth considering removing this function's parameter for the sake of code simplicity.
Note that it's a protected function member which means that it could be theoritically used in user modules by classes inheriting Camera3D.
I can definitely make this change in this PR or in another one if there is a consensus on making this small breaking change.

@Flarkk Flarkk requested review from a team as code owners October 24, 2024 10:32
@AThousandShips AThousandShips added bug topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 24, 2024
@AThousandShips AThousandShips modified the milestones: 4.x, 4.4 Oct 24, 2024
@Flarkk Flarkk force-pushed the fix_project_position branch from df2c085 to 3e731da Compare October 24, 2024 10:38
@Flarkk
Copy link
Contributor Author

Flarkk commented Oct 24, 2024

As a side note, I just realized that XRCamera3D::project_position() is likely doing it wrong too, for different reasons (the user-provided depth does not currently influence the final x and y components of the projected point).

https://github.com/godotengine/godot/blob/ff9fb0abea2027c35f0a024297c780648cc806bc/scene/3d/xr_nodes.cpp#L149C1-L177C3

The first thing to do would be to write unit tests for XRCamera3D too.

@Flarkk Flarkk force-pushed the fix_project_position branch from 3e731da to 771e561 Compare October 24, 2024 20:36
@Flarkk Flarkk requested a review from clayjohn November 26, 2024 11:40
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I'm approving as the code makes sense to me and I trust the tests. I'll admit though, without doing a deep dive into the geometry I'm at a loss why this works better than the old code.

@clayjohn clayjohn removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 27, 2024
@Flarkk
Copy link
Contributor Author

Flarkk commented Nov 27, 2024

For future reference, the key change is really this :

- Projection cm = _get_camera_projection(p_z_depth);
+ Projection cm = _get_camera_projection(_near);

It avoids using a modified camera projection where the near plane is replaced by the user input p_z_depth.
This produces a degenerate frustum when p_z_depth == _far and breaks further steps.

The rest just replaces the call to Projection::get_viewport_half_extents() with the unpacked logic of this function. This allows taking a plane (z_slice) as the input.

Hope it clarifies.


Note : Camera3D::_get_camera_projection(real_t p_near) really confuses me. It doesn't only returns the camera's projection as it says, but also adjusts the near plane to the user input.

As it's not called anywhere else after this PR but with the current camera's near plane as an argument (which defeats the purpose of having an argument at all ...), I'd take this opportunity to change the definition and completely remove the argument.

It's a very small breaking change - as this function is protected and may be used in user modules - but worth it IMO.

@Repiteo Repiteo merged commit 3edf8b9 into godotengine:master Nov 27, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 27, 2024

Thanks! Congratulations on your first merged contribution! 🎉

@Flarkk Flarkk deleted the fix_project_position branch November 27, 2024 20:16
@akien-mga akien-mga changed the title Fix Camera3D::project_position() when depth=zfar Fix Camera3D::project_position() when depth == zfar Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:3d
Projects
None yet
4 participants