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.

Sign up for GitHub

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

fix edge case of ShowExecutionPreview #13173

Merged
merged 2 commits into from
Aug 8, 2022
Merged

fix edge case of ShowExecutionPreview #13173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ff4f0
fix edge case of ShowExecutionPreview
pinzart Aug 4, 2022
e04df32
Merge branch 'master' into ShowExecutionPreview
pinzart Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/DynamoCoreWpf/ViewModels/Core/ConnectorViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ public PreviewState PreviewState
return PreviewState.None;
}

if (Nodevm.ShowExecutionPreview)
if (Nodevm.ShowExecutionPreview || NodeEnd.ShowExecutionPreview)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang It seems to me like we should be watching both ends of the connector.
Can you confirm this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good one!

{
return PreviewState.ExecutionPreview;
}
Expand Down
47 changes: 34 additions & 13 deletions src/Engine/ProtoScript/Runners/LiveRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,17 +478,20 @@ public ChangeSetComputer Clone()
{
comp.currentSubTreeList.Add(subTreePairs.Key, subTreePairs.Value);
}

comp.csData = new ChangeSetData();
comp.csData.ContainsDeltaAST = csData.ContainsDeltaAST;
comp.csData.DeletedBinaryExprASTNodes = new List<AssociativeNode>(csData.DeletedBinaryExprASTNodes);
comp.csData.DeletedFunctionDefASTNodes = new List<AssociativeNode>(csData.DeletedFunctionDefASTNodes);
comp.csData.RemovedBinaryNodesFromModification = new List<AssociativeNode>(csData.RemovedBinaryNodesFromModification);
comp.csData.ModifiedNodesForRuntimeSetValue = new List<AssociativeNode>(csData.ModifiedNodesForRuntimeSetValue);
comp.csData.RemovedFunctionDefNodesFromModification = new List<AssociativeNode>(csData.RemovedFunctionDefNodesFromModification);
comp.csData.ForceExecuteASTList = new List<AssociativeNode>(csData.ForceExecuteASTList);
comp.csData.ModifiedFunctions = new List<AssociativeNode>(csData.ModifiedFunctions);
comp.csData.ModifiedNestedLangBlock = new List<AssociativeNode>(csData.ModifiedNestedLangBlock);

if (csData != null)
{
comp.csData = new ChangeSetData();
comp.csData.ContainsDeltaAST = csData.ContainsDeltaAST;
comp.csData.DeletedBinaryExprASTNodes = new List<AssociativeNode>(csData.DeletedBinaryExprASTNodes);
comp.csData.DeletedFunctionDefASTNodes = new List<AssociativeNode>(csData.DeletedFunctionDefASTNodes);
comp.csData.RemovedBinaryNodesFromModification = new List<AssociativeNode>(csData.RemovedBinaryNodesFromModification);
comp.csData.ModifiedNodesForRuntimeSetValue = new List<AssociativeNode>(csData.ModifiedNodesForRuntimeSetValue);
comp.csData.RemovedFunctionDefNodesFromModification = new List<AssociativeNode>(csData.RemovedFunctionDefNodesFromModification);
comp.csData.ForceExecuteASTList = new List<AssociativeNode>(csData.ForceExecuteASTList);
comp.csData.ModifiedFunctions = new List<AssociativeNode>(csData.ModifiedFunctions);
comp.csData.ModifiedNestedLangBlock = new List<AssociativeNode>(csData.ModifiedNestedLangBlock);
}
return comp;
}

Expand Down Expand Up @@ -601,7 +604,7 @@ private IEnumerable<AssociativeNode> GetDeltaAstListDeleted(IEnumerable<Subtree>
return deltaAstList;
}

private IEnumerable<AssociativeNode> GetDeltaAstListAdded(IEnumerable<Subtree> addedSubTrees)
internal IEnumerable<AssociativeNode> GetDeltaAstListAdded(IEnumerable<Subtree> addedSubTrees)
{
var deltaAstList = new List<AssociativeNode>();
if (addedSubTrees != null)
Expand Down Expand Up @@ -787,7 +790,6 @@ private void UpdateCachedASTList(Subtree st, List<AssociativeNode> modifiedASTLi
}
}


private IEnumerable<AssociativeNode> GetDeltaAstListModified(List<Subtree> modifiedSubTrees)
{
var deltaAstList = new List<AssociativeNode>();
Expand Down Expand Up @@ -1660,11 +1662,30 @@ private void CompileAndExecuteForDeltaExecution(List<AssociativeNode> astList)
private List<Guid> PreviewInternal(GraphSyncData syncData)
{
var previewChangeSetComputer = changeSetComputer.Clone();

// Get the list of ASTs that will be affected by syncData
var previewAstList = previewChangeSetComputer.GetDeltaASTList(syncData);

// Get the list of guid's affected by the astlist
List<Guid> cbnGuidList = previewChangeSetComputer.EstimateNodesAffectedByASTList(previewAstList);

var finalDeltaAstList = new List<AssociativeNode>();

// Newly added nodes will not be in the VM yet.
// So we need to add them to the preview list.
var addCSComputer = changeSetComputer.Clone();

var addedDeltaAsts = addCSComputer.GetDeltaAstListAdded(syncData.AddedSubtrees);
Copy link
Contributor

Choose a reason for hiding this comment

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

@pinzart90 can't you use the previewChangeSetComputer here, do you need to clone it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly yes. If I use the previewChangeSetComputer then the call to GetDeltaAstListAdded will throw an exception with same key already exists

foreach (AssociativeNode ast in addedDeltaAsts)
{
if (ast is BinaryExpressionNode bnode)
{
if (!cbnGuidList.Contains(bnode.guid))
{
cbnGuidList.Add(bnode.guid);
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

it's not just connectors, it should be the node UI as well, but the new UI was not updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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) ?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, that when updating the NodeView's for the visual refresh it appears this feature was never tested.

Copy link
Member

Choose a reason for hiding this comment

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

hard to see here, but the nodes are also highlighted:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the highlight control is commented out in the nodeview xaml
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Views/Core/NodeView.xaml#L703

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return cbnGuidList;
}

Expand Down