[ShaderGraph] [2021.2] Fix include ordering #3924
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…operty declarations
added 3 commits
March 19, 2021 15:08
…12.x version tests)
…it on some test shaders)
jessebarker
reviewed
Mar 22, 2021
| bool isOSX = | ||
| (Application.platform == RuntimePlatform.OSXEditor) || | ||
| (Application.platform == RuntimePlatform.OSXPlayer); | ||
|
|
Contributor
There was a problem hiding this comment.
Vulkan doesn't guarantee more than 16 either. I suspect most desktop implementations actually provide more than 16, but the hardware requirements for the spec are derived from OpenGL ES 3.1, so it's not guaranteed without checking.
Author
There was a problem hiding this comment.
Yeah, it at least passes on Windows Editor / Vulkan on my box.
added 4 commits
March 22, 2021 11:01
…he multi-import tests (we rely on being able to lookup files by GUID)
jessebarker
approved these changes
Mar 23, 2021
Contributor
jessebarker
left a comment
jessebarker
left a comment
There was a problem hiding this comment.
Updated changes look good
...y.render-pipelines.high-definition/Editor/Material/ShaderGraph/Templates/ShaderPass.template
Show resolved
Hide resolved
joshua-davis
approved these changes
Mar 24, 2021
ellioman
approved these changes
Mar 25, 2021
sebastienlagarde
pushed a commit
that referenced
this pull request
Mar 30, 2021
* Fix include ordering by moving graph/node based includes below the property declarations * Adding tests for include file / custom function node issue (and also 12.x version tests) * Handling compile faillures on OSX GLCore tests (sampler limit count hit on some test shaders) * Fix compile * Disabling failing test for now * Think this will fix the tests -- variable name was too long * Testing super short variable names * think this fixes it -- force asset database to update before we run the multi-import tests (we rely on being able to lookup files by GUID)
sebastienlagarde
added a commit
that referenced
this pull request
Mar 31, 2021
This reverts commit 331d995.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
Fix for https://fogbugz.unity3d.com/f/cases/1322467/
This fixes a regression introduced by this PR: #3290
By registering the node include files at generation time, instead of splicing them into the generated code, we can now properly track the include dependencies; but it also moved the location where the includes were spliced. In particular, the node includes now show up before the property declarations in many templates. In order for the CustomFunctionNode included files to be able to access shader properties, the includes must come after the property declarations. This PR adds a new include splice location "Graph", which come after the property declaration, and then moves all node-declared includes to this new location.
Testing status
Added a ShaderGraph test for include file usage of graph properties:
Yamato tests:
(running against trunk f4de2e18f4148a678759ff44540c9d25ffd9d99f, because of trunk breakages)
ShaderGraph PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1322467/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_CUSTOM-REVISION/5939294/job/pipeline
Failures:
Build ShaderGraph on iPhone_Metal_il2cpp_Linear_Standalone_cache_build_Player on version CUSTOM-REVISION -- success on re-run
Universal PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1322467/.yamato%252Fall-universal_split.yml%2523PR_Universal_Split_CUSTOM-REVISION/5939301/job/pipeline
Failures:
URP_Foundation on iPhone_Metal_Standalone_cache_il2cpp_Linear on version CUSTOM-REVISION -- success on re-run
HDRP PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1322467/.yamato%252Fall-hdrp.yml%2523PR_HDRP_CUSTOM-REVISION/5946563/job/pipeline
Failures:
Build VFX_HDRP on Win_DX11_mono_Linear_Standalone_cache_build_Player on version CUSTOM-REVISION -- also fails on master
Build VFX_HDRP on Win_DX12_mono_Linear_Standalone_cache_build_Player on version CUSTOM-REVISION -- also fails on master
VFX_HDRP on Win_DX11_Standalone_cache_mono_Linear on version CUSTOM-REVISION -- also fails on master
VFX_HDRP on Win_DX12_Standalone_cache_mono_Linear on version CUSTOM-REVISION -- also fails on master
Pack and Test Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1322467/.yamato%252F_projectcontext.yml%2523test_all_project_Win_trunk/5946535/job
Comments to reviewers
Notes for the reviewers you have assigned.