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

ShaderRD compilation groups #79606

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Jul 18, 2023

Fixes: #66998

This is a follow up to #63829, in particular to reduz' comment about a "better approach"

This PR introduces the concept of shader compile groups to the ShaderRD class. Compile groups can be used to delay compiling certain variants until some later point (or not at all). This can be used to avoid the cost of advanced shaders for features that a user may not use. Right now, this is only used by the Forward+ Scene Shader, but it can be extended to other shader types that have expensive compilation steps.

The main benefits of this system are:

  1. Reduce load time for scene shader (and decouple load time from shader variants. Thus allowing us to add more shader variants without hurting all users)
  2. Can avoid compiling advanced shaders on unsupported systems

Background

Currently the approach to shader variants is to assemble a list of variant defines out of every possible combination of settings (Multiview, Lightmaps, SSR, TAA). We have 16 combinations of settings, and 9 total possible depth prepass combinations for a total of 25 variants. The current shader allows users to selectively disable variants at initialization time, which we do for the multiview variants.

By default, this means that, for every scene shader we load, we compile all 14 variants (or 25 when using XR) even if the user will never use all of them. Further, we cache all those variants, even if never used.

Implementation

This PR creates 4 groups for the scene shader:

  1. Base (contains 4 variants: Depth, Depth+Normal, Depth + DP, Base color)
  2. Advanced (contains 10 variants: various combinations of SSR, Lightmap, TAA, and 3 additional depth modes
  3. Multiview (contains 4 variants: multiview versions of depth, multiview base)
  4. Multiview Advanced (contains 7 variants: the remaining combinations of SSR, Lightmap, TAA plus multiview)

By default, only the Base group is compiled at load time. If no advanced features are used, the others will not be compiled. The use of XR is detected at load time, so groups 3 and 4 are only ever used if XR is enabled. This results in a 3x reduction of variants needing to be compiled by default.

For scenes that make use of the advanced features, shader compile time remains the same.

shader compile shader cache
Master 7.1s 0.68s
This PR 2.7s 0.14s

Timings taken from a modified version of the Lumberyeard bistro scene (with disabled SDFGI) #74965

This PR does invalidate the current shader cache as it changes the shader cache hashes. No actions will be necessary from users, but the first start after running a version with this PR will have to recompile all shaders from scratch.

Important details for discussion

Rendering Device API addition
In order to support his work, we needed to allocate shader placeholders so that Pipelines could be allocated and reserved. This allows us to set everything up without compiling shaders, then only actually compile the shader once it is needed without setting up a dependency system to recreate all the pipelines as well.

I added RD::shader_create_placeholder() so we could allocate shader RIDs in advance. Are we happy adding this extension to the RD API?

Runtime Compilation
Right now the implementation compiles the advanced shaders at the beginning of the first frame in which the advanced feature is used. For most users, this will happen immediately before the first frame of the game.

The problem with that is it moves some of the slowness from load time (where the user can display a loading bar) to initial rendering time which will present as a freeze between loading and the game starting. While this is significantly better than the situation in Godot 3.x where shaders were compiled when first used. It still might be an issue for some games.

Ideally, the shaders would be flagged for compiling as soon as any of the features are detected at load time (i.e. when loading the Environment). However, currently the environment is created independently from the Forward renderer, so we would need to add in some sort of dependency so the Environment storage functions can notify the renderer that they want to use advanced features. I didn't like the idea of adding that complication, but it may be worthwhile to move the compilation step earlier in the load time.

@BastiaanOlij
Copy link
Contributor

Couple small things (already mentioned in chat but repeating here so it doesn't get lost).

We should remove is_xr_enabled from the system, thought that probably means we need to port this solution to the mobile renderer as well. is_xr_enabled was our old way of creating a multiview shader group, it's actually linked to a setting called "enable xr shaders".

Instead we can simply check for viewcount > 1 at the start of _render_scene and if set enable our multiview group.

My other problem, and that might be a little harder to fix, is that if we have a single viewport, viewcount > 1 is true and we're using advanced features, all shader groups will be enabled, while the only shader group that may be used is SHADER_GROUP_ADVANCED_MULTIVIEW.

@clayjohn clayjohn force-pushed the ShaderRD-compilation-groups branch from 58df4cc to cf0c269 Compare July 21, 2023 14:36
@clayjohn clayjohn requested a review from a team as a code owner July 21, 2023 14:36
@clayjohn
Copy link
Member Author

Couple small things (already mentioned in chat but repeating here so it doesn't get lost).

We should remove is_xr_enabled from the system, thought that probably means we need to port this solution to the mobile renderer as well. is_xr_enabled was our old way of creating a multiview shader group, it's actually linked to a setting called "enable xr shaders".

Instead we can simply check for viewcount > 1 at the start of _render_scene and if set enable our multiview group.

My other problem, and that might be a little harder to fix, is that if we have a single viewport, viewcount > 1 is true and we're using advanced features, all shader groups will be enabled, while the only shader group that may be used is SHADER_GROUP_ADVANCED_MULTIVIEW.

Pushed some changes to substantially address this.

  1. Removing is_xr_enabled is way outside the scope of this PR. To remove it, we need to update the shader_compiler to detect whether a given shader is being used in XR and then compile the XR-supporting variant. Right now we use is_xr_enabled for that, but we need to find something better.
  2. I added code to enable the MULTIVIEW versions when view_count > 1 regardless of is_xr_enabled. However, I left the original code that checks is_xr_enabled as compiling up front will be preferable
  3. I added an optional argument to enable_advanced_shader so that only SHADER_GROUP_ADVANCED_MULTIVIEW is enabled if using viewcount > 1. SHADER_GROUP_ADVANCED will be enabled as soon as a non-XR viewport needs it.

In my opinion, this change should be ready to merge once it passes CI

This allows us to specify a subset of variants to compile at load time and conditionally other variants later.

This works seamlessly with shader caching.

Needed to ensure that users only pay the cost for variants they use
@clayjohn clayjohn force-pushed the ShaderRD-compilation-groups branch from cf0c269 to e970f52 Compare July 21, 2023 14:42
@BastiaanOlij
Copy link
Contributor

@clayjohn looks good to me, I agree that retiring is_xr_enabled is maybe too much to ask for this PR as that would also result in requiring similar changes in the compatibility renderer. Maybe we should add a comment inside of the method that we are looking at retiring it?

I think it also makes sense if we have project settings to pre-enable each shader group. If the user knows a shader group should be included always and is willing to have the cost up front,

@YuriSizov YuriSizov merged commit 1c40263 into godotengine:master Aug 1, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@clayjohn clayjohn deleted the ShaderRD-compilation-groups branch August 1, 2023 15:58
@AThousandShips
Copy link
Member

AThousandShips commented Aug 1, 2023

This looks like it causes a lot of Attempted to free invalid ID: -9222835861727476870 errors on closedown, seems to have started at this point at least, but could be unrelated

Edit: Can confirm that this does not occur on the previously merged PR but on this one

@akien-mga akien-mga changed the title Shader rd compilation groups ShaderRD compilation groups Aug 11, 2023
@hudmarc
Copy link

hudmarc commented Aug 17, 2023

Could this help speed up slow shader compile times on Mac with html5 builds when using ANGLE? see #72584

@akien-mga
Copy link
Member

No that's unrelated, this is for the RenderingDevice backends (Vulkan, D3D 12), not OpenGL/WebGL.

@Kishorekanth7

This comment was marked as spam.

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.

Vulkan: Enabling SSR or SSS with TAA and XR enabled results in all geometry appearing completely black
7 participants