Skip to content

Add incremental update mode to sky#40272

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
clayjohn:VULKAN-time-slicing
Jul 11, 2020
Merged

Add incremental update mode to sky#40272
akien-mga merged 1 commit into
godotengine:masterfrom
clayjohn:VULKAN-time-slicing

Conversation

@clayjohn
Copy link
Copy Markdown
Member

This adds two new process_modes to Sky.

Automatic - selects the appropriate process mode based on shader usage. This is the new default. Ideally, users will be able to leave it on automatic and never select a difference mode.

High Quality Incremental - This is the same as High Quality, except it updates over 8 frames (by default). It processes each roughness layer in a frame to smooth out the cost of importance sampling all the layers. On high-end devices 8 frames will feel almost instant. Most importantly, this mode is used by AUTOMATIC when a light is used in the sky shader. So adding this mode will fix users have hiccups as they rotate the sun in the editor.

I experimented with only updating 1 face per frame (updates over 48 frames). But the update delay was way more noticeable and the frame time reduction was not worth it.

This issue hasn't been reported in the bug tracker, but I have seen it mentioned a few times on IRC, Discord and Twitter

@clayjohn clayjohn added this to the 4.0 milestone Jul 11, 2020
@clayjohn clayjohn requested a review from reduz July 11, 2020 00:49
@clayjohn clayjohn requested a review from a team as a code owner July 11, 2020 00:49
@clayjohn clayjohn force-pushed the VULKAN-time-slicing branch from d32e73d to a54f93c Compare July 11, 2020 07:10
@akien-mga akien-mga merged commit 9678a41 into godotengine:master Jul 11, 2020
@akien-mga
Copy link
Copy Markdown
Member

Thanks!

@Calinou
Copy link
Copy Markdown
Member

Calinou commented Dec 14, 2021

Is the High Quality mode still relevant when we have High Quality Incremental? I can't see any upsides to it. ReflectionProbes only have an equivalent of High Quality Incremental (the Once update mode), not High Quality (ReflectionProbe's Always update mode is comparable to RealTime).

@clayjohn clayjohn deleted the VULKAN-time-slicing branch December 14, 2021 17:34
@clayjohn
Copy link
Copy Markdown
Member Author

@Calinou High Quality incremental needs several frames to converge. High Quality mode is preferable when you want the reflections to be immediately available on load.

@Calinou
Copy link
Copy Markdown
Member

Calinou commented Dec 14, 2021

High Quality mode is preferable when you want the reflections to be immediately available on load.

There is a special case to ensure that the sky is available as soon as possible when using the High Quality Incremental update mode. Doesn't that suffice?

if (sky->processing_layer == 0 && sky_mode == RS::SKY_MODE_INCREMENTAL) {
// On the first frame after creating sky, rebuild in single frame
update_single_frame = true;
sky_mode = RS::SKY_MODE_QUALITY;
}

When updating the sky at run-time, I'm don't think having a really low framerate is a better experience over having parts of the sky being slightly out of date.

@clayjohn
Copy link
Copy Markdown
Member Author

There is a special case to ensure that the sky is available as soon as possible when using the High Quality Incremental update mode. Doesn't that suffice?

Not really, that code only runs when the sky resource has been recreated from scratch. It doesn't apply when, for example, the user changes the sky material during a scene transition or loading screen etc.

When updating the sky at run-time, I'm don't think having a really low framerate is a better experience over having parts of the sky being slightly out of date.

I tend to agree with that. But that doesn't convince me that we should get rid of the Update once mode.

@Calinou
Copy link
Copy Markdown
Member

Calinou commented Dec 14, 2021

Not really, that code only runs when the sky resource has been recreated from scratch. It doesn't apply when, for example, the user changes the sky material during a scene transition or loading screen etc.

When I tested #55933, I noticed that swapping the sky resource for a new one forced the sky to update. So I think it does work when you clear or reassign the sky resource entirely – but it won't trigger when you change a property within the sky resource.

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.

4 participants