Skip to content

Generate a material as a subasset for ShaderGraphs #5795

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

Merged
merged 6 commits into from
Oct 19, 2021

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Sep 24, 2021

Purpose of this PR

This PR adds an automatically generated material subasset on ShaderGraphs:

material_subasset
graph inspector

I bumped the shadergraph importer version, so for existing projects every graph will be reimported and the material willl be generated.

Benefits:

  • It avoids having to manually create a material each time a shadergraph is created
  • When the shadergraph is saved, the material is regenerated, so the default values for properties are updated on the material
  • When changing the default values in the blackboard, the material is updated and therefore changes can be seen instantly in the scene view.

Supported properties:

  • float/vector2/vector3/vector4
  • color
  • boolean
  • texture 2D/3D/Array/Cube
  • keyword Enum/Boolean

That's essentially all properties that can be exposed, I don't think I am missing one

This is part of this JIRA task https://unity3d.atlassian.net/browse/UR-1450


Testing status

See the gif above.
Tested undo/redo of shadergraph properties.
Tested closing shadergraph window with saving or discarding changes.
Tested that the generated material works on a mesh renderer in a standalone build.

The feature has also been tested by several artists in the context of material variants
I will add a graphic test.

@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_trunk
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_trunk

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.

@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as ready for review September 24, 2021 11:13
@adrien-de-tocqueville adrien-de-tocqueville requested a review from a team as a code owner September 24, 2021 11:13
@sebastienlagarde
Copy link
Contributor

For existing projects, the material will not be generated until the SG is saved. It can be done by bumping the importer version but I don't think it's necessary

yes we should bump the importer version.

Should the user be able to disable this feature in the graph settings ?
no

Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

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

What is happening if a users setup this on the fly material to an object and build. does it works in player? (i.e is the build step considering it is a true material and store it correctly in build data? Because shader graph don't exist at runtime.

@adrien-de-tocqueville
Copy link
Contributor Author

What is happening if a users setup this on the fly material to an object and build. does it works in player? (i.e is the build step considering it is a true material and store it correctly in build data? Because shader graph don't exist at runtime.

It works fine, actually it's the same process as for the shader itself, which is an artifact generated by the shadergraph importer that is available at runtime

@cdxntchou cdxntchou requested a review from a team September 28, 2021 16:32
Copy link

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

As we discussed, there might be an edge case if some script comes along and forces AssetDatabase to save the dynamic changes to the default material artifact. Not sure if that is even possible, but that should be an issue with the script if so.

@adrien-de-tocqueville
Copy link
Contributor Author

@ellioman tagging you here although this will not directly impact URP, but for awareness

Copy link
Contributor

@alindmanUnity alindmanUnity left a comment

Choose a reason for hiding this comment

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

UX approval

Copy link
Contributor

@GrantLamb-Unity GrantLamb-Unity left a comment

Choose a reason for hiding this comment

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

Ran some manual tests with the following exposed properties:

  • float
  • Texture 2D
  • Color

Also ran tests to ensure naming conflicts don't cause issues with this auto generated material. No issues were encountered during these tests.

@sebastienlagarde sebastienlagarde merged commit 4e1396a into master Oct 19, 2021
@sebastienlagarde sebastienlagarde deleted the sg/material-subasset branch October 19, 2021 17:02
Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

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

do we have any what's new for shader graph?

odbb added a commit that referenced this pull request Oct 20, 2021
* master:
  Add missing shader stages in shader keyword copying  (#6008)
  [Yamato] Add all DX11 job for URP (#6075)
  [HDRP] Add more performance test coverage (#5814)
  Fix templates ISO date (#6073)
  Fix division by 0 when AO is 0 (#6078)
  Fixed grammar errors (#6077)
  [HDRP][Path Tracing] Improved robustness of the stacklit material (#6066)
  [APV] Cell streaming system (#5731)
  Update revision for URP update test project (#6061)
  [HDRP][Path Tracing] Camera ray misses now return a null value with Minimum Depth > 1 (#6067)
  Renable missing test (Lens Flare) (#5456)
  [HDRP][Path Tracing] Added proper support for interleaved tiling (#5953)
  Fix to render depth or depth/normal of waving grass (#4097)
  Remove ScreenSpaceShadowResolvePass (#6002)
  [HDRP][Docs] Update docs with RendererList related option (#6031)
  Layer drawer used in ray/path tracing now matches 100% with camera's. (#5956)
  Fix iridescence tooltip (#5950)
  [HDRP] Change RenderGraph Begin/Execute function pattern to avoid leaks (#5929)
  [HDRP] Fix errors when switching build targets in editor (#5918)
  Generate a material as a subasset for ShaderGraphs (#5795)
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.

5 participants