Fix Frustum Sky projection translation logic shearing #86138

EnlightenedOne · 2023-12-14T01:09:06Z

Found a bug dealing with frustum sky in planar reflections.
Identified that camera frustum was responsible and that existing defect had been created.
Tested using MRP on the bug ticket fix works for compatibility/mobile/forward+.
The fix is based on how reflection probes work and aims to be lightest touch.
Tests passing but no new unit tests written.

Extra bonus picture (the floor clouds are Frustum projected):

Angle shot showing sky has moved as expected from MRP

Fixes #63863 .

EnlightenedOne · 2023-12-14T12:00:36Z

FYI I added the commit hooks to my local system before the second commit, it should be all good now. I rebased as recommended to bury my bad commits.

drivers/gles3/rasterizer_scene_gles3.cpp

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp

servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp

Calinou

Tested locally, it works as expected. The fix makes sense to me, although it would benefit from a second look just to make sure.

simplescreenrecorder-2023-12-15_14.26.20.mp4

EnlightenedOne · 2024-07-01T21:21:16Z

@EnlightenedOne This needs to be rebased. Just a reminder.

Updated again, please keep me posted if I can do anything to help expedite.

QbieShay · 2024-07-09T14:22:40Z

Hey,

We've reviewed this PR and we're a bit concerned by the double flipping of projection and view matrix. Specifically, this could work only in your usecase (where you have a horizonatal reflection plane).

This PR needs more testing to make sure it's not working only for this specific usecase.

EnlightenedOne · 2024-07-09T17:25:44Z

Happy to talk through the concern. By double flip do you mean changing the projection matrix and the transform or a further up the chain operation in the render flow? The truth is I dont know why the sky is drawn as it is, I assumed it was to do with the frustrum being flipped intentionally in engine to be inside the skybox.

I have not hacked the basis of the camera for my water surface, the mirror I use can rotate on any axis and correctly draw the sky so long as I relect my current camera's transform AND correct for the transform flaw included in this PR. You can completely ignore mirrors and focus on the frustrum camera with skybox sample stand alone. The attached minimum replication project should be sufficient to demo the solution and prove it is agnostic of any reflections.

If any of the other axis were out of alignment @Calinou's video would show unexpected rotation about another axis as the pitch and yaw changes (only roll isn't really in the demo, it was tested), the rotation of the camera about the origin shows a correction to the basis of the frustrum transform. I hope that helps but again im very happy to give more info.

EnlightenedOne · 2024-07-09T18:22:16Z

Here is evidence that you can yaw, pitch and roll the camera and have the frustrum sky draw correctly @Calinou's video is a better overall demo I didn't have a lot of time to record something after recompiling, this is done in Compatibility mode but I tested it in Mobile + Forward:

Please note that the camera in the test sample is a bit crappy but which is why I decided to rotate the thing in the editor to make the rendering accuracy clear.

clayjohn · 2024-07-09T22:08:15Z

@EnlightenedOne What would be beneficial is explaining why flipping the y-axis in both the projection and view matrices is necessary for the frustum offset camera. Intuitively, it doesn't really make sense. I think there is a good chance that this just happens to fix the problem for this MRP rather than being the correct solution.

EnlightenedOne · 2024-07-10T12:06:28Z

It is possible that the solution is suboptimal (might be an easier fix for to the matrix), I assume the components are internally consistent. The MRP for this is Godot's sky solution + frustrum cam with y offset, No scenario exists where the fix can fail as the scope is limited to that.

We are not trying to invert the image but invert the rotational influence to align the way the sky is setup with the way the frustrum is calculated. The transform scale inversion flips the rotational view data on the axis which is where the two are out of sync and the projection inversion reflips the image so the sky is drawn the right way up (trying to get to frustrum orientation to sky orientation).

I guess you are asking 'why' they are not aligned on this axis. I will dig some more and see if I can qualify it better.

EnlightenedOne · 2024-10-20T23:45:07Z

I spent some time wondering where between the frustum offset and the sky rendering things get messed up. As I understand it the sky in Godot is in a nutshell upside down. In the frustum projection creation one of the cells infers an up direction for the offset:

Based on projection math here https://songho.ca/opengl/gl_projectionmatrix.html

So the solve is simply to flip that field:
Projection projection = p_render_data->scene_data->cam_projection;
if (p_render_data->scene_data->cam_frustum) {
// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip
projection[2].y = -projection[2].y;
}

sky.setup_sky...
It seems stable on my end under all conditions, I have pushed an update to the PR and tested all renderers.

EnlightenedOne · 2024-10-21T00:18:48Z

Additional FYI I did look into getting rid of the flag for Frustum camera and flipping the projection matrix for all projections but it seemed pretty dangerous and might bite later on if you expose projection matricies to end users or add new custom projection options. My view is this is as good as the solution can be unless the sky gets flipped later.

clayjohn

We discussed this in today's rendering meeting. The approach makes sense now, thank you for looking into this further.

From the look of the code, it seems like the flip could be done when the matrix is created since it is always enabled when using frustum offset. Is there a good reason to apply the flip each frame when drawing instead of just applying it when creating the projection matrix? Doing so seems more foolproof.

EnlightenedOne · 2024-10-24T07:34:04Z

@clayjohn the projection matrix is shared between the regular right way up rendering and the sky rendering which is upside down. If we flip it earlier the problem will be inverted, the sky will move correctly and every other thing will move with inverted y coords.

It looks like it would go against the grain of Godot's render setup to have a separate projection matrix just for the sky but I am open to suggestions.

EnlightenedOne · 2024-10-30T18:02:07Z

Hi @clayjohn

I ran an experiment with the recommended changes and several permutations tweaking the sky glsl + environment/sky.cpp to try and make it draw the same way up as the rest of the scene:
sky.glsl (- removed):
line 169 cube_normal.y = (uv_interp.y - projection.z) / projection.w;
sky.cpp:
line 1176 correction.set_depth_correction(true, true);
line 1295 correction.set_depth_correction(false);

I cleared the projects shader cache between tests.

The view frustum was corrected offset wise in the frustum but the sky rendering inverted/mirrrored the sun to the opposite position relative to the sky. The main perspective cameras view of the sky was completely broken with these changes such that the sky was both upside down and offset incorrectly relative to the angle of the camera such that the sky triangle became visible. I tried a bunch of permutations but you would have to cut deeper to fix this is my view.

It might make sense to try and clean up the sky and reorder it but it is going to require a deeper overhaul. I think a rewire needs more consideration than I can provide in a vacuum. I recommend even if viewed as a hack going for the PR as-is for a lightest touch adjustment. Sorry I couldn't get the alternate path to fly.

Thanks,
EO

clayjohn

@EnlightenedOne Thank you for trying. I suspect that we are on the right track with the changes we discussed last night. But indeed, the problem is wrapped up in how we inconsistently use the y-flip.

Let's go ahead with this for now, despite it being a bit hacky as I think we all agree that it is a fix even if its not the best one. Then we can remove this once we clean up the y-flip handling.

EnlightenedOne · 2024-11-01T22:34:42Z

That sounds good, any other things necessary for this to be merged?

clayjohn · 2024-11-02T01:44:23Z

That sounds good, any other things necessary for this to be merged?

Nope! It's in the queue to be merged next time we do a batch

AThousandShips · 2024-10-22T11:05:20Z

drivers/gles3/rasterizer_scene_gles3.cpp

 		Projection projection = render_data.cam_projection;
 		if (is_reflection_probe) {
 			Projection correction;
 			correction.columns[1][1] = -1.0;
 			projection = correction * render_data.cam_projection;
+		} else if (render_data.cam_frustum) {
+			// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip


Suggested change

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip.

Per the comment style.

AThousandShips · 2024-10-22T11:05:30Z

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp

-				sky.setup_sky(p_render_data->environment, rb, *p_render_data->lights, p_render_data->camera_attributes, p_render_data->scene_data->view_count, p_render_data->scene_data->view_projection, p_render_data->scene_data->view_eye_offset, p_render_data->scene_data->cam_transform, p_render_data->scene_data->cam_projection, screen_size, p_render_data->scene_data->taa_jitter, this);
+				Projection projection = p_render_data->scene_data->cam_projection;
+				if (p_render_data->scene_data->cam_frustum) {
+					// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip


Suggested change

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip.

AThousandShips · 2024-10-22T11:05:37Z

servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp

-				sky.setup_sky(p_render_data->environment, p_render_data->render_buffers, *p_render_data->lights, p_render_data->camera_attributes, p_render_data->scene_data->view_count, p_render_data->scene_data->view_projection, p_render_data->scene_data->view_eye_offset, p_render_data->scene_data->cam_transform, p_render_data->scene_data->cam_projection, screen_size, p_render_data->scene_data->taa_jitter, this);
+				Projection projection = p_render_data->scene_data->cam_projection;
+				if (p_render_data->scene_data->cam_frustum) {
+					// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip


Suggested change

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip

// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip.

AThousandShips · 2024-10-22T11:05:56Z

servers/rendering/renderer_rd/storage_rd/render_scene_data_rd.h

 	bool flip_y = false;
-


Unrelated, please restore as it helps readability

Thank you, done

Repiteo · 2024-11-05T03:58:08Z

Thanks! Congratulations on your first contribution! 🎉

EnlightenedOne · 2024-11-05T11:10:20Z

Thank you, it is contribution 2, fix 3 but I am very pleased to have this one in!

EnlightenedOne requested a review from a team as a code owner December 14, 2023 01:09

Calinou added bug topic:rendering topic:3d labels Dec 14, 2023

AThousandShips reviewed Dec 14, 2023

View reviewed changes

EnlightenedOne force-pushed the master branch from 5cd57e8 to 2cad084 Compare December 15, 2023 00:39

Calinou added this to the 4.3 milestone Dec 15, 2023

Calinou approved these changes Dec 15, 2023

View reviewed changes

YuriSizov requested a review from a team December 18, 2023 19:21

EnlightenedOne requested review from a team as code owners February 19, 2024 21:49

Mickeon removed the needs work label Jul 1, 2024

QbieShay modified the milestones: 4.3, 4.4 Jul 9, 2024

QbieShay added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Jul 9, 2024

Chaosus added the needs testing label Jul 9, 2024

EnlightenedOne force-pushed the master branch from 7fb26ec to 2b7b9fe Compare July 9, 2024 18:25

EnlightenedOne force-pushed the master branch from 2b7b9fe to 8412b64 Compare October 6, 2024 23:14

EnlightenedOne force-pushed the master branch from 8412b64 to 8bbc6c2 Compare October 21, 2024 00:03

clayjohn reviewed Oct 24, 2024

View reviewed changes

clayjohn approved these changes Oct 30, 2024

View reviewed changes

clayjohn removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Oct 30, 2024

AThousandShips approved these changes Nov 2, 2024

View reviewed changes

Fix Frustum Sky projection translation logic, all pipelines, fixes 63863

1d141ab

EnlightenedOne force-pushed the master branch from 8bbc6c2 to 1d141ab Compare November 2, 2024 21:35

Repiteo merged commit 88d9903 into godotengine:master Nov 5, 2024
20 checks passed

BastiaanOlij mentioned this pull request Nov 19, 2024

Sky is not rendering for right eye in VR (4.4.dev4) #99193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Frustum Sky projection translation logic shearing #86138

Fix Frustum Sky projection translation logic shearing #86138

EnlightenedOne commented Dec 14, 2023 •

edited

Loading

EnlightenedOne commented Dec 14, 2023 •

edited

Loading

Calinou left a comment

EnlightenedOne commented Jul 1, 2024

QbieShay commented Jul 9, 2024

EnlightenedOne commented Jul 9, 2024 •

edited

Loading

EnlightenedOne commented Jul 9, 2024 •

edited

Loading

clayjohn commented Jul 9, 2024

EnlightenedOne commented Jul 10, 2024

EnlightenedOne commented Oct 20, 2024 •

edited

Loading

EnlightenedOne commented Oct 21, 2024

clayjohn left a comment

EnlightenedOne commented Oct 24, 2024 •

edited

Loading

EnlightenedOne commented Oct 30, 2024

clayjohn left a comment

EnlightenedOne commented Nov 1, 2024

clayjohn commented Nov 2, 2024

AThousandShips Oct 22, 2024

EnlightenedOne Nov 2, 2024

AThousandShips Oct 22, 2024

EnlightenedOne Nov 2, 2024

AThousandShips Oct 22, 2024

EnlightenedOne Nov 2, 2024

AThousandShips Oct 22, 2024

EnlightenedOne Nov 2, 2024

Repiteo commented Nov 5, 2024

EnlightenedOne commented Nov 5, 2024

	// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip
	// Sky is drawn upside down, the frustum offset doesn't know the image is upside down so needs a flip.

Fix Frustum Sky projection translation logic shearing #86138

Fix Frustum Sky projection translation logic shearing #86138

Conversation

EnlightenedOne commented Dec 14, 2023 • edited Loading

EnlightenedOne commented Dec 14, 2023 • edited Loading

Calinou left a comment

Choose a reason for hiding this comment

EnlightenedOne commented Jul 1, 2024

QbieShay commented Jul 9, 2024

EnlightenedOne commented Jul 9, 2024 • edited Loading

EnlightenedOne commented Jul 9, 2024 • edited Loading

clayjohn commented Jul 9, 2024

EnlightenedOne commented Jul 10, 2024

EnlightenedOne commented Oct 20, 2024 • edited Loading

EnlightenedOne commented Oct 21, 2024

clayjohn left a comment

Choose a reason for hiding this comment

EnlightenedOne commented Oct 24, 2024 • edited Loading

EnlightenedOne commented Oct 30, 2024

clayjohn left a comment

Choose a reason for hiding this comment

EnlightenedOne commented Nov 1, 2024

clayjohn commented Nov 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Repiteo commented Nov 5, 2024

EnlightenedOne commented Nov 5, 2024

EnlightenedOne commented Dec 14, 2023 •

edited

Loading

EnlightenedOne commented Dec 14, 2023 •

edited

Loading

EnlightenedOne commented Jul 9, 2024 •

edited

Loading

EnlightenedOne commented Jul 9, 2024 •

edited

Loading

EnlightenedOne commented Oct 20, 2024 •

edited

Loading

EnlightenedOne commented Oct 24, 2024 •

edited

Loading