Skip to content

Conversation

joshua-davis
Copy link
Contributor

@joshua-davis joshua-davis commented Sep 28, 2021

…es fogbugz 1355815

Please read the Contributing guide before making a PR.

Checklist for PR maker

  • Have you updated the changelog? Each package has a CHANGELOG.md file.

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1355815/

The behavior of this bug has shifted over the past few months. The last main fix that caused the latest behavior was #3664. This removed a validation error being added to each preview node which is how the old error message used to be displayed. This caused each node to now fall back to the shader compiler error. This shader compiler error was because we put both the error shader and the original shader in the same preview, something akin to:

Shader "MyShaderGraph" {...}
Shader "hidden/preview/error_eff8e5151a46414e929efe020074aa57" {}

Removing the duplicate shaders removed the unreadable error, but left the user with only a log message and the pink preview shaders.
image

To aid discoverability, a help box was added to the blackboard when the variant limit was reached.
image


Testing status

  • Blackboard error appears when adding enough keywords to exceed the limit
  • Error disappears when removing a keyword
  • Error disappears when undoing (to get back below the variant limit)
  • Error re-appears when redoing back over the variant limit
  • Nodes with previews now no longer display an error badge when over the variant limit.
    • They still display as an error (pink shader) which was deemed fine.
  • Console log message still printed with the variant exceeded error
  • Tested the above in both a sub-graph and a graph

Comments to reviewers

Notes for the reviewers you have assigned.

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

m_ConfiguredTextures = shaderProperties.GetConfiguredTextures();
m_Builder.AppendLines(ShaderGraphImporter.k_ErrorShader.Replace("Hidden/GraphErrorShader2", graphName));
// Don't continue building the shader, we've already built an error shader.
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents the duplicate shader from being put in the same file.

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

@joshua-davis joshua-davis marked this pull request as ready for review September 28, 2021 22:27
Copy link
Contributor

@Nightmask3 Nightmask3 left a comment

Choose a reason for hiding this comment

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

code looks good! approving 👍

Copy link
Contributor

@bencloward bencloward left a comment

Choose a reason for hiding this comment

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

Tested this out locally and it's working well. Very clear message now.

@joshua-davis joshua-davis merged commit 13db8c2 into master Oct 1, 2021
@joshua-davis joshua-davis deleted the sg/keyword-error branch October 1, 2021 17:39
Nightmask3 added a commit that referenced this pull request Nov 29, 2021
#5829)

* Fixed how errors were displayed when variant limits were reached. Fixes fogbugz 1355815
# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
Nightmask3 added a commit that referenced this pull request Nov 30, 2021
#5829) (#6425)

* Fixed how errors were displayed when variant limits were reached. Fixes fogbugz 1355815
# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
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