New functionality that supports compile time branching based on user created dropdowns. by russoh · Pull Request #4137 · Unity-Technologies/Graphics

russoh · 2021-04-08T23:07:13Z

Please read the Contributing guide before making a PR.

Read the Graphics repository & Yamato FAQ.

Checklist for PR maker

Have you added a backport label (if needed)? For example, the need-backport-* label. After you backport the PR, the label changes to backported-*.
Have you updated the changelog? Each package has a CHANGELOG.md file.
Have you updated or added the documentation for your PR? When you add a new feature, change a property name, or change the behavior of a feature, it's best practice to include related documentation changes in the same PR. If you do add documentation, make sure to add the relevant Graphics Docs team member as a reviewer of the PR. If you are not sure which person to add, see the Docs team contacts sheet.
Have you added a graphic test for your PR (if needed)? When you add a new feature, or discover a bug that tests don't cover, please add a graphic test.

Purpose of this PR

This PR adds support for branching based on user created dropdowns.

Motivation for work: https://docs.google.com/document/d/1XeUfc2MauivBYx82V68c_u2aQBZJxt2h9VRVu9yemGY/edit

The workflow, and UI for creating dropdowns is very similar to the behavior for working with keyword enums. Dropdowns are a new type of blackboard property, where the user can specify a list of options, and a default. All of the scoping, and keyword specific controls are stripped down, so it is simpler than keywords. The user can drag a dropdown property out onto the graph canvas, which will create a corresponding branch node, with slots for each option.

The key distinction for dropdowns - dropdowns are exposed on the UI of subgraph node instances, where those subgraphs have defined dropdowns. This allows the user to pick the desired branch per subgraph instance from the parent graph. This is all resolved at compile time.

https://jira.unity3d.com/browse/GSG-354

Testing status

Manual testing:

Utilized in Surface Gradient example "Sphere Dropdown And Connection Testing":
https://github.cds.internal.unity3d.com/unity/surfgrad-examples

Automated tests:

Graphics/TestProjects/ShaderGraph/UtilityNodes.unity : Dropdowns
https://jira.unity3d.com/browse/GSG-396

Comments to reviewers

Basic video of this in action:
SubgraphDropdown.zip

For testing, use instructions here for getting the compatible build: https://github.cds.internal.unity3d.com/unity/surfgrad-examples/blob/master/README.md

IMPORTANT - you cannot create more than dropdown using the conventional + via the blackboard. If you do this, any secondarily created properties will not be visible, and the graph will break. You MUST make new properties by duplicating the first dropdown using Ctrl-D, until this bug has been addressed. The issue also affects keyword enums, and will be addressed by a separate PR as it is common to both.

I will merge in latest of master periodically to get ready for final merge.

Version numbers of serialized data will need bumping between landing of PRs.

Fix undo for dropdowns. Limit dropdowns to subgraphs. Reordering dropdowns does not affect subgraph node selection. Fix id assignment for keywords (should find unused id, not use current list index). Fix default dropdown name. Fix searcher for dropdowns.

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/Editor/Drawing/Blackboard/BlackboardFieldView.cs # com.unity.shadergraph/Editor/Drawing/Blackboard/BlackboardProvider.cs # com.unity.shadergraph/Editor/Drawing/Inspector/PropertyDrawers/ShaderInputPropertyDrawer.cs # com.unity.shadergraph/Editor/Drawing/Views/MaterialGraphView.cs

… into sg/surfgrad/dropdowns

ghost · 2021-04-15T22:26:39Z

Test doc: https://docs.google.com/document/d/1-tZ5T0KH_u2ZZE4Fxd8B7bs7QghTkHlj3tAa3e9eSb0/edit?usp=sharing

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

…eywords: https://github.com/Unity-Technologies/Graphics/pull/4184/files

joshua-davis

One other small note: When you re-order the items in the keywords the Default value seems to point to whatever's at the index, not the item by name. That is if you have:
{ItemA, ItemB, ItemC} and the default is currently ItemB and you then reorder to {ItemA, ItemC, ItemB} then the default will now be ItemC (both index 1). Is this intended? Not a big deal either way, just something I noticed.

Fix for preventing default dropdown value changing when reordering dropdown entries. Add default else to dropdown code gen.

joshua-davis · 2021-04-23T17:46:49Z

+                    }
+
+                    {
+                        sb.AppendLine("{");


nit: you could use sb.BlockScope here to clean this up a bit. In particular this becomes:

using(sb.BlockScope()) { var value = GetSlotValue(GetSlotIdForPermutation(new KeyValuePair<ShaderDropdown, int>(dropdown, i)), generationMode); sb.AppendLine(string.Format($"{GetVariableNameForSlot(OutputSlotId)} = {value};")); }

Something I just noticed that I didn't before.

joshua-davis

Small nit, but otherwise looks good

ghost

All issues found during my testing were addressed.

… into sg/surfgrad/dropdowns

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

russoh · 2021-04-28T14:28:03Z

This one has already had a full green test panel on Shader Graph ABV with the changes specific to this branch:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Fsurfgrad%252Fdropdowns/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/6477815/job

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

russoh · 2021-04-28T18:21:39Z

ScriptableRenderPipelinePrivate console passes for this PR: https://yamato.cds.internal.unity3d.com/jobs/339-ScriptableRenderPipelinePrivate/tree/sg%252Fsurfgrad%252Fdropdowns/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/6557856/job/pipeline

PR to land changes to ScriptableRenderPipelinePrivate: https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/168

cdxntchou

Should probably remove the unnecessary versioning changes. The rest looks good!

elizabeth-legros · 2021-04-28T19:08:50Z

                        keywordGuidMap[input0.m_Guid.m_GuidSerialized] = keyword;
                    }

+                    foreach (var serializedDropdown in graphData0.m_SerializedDropdowns)


Do we need to handle dropdowns in old master node graphs coming in? Are we backporting dropdowns to master nodes?

This is a good question, should definitely get clarity on that flow.

If we are not (which I would hope at this point tbh), then all the graphdata0 or abstractmaterialnode0 stuff should be unnecessary

Cheers, I have removed this now.

elizabeth-legros · 2021-04-28T19:09:59Z


        public string m_KeywordGuidSerialized;

+        public string m_DropdownGuidSerialized;


again, will dropdowns be ported to old master nodes? If no, not needed

Also removed, cheers.

elizabeth-legros · 2021-04-28T19:10:30Z


        public List<SerializationHelper.JSONSerializedElement> m_SerializedKeywords;

+        public List<SerializationHelper.JSONSerializedElement> m_SerializedDropdowns;


if not backporting, unnecessary

Removed this one.

… into sg/surfgrad/dropdowns

russoh · 2021-04-29T18:52:16Z

All test have now passed. I will only pull to resolve the Changelog conflict.

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

russoh added 2 commits April 7, 2021 11:29

Compile time dropdown blackboard properties + DropDownNode.

def15db

Cleanup comments.

14501ff

russoh changed the title ~~New functionality that adds compile time resolved branching via user configurable dropdowns.~~ New functionality that supports compile time branching based on user created dropdowns. Apr 8, 2021

russoh added Shader Graph SRP labels Apr 8, 2021

russoh requested review from a user, Nightmask3, alindmanUnity and cdxntchou April 9, 2021 00:16

russoh added 12 commits April 12, 2021 16:44

Bump version numbers.

b5d938b

Various fixes needed post merge of master.

f374990

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

154910d

… into sg/surfgrad/dropdowns

Formatting.

0b8a479

Output shader code formatting.

10ae436

Formatting.

b97bc19

Check name clash.

6fc6d36

Tests.

2fbe4f0

Add material.

c7cd814

Fixes.

0509651

russoh marked this pull request as ready for review April 15, 2021 15:14

russoh requested a review from a team as a code owner April 15, 2021 15:14

russoh added 2 commits April 21, 2021 15:02

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

de6b489

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

Incorporate bug fix for adding dropdowns based on recent PR fix for k…

e19b4ff

…eywords: https://github.com/Unity-Technologies/Graphics/pull/4184/files

joshua-davis reviewed Apr 22, 2021

View reviewed changes

russoh added 2 commits April 22, 2021 11:41

Fix for refreshing blackboard when pasting properties.

ecc004b

Fix for preventing default dropdown value changing when reordering dropdown entries. Add default else to dropdown code gen.

Fix erroneous copy command.

7dddb29

Nightmask3 requested changes Apr 22, 2021

View reviewed changes

Update test images to latest.

8eb9e9a

joshua-davis reviewed Apr 23, 2021

View reviewed changes

joshua-davis approved these changes Apr 23, 2021

View reviewed changes

russoh added 3 commits April 23, 2021 11:40

Update iPhone image.

73d8fe7

Updated OSX image.

f6b9fea

Prevent dragging dropdowns into parent graph.

e62270b

ghost approved these changes Apr 23, 2021

View reviewed changes

russoh added 3 commits April 26, 2021 10:09

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

3bb9117

… into sg/surfgrad/dropdowns

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

c1383b5

… into sg/surfgrad/dropdowns

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

28294c6

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

573f987

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

cdxntchou suggested changes Apr 28, 2021

View reviewed changes

Comment thread com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs

Comment thread com.unity.shadergraph/Editor/Data/Legacy/AbstractMaterialNode0.cs Outdated

Comment thread com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs Outdated

elizabeth-legros reviewed Apr 28, 2021

View reviewed changes

Comment thread com.unity.shadergraph/Editor/Data/Graphs/DynamicVectorMaterialSlot.cs

elizabeth-legros reviewed Apr 28, 2021

View reviewed changes

Comment thread com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs

elizabeth-legros reviewed Apr 28, 2021

View reviewed changes

Comment thread com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs

elizabeth-legros reviewed Apr 28, 2021

View reviewed changes

elizabeth-legros approved these changes Apr 28, 2021

View reviewed changes

russoh added 2 commits April 28, 2021 12:35

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

213eae9

… into sg/surfgrad/dropdowns

Remove redundant serialization code.

4b27597

russoh requested review from cdxntchou and removed request for alindmanUnity April 28, 2021 23:27

russoh added 2 commits April 29, 2021 11:56

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

3bd61e9

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

26785b4

… into sg/surfgrad/dropdowns # Conflicts: # com.unity.shadergraph/CHANGELOG.md

marctem merged commit e97f16e into master Apr 29, 2021

marctem deleted the sg/surfgrad/dropdowns branch April 29, 2021 21:16


		public string m_KeywordGuidSerialized;

		public string m_DropdownGuidSerialized;


		public List<SerializationHelper.JSONSerializedElement> m_SerializedKeywords;

		public List<SerializationHelper.JSONSerializedElement> m_SerializedDropdowns;

Conversation

russoh commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please read the Contributing guide before making a PR.

Checklist for PR maker

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

ghost commented Apr 15, 2021

Uh oh!

joshua-davis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-davis left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

russoh commented Apr 28, 2021

Uh oh!

russoh commented Apr 28, 2021

Uh oh!

cdxntchou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russoh commented Apr 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

russoh commented Apr 8, 2021 •

edited

Loading