Skip to content

Conversation

remi-chapelain
Copy link
Contributor

Purpose of this PR

Following this forum post, I propose adding this bit of information to the documentation.

Even though, the recommended route to take is to use the Lit master node in shader graph instead of using HDRP/Lit materials.
I still think users will try it (cf forum post) and thus makes sense to include it.


Comments to reviewers

Text is "draft", feel free to correct anything according to proper English grammar and documentation standards :)

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

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_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.

@remi-chapelain remi-chapelain marked this pull request as ready for review September 16, 2021 09:25
Copy link
Contributor

@JMargevics JMargevics left a comment

Choose a reason for hiding this comment

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

Topic and the addition make perfect sense. Approving ✔👍

@@ -0,0 +1,11 @@
# Editing shader properties at runtime

For High Definition Render Pipeline (HDRP) non-Shader Graph shaders, such as the [Lit](Lit-Shader.md), [Unlit](Unlit-Shader.md) shaders, changing a property at runtime, in some cases, does not have any effect. This is because if the property is not enabled before runtime, the specific shader variant associated with this property is not included in the shader (and is also not included when bulding a player). To include the shader variant, the proper keywords need to be enabled on the material before editing.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is various incorrect statement here. Probe exist also for shader graph.

Here is a proposal:

Each shader feature (Keyword) use in a shader (link to the doc) produce a different shader code to compile. Each of this shader code is call a shader variant. Unity try to reduce the number of shader variant to compile by only considering the sets of kewords use in a Material. Other shader variant don't exist. When building a Player, it is the union of the all shader variant based on keyword used that is compiled.

If you try to create a Material at runtime and change proerties which affect which keywords is enabled. In case this combination was not existing when building the player, this shader variant will not exist and the rendering will fail.
This can happen if you use for example the HDRP/Lit shader and try to setup a iridecence material but don't have any ididescence properties enabled in an already existing Material.

^ So this is not good but this is what I think we should express. We should be clear that 1) This is for Unity in general, not only HDRP. Builtin have same issue. And that this is also for shader graph if you add keyword in shader graph.
Then the goal is to explain that users must enable the right keyword at runtime and that this shader variant need to exist.

this statement can be share with URP documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will update with a new proposal with this info !

@remi-chapelain
Copy link
Contributor Author

remi-chapelain commented Sep 17, 2021

Updated the text based on what has been discussed with @sebastienlagarde

@Vic-Cooper
Copy link
Contributor

This is large enough to need editorial review so I'm working on this in a google doc..

@remi-chapelain I'll invite you for a tech review when I've made changes :).

@sebastienlagarde
Copy link
Contributor

update on this PR?

@Vic-Cooper
Copy link
Contributor

@sebastienlagarde This doc has been absorbed into the shader docs. See the draft google doc for input from Kerry. Please close and do not merge this PR.

@remi-chapelain
Copy link
Contributor Author

Closing this PR because the information have been included in this more recent one.

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.

4 participants