-
-
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 (2) #76550
Fix voxel GI issues (2) #76550
Conversation
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.
Looks good!
It might be worth checking whether this resolves #55334. (The issue can be reproduced in https://github.com/Calinou/godot-reflection/tree/voxelgi if you want to check there.) |
Does it make sense to combine both? |
In some situations, yes. For instance, if you want fully real-time GI in a specific area of the world, but want to use SDFGI for general open world GI. (SDFGI isn't fully real-time as it lacks support for dynamic occluders and emissive objects.) |
I've included another fix in the commit; namely, initializers added to some fields of Without this extra change, I was being able to reproduce #76243.* And, considering the kind of fix, @Calinou's hypothesis about optimization levels playing a role makes full sense. Without initializers, garbage data was reaching the renderer and, if certain values happened to be Inf or Nan, related computations delivered wrong values. Specifically, the computation of godot/servers/rendering/renderer_rd/shaders/environment/volumetric_fog_process.glsl Lines 385 to 428 in 2a0aef5
*: Interestingly enough, I could only see this specific issue in the D3D12 renderer. That made me think it was a D3D12-specific issue, but "happily" it turned out to be a menace for the whole rendering infrastructure. |
I think this is a bug I introduced when adding either Volumetric Fog Energy or Shadow Opacity properties to Light3D. Sorry for the inconvenience 🙁 |
Thanks! 🎉 Merged. |
Cherry-picked for 4.0.3. |
Follow up of #76437, in the spirit of what I suggested here: #76437 (comment).
Fixes additional manifestations of #76243 when other effects are toggled (I've experienced, and confirmed no longer an issue with this PR, with SSAO). It may also fix other still not caught bugs.