Fix Frustum Sky projection translation logic shearing

[EnlightenedOne]

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]

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.

[Chaosus]

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

[EnlightenedOne]

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

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

[QbieShay]

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]

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]

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]

@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]

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]

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]

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.

[EnlightenedOne]

@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]

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

[EnlightenedOne]

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

[clayjohn]

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

[Repiteo]

Thanks! Congratulations on your first contribution! 🎉

[EnlightenedOne]

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