-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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 voxel GI issues #76437
Fix voxel GI issues #76437
Conversation
@@ -3493,6 +3493,9 @@ void GI::init(SkyRD *p_sky) { | |||
if (RendererSceneRenderRD::get_singleton()->is_vrs_supported()) { | |||
defines += "\n#define USE_VRS\n"; | |||
} | |||
if (!RD::get_singleton()->sampler_is_format_supported_for_filter(RD::DATA_FORMAT_R8G8_UINT, RD::SAMPLER_FILTER_LINEAR)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying the format here feels too hard-coded to my taste, but I couldn't find a way of querying the format of the actual texture. This is not the end of the world, but has a risk of getting out of sync with the code that creates the texture.
Does this fix #76243? I thought it was due to compiler optimization levels but that may have been a fluke. |
Oh, that's exactly what one of the commits addresses! Not completely sure if its the same root cause or it's instead a different bug that looks like the same. I'm being optimistic and choosing to believe it's the same, and so updating the PR description. |
432387c
to
72dd292
Compare
72dd292
to
7d1a86a
Compare
Hmm... I've been able to reproduce the fog glitch once. I'll investigate a bit more before concluding this PR doesn't fix it. |
I think I've found the glitched fog issue! Will finish updating the PR tomorrow. Tagging as Needs work. |
7d1a86a
to
7e3f1dd
Compare
7e3f1dd
to
371c535
Compare
OK. I finally think this is good for reviewing. And fixes the flood-of-white-or-black fog issue! I've overhauled the PR description. |
@@ -3695,14 +3698,16 @@ void GI::setup_voxel_gi_instances(RenderDataRD *p_render_data, Ref<RenderSceneBu | |||
if (p_render_buffers->has_custom_data(RB_SCOPE_FOG)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this block of checks would belong better to just before using the uniform sets. They will be invalid by that time because of deletion of dependencies, so doing the check there is to me more correct. The idea would be being reactive to what actually happened instead of being proactive in guessing what may have happened. That, of course, without detriment of being proactive as an optimization in cases where that can save some checks (a bit like this PR does by assuming the whole group of sets is either valid or invalid).
In any case, this is a suggestion to the rendering maintainers. I didn't want to attempt something like that in this PR, which would broaden its scope needlessly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. There isn't much of a reason to be caching this so early
For the records, I've still seen the floody fog glitch once even with this changes, in the D3D12 renderer. But I believe this PR fixes it like 99% since without it I've been able to reproduce the issue consistently if I force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good as a minimalistic approach to fixing the issues. Indeed clearing those uniform sets should be moved out of the gi_setup code as it creates a weird dependency between the two classes.
371c535
to
09aa1bb
Compare
Thanks! |
Cherry-picked for 4.0.3. |
UPDATE: To avoid multiple 'update' notices throughout, just know that the full text has been revised after enhancing the fixes.
Changes kept in two commits because, even if related, they address different issues:
Commit 1: Fix breakages of volumetric fog on voxel GI changes
Fixes the book-keeping of related uniforms. Without this change, some of them could fail to be created when needed and some others could be pointing to outdated voxel GI buffers. For the uniform sets related to GI instances, a minor sets group idiom is introduced, that helps tracking validity and saves some checks, while also assisting developers in detecting bugs.
Fixes Vulkan: Volumetric Fog breaks when number of visible VoxelGI nodes changes #69565.
Fixes Vulkan: Volumetric fog glitches out in builds that use
optimize=none
, but not other optimization levels #76243.Commit 2: Fix unsupported sampler filter used for voxel GI
Fixes a validation error* about an R8G8 texture being sampled linearly when the driver reports no support for that.
*: