Fix Camera3D project_* methods not accounting for frustum offset

[bcmpinc]

I extracted the duplicated code into a new function Camera3D::_get_camera_projection(p_near) and then added support for the PROJECTION_FRUSTUM mode. This partially addresses #61174.

Additionally I added Camera3D::get_camera_projection() which returns the Camera's projection matrix, but is currently unused. I think it is useful to expose that method to GDScript as well (currently there's no practical way to get the camera's projection matrix). How would I do that? Should this method be virtual (just like get_camera_transform())? and added GDScript bindings.

I still have to add unit tests, probably merge some of the commits and improve the commit messages. Is there anything else that I need to do for this PR to be accepted?

[bcmpinc]

I noticed it currently only works properly if the camera size is set to 1.

Edit: project_ray_origin and project_ray_normal don't use the projection matrix, so those did not get fixed automatically. I've fixed project_ray_origin in the commit below.

[bcmpinc]

So to fix project_ray_normal I'd plan to use the projection matrix, as that makes the code a lot easier and work independently of what projection method is used.

I would like to give project_ray_origin the same treatment, however that would change how that method works for perspective. Instead of returning the position of the camera, it would return the point on the near-plane of the camera's frustum. This would be more in line with its documentation though:

Vector3 project_ray_origin ( Vector2 screen_point ) const
Returns a 3D position in world space, that is the result of projecting a point on the Viewport rectangle by the inverse camera projection. This is useful for casting rays in the form of (origin, normal) for object intersection or picking.

This relates to issue #41872 solving that issue in a different manner.

The documentation of the ray-tracing tutorial would also need to be updated.

[YuriSizov]

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

[bcmpinc]

I've squashed the commits. It fixes the issues with the project_* methods, except for project_ray_normal.

I'm currently not working on a fix for project_ray_origin, so if someone else wants to give that a try then leave a comment here.

[akien-mga]

There seems to be two issues remaining:

• The methods should be ordered alphabetically in the XML, so the new get_camera_projection should go before get_camera_rid.
• The test project we run on CI is crashing when calling Camera3D.new().get_camera_projection() (get_viewport() is a nullptr until properly set up).

[bcmpinc]

I've added a condition check to ensure that the Camera3D is part of the scene tree. I've moved the documentation entry in the XML file and added information about it requiring to be part of the scene tree.

[YuriSizov]

Superficially this makes sense and the clean up is nice, but I'm no expert. cc @Calinou @clayjohn

[Calinou]

I've tested this PR locally (rebased against master) but the original issue with the MRP from #61174 remains:

The red cross denotes my click position, similar to the MRP.

The MRP uses project_ray_origin() and project_ray_normal() though, so I assume this is expected given the PR's description.

The expected result (and the one I got with perspective projection, orthographic projection and frustum projection with no offset) is that the green cylinder (that indicates the world position gotten from the conversion) should point to the box in the back, but instead it points at the wall.

The logic of the PR itself makes sense to me, and exposing get_camera_projection() will surely be appreciated by many 🙂

[YuriSizov]

The MRP uses project_ray_origin() and project_ray_normal() though, so I assume this is expected given the PR's description.

So our recommendation for the author of the issue would be to implement it differently after the fix is merged?

[clayjohn]

Taking a look at the code and running locally. The code changes seem fine and appear to be an improvement, however, the problem identified in #61174 remains and the MRP is not fixed. So there must be an additional change needed somewhere to make this complete.

[YuriSizov]

@clayjohn I think this is what Calinou points to?

[clayjohn]

@clayjohn I think this is what Calinou points to?

yep

[bcmpinc]

The MRP relies on project_ray_origin() and project_ray_normal(). I did not feel confident enough to change those without unit tests. However, I ran into a lot of issues trying to get debugging to work and was unable to write those tests.

The projection matrix being exposed does allow a workaround to be written in GDScript though.

[clayjohn]

The MRP relies on project_ray_origin() and project_ray_normal(). I did not feel confident enough to change those without unit tests. However, I ran into a lot of issues trying to get debugging to work and was unable to write those tests.

The projection matrix being exposed does allow a workaround to be written in GDScript though.

Thanks for the clarification. That makes sense. I was a bit confused because project_ray_normal() is fixed somewhat by switching to using set_frustum() instead of always set_perspective() but it seems that isn't enough to fix the MRP.

What sort of issues were you running into? Maybe we can help you get unit tests working so you can take the next step in fixing the MRP. If you aren't interested in taking this further, that is also understandable. I think we can go ahead and merge as is without closing the original bug report.

[akien-mga]

Thanks!

[YuriSizov]

Cherry-picked for 4.1.1.