Detect more pipeline settings at load time to avoid pipeline stutters

[clayjohn]

This fixes an unreported bug that created pipeline stutters at run time.

Background

We can't compile every pipeline variant at load time for the ubershader. There are too many. So we detect which ones we know we need and then only compile those. This has the bonus of reducing load times for simple games that only need a few variants.

However, when a feature that needs a new variant is added at run time, we have to go back and compile the missing variant for all existing pipelines. In bigger games this can take a long time.

This is why the docs say to avoid adding things like VoxelGI nodes to the SceneTree at run time.

The problem

Using SSS or the normal_roughness_texture can trigger a new variant being added. What makes these features unique is that they were only being checked if an object that survived culling made it into the render pass. This means that you can trigger pipeline recompilations by looking around.

The solution

We can set the pipeline state when the material is assigned to the object instead of setting it when the object is first visible. This moves the cost to load time and the stutter is gone. Now the usual advice applies, just ensure that any material that uses SSS or the normal_roughness_texture is present in the SceneTree at load time and you will be fine.

Edit

The new approach is basically the same, but now checks the screen textures as well and allocates textures up front too to avoid stuttering on texture creation. On desktops, texture creation isn't really noticeable, but on mobile devices it can lead to noticeable stutter.

Fixes: #101554

[DarioSamo]

Makes sense!

[DarioSamo]

I changed my mind slightly about how we should handle this. While this solves pipeline stutters, it won't solve any other stutters caused by screen buffer creation when the element suddenly appears. Something about where this was placed breaks the regular flow of where this behavior is usually handled.

I think what we need is some sort of way to build a default scene state based on tracking what geometry surfaces were created (and also deleted). Said default values would be grabbed here instead of always false:

godot/servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp

Lines 881 to 885 in a9c7578

	scene_state.used_sss = false;
	scene_state.used_screen_texture = false;
	scene_state.used_normal_texture = false;
	scene_state.used_depth_texture = false;
	scene_state.used_lightmap = false;

That way, we can avoid the current stutters that are caused by the sudden creation of resources when something that uses the effect comes into view.

[DarioSamo]

The idea for handling this in a way that is based of the surface geometry is pretty straightforward. Both surface creation and surface deletion must call the same function with an argument to increase or decrease counters.

// adjust_surface_counters must be called after creating a surface and before deleting one.

void adjust_surface_counters(Surface *p_surface, bool p_increase) {
  int counter_change = p_increase ? 1 : -1;
  if (p_surface->flags & USES_SSS) {
    surfaces_state.subsurface_scattering += counter_change;
  }

  if (p_surface->flags & USES_NORMAL_TEXTURE) {
    surfaces_state.normal_textures += counter_change;
  }
}

Then the above code can simply do:

scene_state.used_sss = surfaces_state.subsurface_scattering > 0; 
 scene_state.used_normal_texture = surfaces_state.normal_textures > 0; 

And consequently, this will also enable the pipeline data and create the textures necessary, eliminating both sources of stutter that can happen if either of these elements are out of view.

out of date

[clayjohn]

@DarioSamo That's a good idea. It will solve #101554 as well

[clayjohn]

Then the above code can simply do:

scene_state.used_sss = surfaces_state.subsurface_scattering > 0; 
 scene_state.used_normal_texture = surfaces_state.normal_textures > 0; 

And consequently, this will also enable the pipeline data and create the textures necessary, eliminating both sources of stutter that can happen if either of these elements are out of view.

It turned out not to be this simple lol. These can trigger behaviour that we only want in frames where it is necessary (like resolving the depth texture etc.). Instead we need to trigger resource + pipeline creation based on the surface data and then still check what is visible per frame for the scene_state. A bit more complex unfortunately.

[clayjohn]

Updated! In the end it didn't make sense to track the absolute numbers as all of these resources are created when needed and not freed.

I originally incremented the counter for each usage and then used the counter to initialize the scene_state variables, e.g.

scene_state.used_sss = surfaces_state.subsurface_scattering > 0; 

However, that broke rendering in a couple of subtle ways because we were doing things like texture copies unnecessarily.

What I ended up with I think is actually a little bit simpler and should scale well if other resources need this treatment

[DarioSamo]

Looks good now!

Would it be a good idea to perhaps fail or assert for the existence of the screen and depth textures respectively in the copy() methods now that the "ensure" behavior was moved out of it? Just to catch any errors that could happen from calling one function without having called the ensure previously.

No more suggestions from me besides that.

[Repiteo]

Wait, it didn't count this as merged? That's… Weird. Oh well, for all intents and purposes it is merged, so I'll pretend it worked without a hitch

Thanks!

[clayjohn]

@Repiteo This didn't get merged, an old commit that is out of date is somehow merged

See the difference between this PR and the commit that got merged

665bdf4 (merged)

bf47c42 (actual commit)

[Repiteo]

First time I've seen something like that happen! I'll revert that specific commit and explicitly merge this one after

[Repiteo]

Thanks! (again!)