fix edge case of ShowExecutionPreview #13173
Conversation
| @@ -504,7 +504,7 @@ public PreviewState PreviewState | |||
| return PreviewState.None; | |||
| } | |||
|
|
|||
| if (Nodevm.ShowExecutionPreview) | |||
| if (Nodevm.ShowExecutionPreview || NodeEnd.ShowExecutionPreview) | |||
There was a problem hiding this comment.
@QilongTang It seems to me like we should be watching both ends of the connector.
Can you confirm this ?
| cbnGuidList.Add(bnode.guid); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The preview functionality in question is to color the connectors between nodes that need to be recomputed.
Not sure why we need to be looking inside the VM to figure out what nodes need to be recomputed.
I saw a NodeModel's have a dirty flag system. We could just go through all NodeModels to identify which one need recompute... But maybe there are cases that I am not seeing...
@aparajit-pratap @mjkkirschner @QilongTang any thoughts ?
There was a problem hiding this comment.
it's not just connectors, it should be the node UI as well, but the new UI was not updated.
There was a problem hiding this comment.
@mjkkirschner by but the new UI was not updated. do you mean this UI feature is not complete yet (not yet turned on/implemented fully yet) ?
There was a problem hiding this comment.
I mean, that when updating the NodeView's for the visual refresh it appears this feature was never tested.
There was a problem hiding this comment.
hard to see here, but the nodes are also highlighted:
There was a problem hiding this comment.
the highlight control is commented out in the nodeview xaml
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Views/Core/NodeView.xaml#L703
There was a problem hiding this comment.
Yeah, appearantly this is an incomplete feature but we should be able to make it better, once this PR is merged I can continue to work on #12908
|
SelfServe tests passed. |
| // So we need to add them to the preview list. | ||
| var addCSComputer = changeSetComputer.Clone(); | ||
|
|
||
| var addedDeltaAsts = addCSComputer.GetDeltaAstListAdded(syncData.AddedSubtrees); |
There was a problem hiding this comment.
@pinzart90 can't you use the previewChangeSetComputer here, do you need to clone it again?
There was a problem hiding this comment.
Sadly yes. If I use the previewChangeSetComputer then the call to GetDeltaAstListAdded will throw an exception with same key already exists
https://jira.autodesk.com/browse/DYN-4984
Graph preview states issue:
Connectors are not colored as invalid when connecting an existing node to a newly added node.
EngineController.PreviewGraphSyncData()is used to figure out what Nodes must be recomputed (dirty nodes).EngineController.PreviewGraphSyncData()will try to look inside the VM to figure out the entire extent of modified nodes...but newly added nodes are not registered in the VM yet (they need to be computed first)