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

[2021.2][ShaderGraph] Add BuiltIn Target variant stripping #5204

Merged
merged 4 commits into from
Jul 23, 2021

Conversation

jessebarker
Copy link
Contributor

@jessebarker jessebarker commented Jul 22, 2021


Purpose of this PR

This PR addresses https://fogbugz.unity3d.com/f/cases/1345580/

Currently, the BuiltIn Target does no variant stripping (apart from what the ShaderLab compiler does for us). For users testing assets against, say, both BuiltIn and URP, this can yield a huge set of variants for compilation. This code is heavily based on the stripping done by URP, primarily because the BuiltIn Target and some of its shader library are also derived from the URP implementation (though most of the BuiltIn shader library are the same cginc files used by hand-written shaders for the built-in render pipeline). Some things to note (this should all be documented in comments in the code, but feel free to comment if it isn't clear:

  • BuiltIn cannot strip many variants that URP or HDRP can (e.g., there is no "global" rendering path setting for built-in the way there is for URP and HDRP via the asset, so we have to assume that deferred and forward can be used).
  • BuiltIn will strip all SRP shader variants only if there is no SRP asset assigned in settings (i.e., we are sure that built-in is the pipeline in use)
  • Only non-SRP BuiltIn Shader Graph shader snippet stripping activity is logged to the console, so the console log will not show the full stripping results (see caveat below).

Additional caveat on the logging of variant stripping in general: There are a large number of shader variants that are not logged by the existing URP and HDRP processors (the same will be true for the BuiltIn processor), even though they are seen/processed. Many of these occur after the ones logged by these processors, which means the final numbers (totals/percentages) in the console log are not the real final numbers. For example (using the URP Lit Shader Graph mentioned below), filtering so that only the BuiltIn passes are logged makes it appear as though we have included 12.5K out of 295K variants, when the real number is the 13K out of 410K variants mentioned below in the testing status. This is due to the large number of hidden/legacy/other shaders that are actually included in the project, but don't pass the logging filter.


Testing status

Manual testing according to the repro shadergraph from the bug (result after stripping is a similar number of total variants passed to the compiler as for URP - roughly 7K out of 280K variants, or about 2%).
Manual testing similar to above, but with the proposed new URP Lit Shader Graph (roughly 13K out of 410K variants, or about 3%).
Working on automated tests using the URP Lit Shader Graph.

Yamato jobs:


Comments to reviewers

Notes for the reviewers you have assigned.

    -  view DirectionWS is never filled out in the legacy pass' function. Always use the value computed by SRP
    - Discovered during built-in shader stripping testing with new Lit shadergraph
@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2021.2
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_2021.2
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@jessebarker jessebarker marked this pull request as ready for review July 22, 2021 22:23
@jessebarker jessebarker requested a review from a team as a code owner July 22, 2021 22:23
@jessebarker jessebarker changed the title Sg builtin variant stripping [2021.2][ShaderGraph] Add BuiltIn Target variant stripping Jul 22, 2021
Copy link
Contributor

@cdxntchou cdxntchou 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!

@cdxntchou cdxntchou merged commit 1022e22 into master Jul 23, 2021
@cdxntchou cdxntchou deleted the sg-builtin-variant-stripping branch July 23, 2021 19:24
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.

3 participants