Skip to content

Conversation

cdxntchou
Copy link

@cdxntchou cdxntchou commented Feb 23, 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/2021.1%252Fsg%252Ffix%252F1296776/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2021.1/5480887/job/pipeline

HDRP PR Job: Failures match parent 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/2021.1%252Fsg%252Ffix%252F1296776/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.1/5480909/job/pipeline

Failures:
HDRP on Linux_Vulkan_playmode_cache_mono_Linear on version 2021.1 -- both failing on same test in same way
HDRP on OSX_Metal_playmode_cache_mono_Linear on version 2021.1 -- same 18 tests failing same way
HDRP on Win_Vulkan_playmode_cache_mono_Linear on version 2021.1 -- same test failed same way

Universal PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/2021.1%252Fsg%252Ffix%252F1296776/.yamato%252Fall-universal.yml%2523PR_Universal_2021.1/5480938/job/pipeline

Package Validation: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/2021.1%252Fsg%252Ffix%252F1296776/.yamato%252F_projectcontext.yml%2523test_all_project_Win_2021.1/5520299/job

Parent 2021.1/staging: 7f85971

2021.1/staging HDRP PR Job:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/2021.1%252Fstaging/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.1/5484814/job/pipeline


Comments to reviewers

Notes for the reviewers you have assigned.

# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
#	com.unity.shadergraph/Editor/Generation/Processors/Generator.cs
#	com.unity.shadergraph/Editor/Generation/Processors/PropertyCollector.cs
#	com.unity.shadergraph/Editor/Importers/ShaderSubGraphImporter.cs
@cdxntchou cdxntchou requested a review from a user February 23, 2021 18:45
@cdxntchou cdxntchou marked this pull request as ready for review February 23, 2021 18:45
@cdxntchou cdxntchou requested a review from a team as a code owner February 23, 2021 18:45
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.

Dev testing is sufficient

@marctem marctem self-requested a review February 26, 2021 18:53
@marctem marctem merged commit 8513a42 into 2021.1/staging Feb 26, 2021
@marctem marctem deleted the 2021.1/sg/fix/1296776 branch February 26, 2021 18:57
@marctem marctem removed their request for review February 26, 2021 18:57
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