Cleanup and improve sky render #70253

BastiaanOlij · 2022-12-18T12:44:50Z

This PR fixes a number of issues with the sky rendering and moves to some of the newer abilities we added:

Renames a few ambiguous method names to make it easier to debug code.
Unifies the two implementations of draw (now draw_sky) so we only have one implementation to manage.
Moves updating half and quarter buffers to before we start our opaque render (this was already done in the mobile renderer, now clustered renderer does the same).
Now uses our new render buffers implementation to manage the half and quarter buffers.
Using half and quarter buffers now also works in stereoscopic rendering
Uses full unprojection and eye offset adjustment when rendering in multiview, due to us embedding the eye offset (both rotational and positional) into the projection matrix the short hand unprojection method fails and resulted in minor discomfort on Index headsets, and a highly noticeable wobbling effect of the sky when looking around on an Meta Quest.
Reflection probe now uses RenderSceneBuffersRD to handle helper buffers. This can be further improved by moving the logic for creating all buffers used here. This would further clean up code currently duplicated between our reflection probe logic and SkyRD::update_radiance_buffers

Still todo:

Look into using the Z buffer for fog in stereoscopic view, we may need to add our eye_offset to this

Bugsquad edit:

Fixes VR XR PanoramaSkyMaterial rendering problem #62738

servers/rendering/renderer_rd/environment/sky.cpp

BastiaanOlij · 2022-12-18T12:54:06Z

servers/rendering/renderer_rd/environment/sky.cpp

+	ERR_FAIL_COND(p_render_buffers.is_null());
+
+	// make sure we support our view count
+	ERR_FAIL_COND(p_view_count == 0);


I wonder if we should actually return a true/false from setup_sky, then we can do our checks in one place and prevent the rest from running on failure. Right now it is possibly we run other stages of sky rendering if setup fails.

Thats a good idea, we could likely set draw_sky to false if setup_sky fails which would avoid the other operations. Thats something for another PR though as we would need to think about what kind of fallback information we would need to provide in that case (clear color etc.).

BastiaanOlij · 2022-12-21T00:27:23Z

Fix sky orientation not being applied when rendering radiance buffers

This turns out not to be needed, updating radiance buffers for Sky is the exception where we do not pre-apply the sky orientation, instead the sky-orientation is applied when the radiance is read in the scene shader. This prevents the need for costly updating of the radiance map just because the sky orientation changes.

BastiaanOlij · 2022-12-21T01:54:00Z

The big change in this new commit is using render buffers for reflection probes. This way there is no need for duplicating logic in Sky to handle buffers differently between scene rendering and reflection probe rendering.

There is a good argument for embedding the full reflection probe atlas logic into render buffers as a lot of code is duplicated for this in Sky to make update_radiance_buffers work and it would allow us to further cleanup our render logic as it can now check at the start whether render buffers are supplied and just roll with it. That however is for a future PR I think.

I'm putting this ready for review, I still need to check if the Z-buffer in stereoscopic is working as expected but that is not critical to what this PR is about and if it is broken, it was already broken.

servers/rendering/renderer_rd/storage_rd/light_storage.cpp

servers/rendering/renderer_rd/environment/sky.cpp

clayjohn

Some initial comments

servers/rendering/renderer_rd/environment/sky.cpp

clayjohn · 2022-12-22T22:30:06Z

servers/rendering/renderer_rd/environment/sky.cpp

 			}
 		}
 		uniforms.push_back(u);
 	}

-	texture_uniform_sets[p_version] = RD::get_singleton()->uniform_set_create(uniforms, p_default_shader_rd, SKY_SET_TEXTURES);
-	return texture_uniform_sets[p_version];
+	return RD::get_singleton()->uniform_set_create(uniforms, p_default_shader_rd, SKY_SET_TEXTURES);


Why are we no longer caching the uniform sets? Since we aren't using the uniform set cache here we will end up producing a new uniform set each time this is called

How did I end up with that, it's supposed to be calling the framebuffer cache, I'll fix it up

Ok should be correct now.

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp

servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp

servers/rendering/renderer_rd/storage_rd/light_storage.cpp

BastiaanOlij · 2022-12-23T05:47:06Z

No, the atlas should manage freeing the textures and framebuffers. It allocates them all at program start and then frees them all at the end. This is just for other resources taking ownership of a single layer of the texture array.

Hmm, that makes sense though I don't think they are freed at the end, I could be missing the code somewhere but it looks like they are only freed if the atlas size changes?

clayjohn · 2022-12-23T06:20:20Z

No, the atlas should manage freeing the textures and framebuffers. It allocates them all at program start and then frees them all at the end. This is just for other resources taking ownership of a single layer of the texture array.

Hmm, that makes sense though I don't think they are freed at the end, I could be missing the code somewhere but it looks like they are only freed if the atlas size changes?

At the end the atlas size is set to 0 to free it

BastiaanOlij · 2022-12-23T08:49:19Z

At the end the atlas size is set to 0 to free it

Mixed up sky and reflection probe, shows even more than it makes sense to move this into our buffers object.
Anyway found it in LightStorage::reflection_atlas_free, indeed calls reflection_atlas_set_size to set it to 0 first.

clayjohn

Looks good! I think your remaining ideas would be good to do in future PRs. This already is getting quite large

akien-mga · 2022-12-23T22:45:41Z

Thanks!

BastiaanOlij added bug topic:rendering topic:xr topic:3d labels Dec 18, 2022

BastiaanOlij added this to the 4.0 milestone Dec 18, 2022

BastiaanOlij self-assigned this Dec 18, 2022

BastiaanOlij force-pushed the cleanup_sky_render branch 2 times, most recently from bdd1521 to 92dd9e5 Compare December 18, 2022 12:52

BastiaanOlij commented Dec 18, 2022

View reviewed changes

servers/rendering/renderer_rd/environment/sky.cpp Outdated Show resolved Hide resolved

BastiaanOlij commented Dec 18, 2022

View reviewed changes

This was referenced Dec 18, 2022

Use projection matrix without offset #62948

Closed

VR XR PanoramaSkyMaterial rendering problem #62738

Closed

BastiaanOlij force-pushed the cleanup_sky_render branch from 92dd9e5 to 6db3c67 Compare December 18, 2022 12:59

BastiaanOlij mentioned this pull request Dec 18, 2022

[TRACKER] Render reorganisation #67600

Open

BastiaanOlij force-pushed the cleanup_sky_render branch 2 times, most recently from 0c28686 to a17a91f Compare December 20, 2022 23:54

BastiaanOlij marked this pull request as ready for review December 21, 2022 01:54

BastiaanOlij requested a review from a team as a code owner December 21, 2022 01:54

BastiaanOlij commented Dec 21, 2022

View reviewed changes

servers/rendering/renderer_rd/storage_rd/light_storage.cpp Show resolved Hide resolved

BastiaanOlij force-pushed the cleanup_sky_render branch from a17a91f to 6b41d7b Compare December 21, 2022 01:56

BastiaanOlij commented Dec 21, 2022

View reviewed changes

servers/rendering/renderer_rd/environment/sky.cpp Show resolved Hide resolved

clayjohn reviewed Dec 22, 2022

View reviewed changes

BastiaanOlij force-pushed the cleanup_sky_render branch from 6b41d7b to b772488 Compare December 23, 2022 05:42

Cleanup and improve sky render

e886a7a

BastiaanOlij force-pushed the cleanup_sky_render branch from b772488 to e886a7a Compare December 23, 2022 08:51

clayjohn approved these changes Dec 23, 2022

View reviewed changes

akien-mga merged commit f064898 into godotengine:master Dec 23, 2022

BastiaanOlij deleted the cleanup_sky_render branch December 24, 2022 02:12

jtnicholl mentioned this pull request Dec 24, 2022

Scenes with both sky panoramas and ReflectionProbes print "test" 100+ times alongside thousands of errors #70532

Closed

BastiaanOlij mentioned this pull request Dec 29, 2022

Complete converting effects to use new render buffers where applicable #67602

Open

9 tasks

clayjohn mentioned this pull request Jan 30, 2023

Texture Array Sky Reflections Project Setting Causes Crash #71444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup and improve sky render #70253

Cleanup and improve sky render #70253

BastiaanOlij commented Dec 18, 2022 •

edited

BastiaanOlij Dec 18, 2022

clayjohn Dec 23, 2022

BastiaanOlij commented Dec 21, 2022

BastiaanOlij commented Dec 21, 2022

clayjohn left a comment

clayjohn Dec 22, 2022

BastiaanOlij Dec 22, 2022

BastiaanOlij Dec 23, 2022

BastiaanOlij commented Dec 23, 2022

clayjohn commented Dec 23, 2022

BastiaanOlij commented Dec 23, 2022

clayjohn left a comment

akien-mga commented Dec 23, 2022

Cleanup and improve sky render #70253

Cleanup and improve sky render #70253

Conversation

BastiaanOlij commented Dec 18, 2022 • edited

BastiaanOlij Dec 18, 2022

Choose a reason for hiding this comment

clayjohn Dec 23, 2022

Choose a reason for hiding this comment

BastiaanOlij commented Dec 21, 2022

BastiaanOlij commented Dec 21, 2022

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn Dec 22, 2022

Choose a reason for hiding this comment

BastiaanOlij Dec 22, 2022

Choose a reason for hiding this comment

BastiaanOlij Dec 23, 2022

Choose a reason for hiding this comment

BastiaanOlij commented Dec 23, 2022

clayjohn commented Dec 23, 2022

BastiaanOlij commented Dec 23, 2022

clayjohn left a comment

Choose a reason for hiding this comment

akien-mga commented Dec 23, 2022

BastiaanOlij commented Dec 18, 2022 •

edited