Skip to content
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

Clear SDFGI textures when created #80889

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Aug 22, 2023

Related discussion: #39961 (comment)

Note that visual artifacts also happens when changing some SDFGI parameters, not just when it's first created.

Clears all SDFGI textures when they are created to avoid visual artifacts from uninitialized data. This is a shotgun approach to clear everything, I tried to find individual textures responsible for that glitching, but I couldn't isolate any single confirmed culprit. The existing code does clear a few textures already, but as the bug report shows it doesn't seem to be enough. Someone more familiar with this code might figure out if there is a more limited set of textures that have to be cleared.

The issue itself is pretty easy to repro even with a simple scene, toggling environment SDFGI on and off and adjusting its parameters results in those visual glitches pretty often. Resizing the window and tweaking the parameter also seems to also show the issue quickly. After this PR, I cant't repro the issue anymore.

Also changes texture debug names from VoxelGI to SDFGI, looks like a mistake to me (and also to the person who wrote the bug report).

@bitsawer bitsawer added this to the 4.2 milestone Aug 22, 2023
@bitsawer bitsawer requested a review from a team as a code owner August 22, 2023 12:36
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (Linux + NVIDIA), it works as expected.

In a future PR, I wonder if we initialize SDFGI color to approximately match the environment color when Read Sky Light is enabled. This makes the transition when toggling SDFGI less jarring, which is nice to have when initially loading a scene (so you don't have to hide the transition as much).

@jcostello
Copy link
Contributor

Tested locally (Linux + NVIDIA), it works as expected.

In a future PR, I wonder if we initialize SDFGI color to approximately match the environment color when Read Sky Light is enabled. This makes the transition when toggling SDFGI less jarring, which is nice to have when initially loading a scene (so you don't have to hide the transition as much).

A signal when finish could fix when loading a scene

@bitsawer
Copy link
Member Author

bitsawer commented Aug 24, 2023

It would be interesting to test the skylight color tuning or even instead of starting from 0 initialized values try to converge from old values if possible. But the latter is kind of #39961 territory.

Not sure if SDFGI ready signal is needed, I assume that you can just wait Project Settings Frames To Converge amount of frames and it should be ready. Not sure though, haven't tested it.

However, one useful thing would be to force converge the SDFGI when loading a scene instead of sitting and waiting until enough frames have gone by. A basic experiment with RenderingServer.force_draw() like this doesn't seem to work (at least in editor mode), but maybe there is some other way;

node.environment.sdfgi_enabled = true

for i in range(30): #Or whatever your SDFGI convergence setting is
    RenderingServer.force_sync()
    RenderingServer.force_draw(true, 0.1)

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just left one little comment

servers/rendering/renderer_rd/environment/gi.cpp Outdated Show resolved Hide resolved
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I also tested locally and this appears to work well

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 28, 2023
@akien-mga akien-mga merged commit 22b7fca into godotengine:master Aug 28, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling SDFGI for the first time causes artefacts due to corrupted textures
6 participants