Skip to content
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

[VFX] Fix new SG integration while using keywords #6912

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Jan 31, 2022


Purpose of this PR

Fix this fogbugz

  • Keyword aren't exposed from VFX but we shouldn't fallback to incorrect shader (black in HDRP, compilation error in URP)
  • Improve error feedback
  • Fix shader generation with VFX & SG + Keyword Permutation

⚠️ This PR doesn't add support of exposed keyword editable in VFX but only prevents compilation issue while using them.


Testing status

_error_feedback_sg.mp4

Comments to reviewers

About needed change in ShaderGraph code generation

Without the modification in ShaderSpliceUtil.cs, the generated code ends with:

// Configure the output type-spcific mesh definition and index calculation for the rest of the element data.
#if defined(KEYWORD_PERMUTATION_0) || defined(KEYWORD_PERMUTATION_1) || defined(KEYWORD_PERMUTATION_2)
$include("VFXConfigPlanarPrimitive.template.hlsl")
#endif

The keyword generation is adding the #if condition but the content was already a $include (see here)

See also this conversation about usage of keyword with VFX

About ShaderGraph allowed usage in VFX

The behavior has been fine tuned ⬇️

🔴 : Error
🟠 : Warning
🟢 : Allowed

Before

Type Not Exposed Exposed
Virtual Texture N/A (always exposed) 🔴
Gradient 🔴 N/A (so far, always not exposable)
Diffusion profile 🔴 🔴
Keywords (local/global & multicompile/shaderfeature) 🔴 🔴

In case of error, we were fallbacking to an unexpected shader generation which can lead to a black material in HDRP or compilation error in URP.

After

Type Not Exposed Exposed
Virtual Texture N/A (always exposed) 🔴 Critical issue, we won't compile.
Gradient 🟢 N/A (so far, always not exposable)
Diffusion profile 🟢 (not sure it's recommanded neither) 🔴 Unexpected behavior.
Keywords (local/global & multicompile/shaderfeature) 🟢 🟠 The value isn't visible in VFX Output but the material can be modified by script. It can have an odd behavior due to the default keyword state. However, it should be consistent with general material usage.

In case of error, the output is discared from the compilation. When it's a warning, the generation is proceeding but there is an error feedback in VFX (the Debug.LogWarning is kept since these problems can occurs with ShaderGraph modification)

About error feedback from compilation

We were already following a pattern Begin()/End() with SetupCompilation()/EndCompilation().
I added a way to collect and store errors/warnings during compilation to supply them in GenerateErrors.
It also affects the old shader graph integration when a shader graph Lit is used on Unlit output and vice versa.

Additionnal Note

Known issue about missing inspector UX from material (see 1388531), the problem landed in 2022.2.0a1.9_e743b07bbaa9

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Jan 31, 2022
@VitaVFX VitaVFX requested review from VitaVFX and removed request for a team February 7, 2022 10:25
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Feb 8, 2022
@VitaVFX
Copy link

VitaVFX commented Feb 9, 2022

Hi! Dropping WIP Test Doc. Jotted down several questions.

@PaulDemeulenaere PaulDemeulenaere requested a review from a team as a code owner February 10, 2022 11:14
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Feb 10, 2022
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Feb 10, 2022
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Feb 14, 2022
Copy link

@VitaVFX VitaVFX left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this!

All raised concerns were fixed, couldn't find any other oustanding behavior specific to VFX. Resharing Test Doc for visibility.

@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Feb 18, 2022
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Mar 7, 2022
@PaulDemeulenaere
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants