Skip to content

Conversation

@cdxntchou
Copy link

@cdxntchou cdxntchou commented Feb 19, 2021

Purpose of this PR

Fix for bug https://fogbugz.unity3d.com/f/cases/1296776/
and https://fogbugz.unity3d.com/f/cases/1310624/

PR links:
2021.2: #3625
Backport 2021.1: #3623
Backport 2020.2: #3577

Previously, we built a global list of properties to populate the Property{} block in the shaderlab shader, but we built a separate list of properties per pass, that was used to build the HLSL declarations of those properties.

The per-pass properties would only include properties declared by the active Target and the active nodes used by the pass, so they could result in different properties for different passes.

The SRP Batcher requires all passes within a SubShader to use an identical UnityPerMaterial cbuffer declaration, which was often broken by the different property lists. (Also in the future the new BindSets will have a similar requirement for object resources (textures, samplers, etc) to be the same across all passes).

This change switches the HLSL declarations to use a per-sub-shader calculated list of properties. It gathers properties from any nodes upstream of blocks used by the Target, as well as graph and Target properties.


Testing status

  • Verified that URP shaders are now produced with a single UnityPerMaterial layouts per Target, in use cases where they previously were not (usually Texture nodes attached to Master Blocks that are not used by shadow passes)
  • Verified HDRP shaders
  • Verified URP + HDRP shaders (in HDRP)
  • Verified those shaders are now detected as SRP Batcher compatible, and render in Hybrid renderer
  • Verified importer versions bumped
  • Tested that subgraph properties still work

Discovered Issues:

  • OpenGL does not like '__' in an identifier, will fail shader compilation, pre-existing issue
  • URP + HDRP shaders (in URP) -- does not work, even though the generated shader appears to be following all the rules. pre-existing issue. Contacted Arnaud to figure out if this is a bug in the SRP Batcher compatibility check. https://fogbugz.unity3d.com/f/cases/1307728/

Yamato:

ShaderGraph PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/10.x.x%252Fsg%252Ffix%252F1296776/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2020.2/5458668/job/pipeline

HDRP PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/10.x.x%252Fsg%252Ffix%252F1296776/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2020.2/5460524/job

Universal PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/10.x.x%252Fsg%252Ffix%252F1296776/.yamato%252Fall-universal.yml%2523PR_Universal_2020.2/5458736/job/pipeline

Corresponding 10.x.x/release: a64da1f
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/10.x.x%252Frelease/.yamato%252Fall-universal.yml%2523PR_Universal_2020.2/5442672/job/pipeline


Comments to reviewers

Notes for the reviewers you have assigned.

@cdxntchou cdxntchou changed the title [10.x.x] [2020.2] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list [2020.2] [10.x.x] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list Feb 19, 2021
…g/fix/1296776

# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
@cdxntchou cdxntchou changed the base branch from master to 10.x.x/release February 22, 2021 17:24
@cdxntchou cdxntchou marked this pull request as ready for review February 22, 2021 22:53
@cdxntchou cdxntchou requested a review from a team as a code owner February 22, 2021 22:53
@cdxntchou cdxntchou requested a review from a user February 22, 2021 22:54
Copy link
Contributor

@jessebarker jessebarker left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Bug filed to deal with case that was unable to be fixed (both targets built in URP): https://fogbugz.unity3d.com/f/cases/1307728/
With that I'll approve.

@cdxntchou cdxntchou merged commit 26a1096 into 10.x.x/release Feb 25, 2021
@cdxntchou cdxntchou deleted the 10.x.x/sg/fix/1296776 branch February 25, 2021 00:46
AndrewSaraevUnity pushed a commit that referenced this pull request Apr 29, 2021
…derer compatibility by always using a consolidated property list (#3577)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

(cherry picked from commit 26a1096)
AndrewSaraevUnity added a commit that referenced this pull request May 4, 2021
* [HDRP] Make Decal shader compatible with SRP batcher (#3806)

* Fix SRP batcher with decal

* Update CHANGELOG.md

* Update CHANGELOG.md

(cherry picked from commit 5a88bb8)

* Added DOTS_INSTANCING_ON variants to the "HDRP/Decal" shader to support mesh decals using it in the Hybrid Renderer. (#4284)

(cherry picked from commit fa2ff6e)

* [2020.2] [10.x.x] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list (#3577)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

(cherry picked from commit 26a1096)

Co-authored-by: sebastienlagarde <[email protected]>
Co-authored-by: Chris Tchou <[email protected]>
pastasfuture pushed a commit that referenced this pull request Aug 20, 2021
… a consolidated property list (#3577) (#2)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

(cherry picked from commit 26a1096)
pastasfuture pushed a commit that referenced this pull request Sep 15, 2021
… a consolidated property list (#3577) (#2)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

(cherry picked from commit 26a1096)
mohammad22 pushed a commit that referenced this pull request Mar 2, 2022
… a consolidated property list (#3577) (#2)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

(cherry picked from commit 26a1096)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants