Skip to content

Commit

Permalink
Fix "ToSubgraph" with multiple outputs and allow passthrough (#2074)
Browse files Browse the repository at this point in the history 
* first pass, need to talk to Landon about fix

* Update CHANGELOG.md

* making found properties also look for a matching reference name

* code review changes

* code review bug fix and made copying properties/keywords form the blackboard consistent with how nodes behave in searching for properties/keywords

* small bugfix for duplicating properties on graph/between graphs

* more bugfixing

* fixing to subgraph workflow

* code review notes

* more bugfixing and improvements

* adding fixup of graph with convert to subgraph for alex to test

* bugfixing

* Update CHANGELOG.md

* adding back in error throw that was commented out for testing

* Update CHANGELOG.md

* Fixing formatting

Co-authored-by: Chris Tchou <ctchou@unity3d.com>
  • Loading branch information
@elizabeth-legros
elizabeth-legros and Chris Tchou authored Dec 15, 2020
1 parent a31a2b3 commit 4d843ce
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 73 deletions.
5 changes: 3 additions & 2 deletions com.unity.shadergraph/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Shaders using SamplerState types now compile with GLES2 (SamplerStates are ignored, falls back to Texture-associated sampler state) [1292031]
- Fixed Graph Inspector scaling that was allocating too much space to the labels [1268134]

### Fixed
### Fixed
- Fixed some issues with our Convert To Subgraph contextual menu to allow passthrough and fix inputs/outputs getting lost.
- Fixed issue where a NullReferenceException would be thrown on resetting reference name for a Shader Graph property
-

## [10.2.0] - 2020-10-19

Expand Down Expand Up @@ -64,6 +64,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Fixed an issue where unknown type Nodes (i.e. HDRP-only nodes used without HDRP package) could be copied, resulting in an unloadable graph [1288475]
- Fixed an issue where dropping HDRP-only properties from the blackboard field into the graph would soft-lock the graph [1288887]


## [10.1.0] - 2020-10-12

### Added
Expand Down
16 changes: 16 additions & 0 deletions com.unity.shadergraph/Editor/Data/Graphs/GraphData.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,7 @@ void RemoveEdgeNoValidate(IEdge e, bool reevaluateActivity = true)
if (m_NodeEdges.TryGetValue(output.objectId, out outputNodeEdges))
outputNodeEdges.Remove(e);

m_AddedEdges.Remove(e);
m_RemovedEdges.Add(e);
if (b != null)
{
Expand Down Expand Up @@ -1135,6 +1136,21 @@ public IEnumerable<IEdge> GetEdges(SlotReference s)
return edges;
}

public void GetEdges(AbstractMaterialNode node, List<IEdge> foundEdges)
{
if (m_NodeEdges.TryGetValue(node.objectId, out var edges))
{
foundEdges.AddRange(edges);
}
}

public IEnumerable<IEdge> GetEdges(AbstractMaterialNode node)
{
List<IEdge> edges = new List<IEdge>();
GetEdges(node, edges);
return edges;
}

public void ForeachHLSLProperty(Action<HLSLProperty> action)
{
foreach (var prop in properties)
Expand Down
221 changes: 155 additions & 66 deletions com.unity.shadergraph/Editor/Drawing/MaterialGraphEditWindow.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -688,13 +688,14 @@ public void ToSubGraph()
var metaKeywords = graphView.graph.keywords.Where(x => keywordNodes.Contains(x));

var copyPasteGraph = new CopyPasteGraph(graphView.selection.OfType<ShaderGroup>().Select(x => x.userData),
graphView.selection.OfType<IShaderNodeView>().Where(x => !(x.node is PropertyNode || x.node is SubGraphOutputNode)).Select(x => x.node).Where(x => x.allowedInSubGraph).ToArray(),
nodes,
graphView.selection.OfType<Edge>().Select(x => x.userData as Graphing.Edge),
graphInputs,
metaProperties,
metaKeywords,
graphView.selection.OfType<StickyNote>().Select(x => x.userData),
true);
true,
false);

// why do we serialize and deserialize only to make copies of everything in the steps below?
// is this just to clear out all non-serialized data?
Expand Down Expand Up @@ -763,6 +764,7 @@ public void ToSubGraph()
// figure out what needs remapping
var externalOutputSlots = new List<Graphing.Edge>();
var externalInputSlots = new List<Graphing.Edge>();
var passthroughSlots = new List<Graphing.Edge>();
foreach (var edge in deserialized.edges)
{
var outputSlot = edge.outputSlot;
Expand All @@ -785,6 +787,12 @@ public void ToSubGraph()
{
externalOutputSlots.Add(edge);
}
else
{
externalInputSlots.Add(edge);
externalOutputSlots.Add(edge);
passthroughSlots.Add(edge);
}
}

// Find the unique edges coming INTO the graph
Expand All @@ -800,6 +808,9 @@ public void ToSubGraph()
const int subtractHeight = 20;
var propPos = new Vector2(0, -((amountOfProps / 2) + height) - subtractHeight);

var passthroughSlotRefLookup = new Dictionary<SlotReference, SlotReference>();

var passedInProperties = new Dictionary<AbstractShaderProperty, AbstractShaderProperty>();
foreach (var group in uniqueIncomingEdges)
{
var sr = group.slotRef;
Expand All @@ -812,68 +823,78 @@ public void ToSubGraph()
: null;

AbstractShaderProperty prop;
switch (fromSlot.concreteValueType)
if (fromProperty != null && passedInProperties.TryGetValue(fromProperty, out prop))
{
case ConcreteSlotValueType.Texture2D:
prop = new Texture2DShaderProperty();
break;
case ConcreteSlotValueType.Texture2DArray:
prop = new Texture2DArrayShaderProperty();
break;
case ConcreteSlotValueType.Texture3D:
prop = new Texture3DShaderProperty();
break;
case ConcreteSlotValueType.Cubemap:
prop = new CubemapShaderProperty();
break;
case ConcreteSlotValueType.Vector4:
prop = new Vector4ShaderProperty();
break;
case ConcreteSlotValueType.Vector3:
prop = new Vector3ShaderProperty();
break;
case ConcreteSlotValueType.Vector2:
prop = new Vector2ShaderProperty();
break;
case ConcreteSlotValueType.Vector1:
prop = new Vector1ShaderProperty();
break;
case ConcreteSlotValueType.Boolean:
prop = new BooleanShaderProperty();
break;
case ConcreteSlotValueType.Matrix2:
prop = new Matrix2ShaderProperty();
break;
case ConcreteSlotValueType.Matrix3:
prop = new Matrix3ShaderProperty();
break;
case ConcreteSlotValueType.Matrix4:
prop = new Matrix4ShaderProperty();
break;
case ConcreteSlotValueType.SamplerState:
prop = new SamplerStateShaderProperty();
break;
case ConcreteSlotValueType.Gradient:
prop = new GradientShaderProperty();
break;
case ConcreteSlotValueType.VirtualTexture:
prop = new VirtualTextureShaderProperty()
{
// also copy the VT settings over from the original property (if there is one)
value = (fromProperty as VirtualTextureShaderProperty)?.value ?? new SerializableVirtualTexture()
};
break;
default:
throw new ArgumentOutOfRangeException();
}
else
{
switch (fromSlot.concreteValueType)
{
case ConcreteSlotValueType.Texture2D:
prop = new Texture2DShaderProperty();
break;
case ConcreteSlotValueType.Texture2DArray:
prop = new Texture2DArrayShaderProperty();
break;
case ConcreteSlotValueType.Texture3D:
prop = new Texture3DShaderProperty();
break;
case ConcreteSlotValueType.Cubemap:
prop = new CubemapShaderProperty();
break;
case ConcreteSlotValueType.Vector4:
prop = new Vector4ShaderProperty();
break;
case ConcreteSlotValueType.Vector3:
prop = new Vector3ShaderProperty();
break;
case ConcreteSlotValueType.Vector2:
prop = new Vector2ShaderProperty();
break;
case ConcreteSlotValueType.Vector1:
prop = new Vector1ShaderProperty();
break;
case ConcreteSlotValueType.Boolean:
prop = new BooleanShaderProperty();
break;
case ConcreteSlotValueType.Matrix2:
prop = new Matrix2ShaderProperty();
break;
case ConcreteSlotValueType.Matrix3:
prop = new Matrix3ShaderProperty();
break;
case ConcreteSlotValueType.Matrix4:
prop = new Matrix4ShaderProperty();
break;
case ConcreteSlotValueType.SamplerState:
prop = new SamplerStateShaderProperty();
break;
case ConcreteSlotValueType.Gradient:
prop = new GradientShaderProperty();
break;
case ConcreteSlotValueType.VirtualTexture:
prop = new VirtualTextureShaderProperty()
{
// also copy the VT settings over from the original property (if there is one)
value = (fromProperty as VirtualTextureShaderProperty)?.value ?? new SerializableVirtualTexture()
};
break;
default:
throw new ArgumentOutOfRangeException();
}

prop.displayName = fromProperty != null
? fromProperty.displayName
: fromSlot.concreteValueType.ToString();
prop.displayName = GraphUtil.SanitizeName(subGraph.addedInputs.Select(p => p.displayName), "{0} ({1})",
prop.displayName);
prop.displayName = fromProperty != null
? fromProperty.displayName
: fromSlot.concreteValueType.ToString();
prop.displayName = GraphUtil.SanitizeName(subGraph.addedInputs.Select(p => p.displayName), "{0} ({1})",
prop.displayName);
subGraph.AddGraphInput(prop);
if (fromProperty != null)
{
passedInProperties.Add(fromProperty, prop);
}
}

subGraph.AddGraphInput(prop);
var propNode = new PropertyNode();
{
var drawState = propNode.drawState;
Expand All @@ -885,13 +906,45 @@ public void ToSubGraph()
subGraph.AddNode(propNode);
propNode.property = prop;


Vector2 avg = Vector2.zero;
foreach (var edge in group.edges)
{
subGraph.Connect(
new SlotReference(propNode, PropertyNode.OutputSlotId),
edge.inputSlot);
externalInputNeedingConnection.Add(new KeyValuePair<IEdge, AbstractShaderProperty>(edge, prop));
if (passthroughSlots.Contains(edge) && !passthroughSlotRefLookup.ContainsKey(sr))
{
passthroughSlotRefLookup.Add(sr, new SlotReference(propNode, PropertyNode.OutputSlotId));
}
else
{
subGraph.Connect(
new SlotReference(propNode, PropertyNode.OutputSlotId),
edge.inputSlot);

int i;
var inputs = edge.inputSlot.node.GetInputSlots<MaterialSlot>().ToList();

for (i = 0; i < inputs.Count; ++i)
{
if (inputs[i].slotReference.slotId == edge.inputSlot.slotId)
{
break;
}
}
avg += new Vector2(edge.inputSlot.node.drawState.position.xMin, edge.inputSlot.node.drawState.position.center.y + 30f * i);
}
//we collapse input properties so dont add edges that are already being added
if (!externalInputNeedingConnection.Any(x => x.Key.outputSlot.slot == edge.outputSlot.slot && x.Value == prop))
{
externalInputNeedingConnection.Add(new KeyValuePair<IEdge, AbstractShaderProperty>(edge, prop));
}
}
avg /= group.edges.Count;
var pos = avg - new Vector2(150f, 0f);
propNode.drawState = new DrawState()
{
position = new Rect(pos, propNode.drawState.position.size),
expanded = propNode.drawState.expanded
};
}

var uniqueOutgoingEdges = externalInputSlots.GroupBy(
Expand All @@ -912,7 +965,7 @@ public void ToSubGraph()

foreach (var edge in group.edges)
{
var newEdge = subGraph.Connect(edge.outputSlot, inputSlotRef);
var newEdge = subGraph.Connect(passthroughSlotRefLookup.TryGetValue(edge.outputSlot, out SlotReference remap) ? remap : edge.outputSlot, inputSlotRef);
externalOutputsNeedingConnection.Add(new KeyValuePair<IEdge, IEdge>(edge, newEdge));
}
}
Expand Down Expand Up @@ -961,10 +1014,46 @@ public void ToSubGraph()
}

graphObject.graph.RemoveElements(
graphView.selection.OfType<IShaderNodeView>().Select(x => x.node).Where(x => x.allowedInSubGraph).ToArray(),
graphView.selection.OfType<IShaderNodeView>().Select(x => x.node).Where(x => !(x is PropertyNode || x is SubGraphOutputNode) && x.allowedInSubGraph).ToArray(),
new IEdge[] {},
new GroupData[] {},
graphView.selection.OfType<StickyNote>().Select(x => x.userData).ToArray());

List<GraphElement> moved = new List<GraphElement>();
foreach (var nodeView in graphView.selection.OfType<IShaderNodeView>())
{
var node = nodeView.node;
if (graphView.graph.removedNodes.Contains(node) || node is SubGraphOutputNode)
{
continue;
}

var edges = graphView.graph.GetEdges(node);
int numEdges = edges.Count();
if (numEdges == 0)
{
graphView.graph.RemoveNode(node);
}
else if (numEdges == 1 && edges.First().inputSlot.node != node) //its an output edge
{
var edge = edges.First();
int i;
var inputs = edge.inputSlot.node.GetInputSlots<MaterialSlot>().ToList();
for (i = 0; i < inputs.Count; ++i)
{
if (inputs[i].slotReference.slotId == edge.inputSlot.slotId)
{
break;
}
}
node.drawState = new DrawState()
{
position = new Rect(new Vector2(edge.inputSlot.node.drawState.position.xMin, edge.inputSlot.node.drawState.position.center.y) - new Vector2(150f, -30f * i), node.drawState.position.size),
expanded = node.drawState.expanded
};
(nodeView as GraphElement).SetPosition(node.drawState.position);
}
}
graphObject.graph.ValidateGraph();
}

Expand Down
13 changes: 8 additions & 5 deletions com.unity.shadergraph/Editor/Util/CopyPasteGraph.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ sealed class CopyPasteGraph : JsonObject
public CopyPasteGraph() {}

public CopyPasteGraph(IEnumerable<GroupData> groups, IEnumerable<AbstractMaterialNode> nodes, IEnumerable<Edge> edges,
IEnumerable<ShaderInput> inputs, IEnumerable<AbstractShaderProperty> metaProperties, IEnumerable<ShaderKeyword> metaKeywords, IEnumerable<StickyNoteData> notes, bool keepOutputEdges = false)
IEnumerable<ShaderInput> inputs, IEnumerable<AbstractShaderProperty> metaProperties, IEnumerable<ShaderKeyword> metaKeywords, IEnumerable<StickyNoteData> notes,
bool keepOutputEdges = false, bool removeOrphanEdges = true)
{
if (groups != null)
{
Expand Down Expand Up @@ -103,10 +104,12 @@ public CopyPasteGraph(IEnumerable<GroupData> groups, IEnumerable<AbstractMateria
AddMetaKeyword(metaKeyword);
}

m_Edges = m_Edges
.Distinct()
.Where(edge => nodeSet.Contains(edge.inputSlot.node) || (keepOutputEdges && nodeSet.Contains(edge.outputSlot.node)))
.ToList();
var distinct = m_Edges.Distinct();
if (removeOrphanEdges)
{
distinct = distinct.Where(edge => nodeSet.Contains(edge.inputSlot.node) || (keepOutputEdges && nodeSet.Contains(edge.outputSlot.node)));
}
m_Edges = distinct.ToList();
}

void AddGroup(GroupData group)
Expand Down

0 comments on commit 4d843ce

Please sign in to comment.