| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-9111: prevent graph execution on input nodes autocomplete #16348
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.
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
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-91111
chubakueno
changed the title
| @@ -307,6 +307,11 @@ internal void ConsolidateTransientNodes() | |||
|
|
|||
| NodeAutoCompleteUtilities.PostAutoLayoutNodes(node.WorkspaceViewModel.Model, node.NodeModel, transientNodes.Select(x => x.NodeModel), true, true, PortViewModel.PortType, null); | |||
|
|
|||
| if (PortViewModel.PortType == PortType.Input) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want to mark as modified the "first"/left most node in the cluster - that includes the "input cluster" as well. And we use RaisesModificationEvents from below to stop all execution at the beginning of this function and enable it again at the end and only mark as modified the "first"/left most node after that.
Probably a good optimization as well maybe since it ensure a single execution for all nodes involved ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the intuitive behavior, but for some reason without this line it doesnt work (havent dug into why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually makes sense because the source node is the triggering node, so marking it as modified since we are now fulfilling the input ports makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the autocomplete workflow to avoid triggering a graph execution when nodes are autocompleted onto an input port, and temporarily suppresses modification events when adding cluster connectors.
- Adds a check to only mark input-port nodes as modified instead of requesting a run.
- Wraps connector creation in a temporary suppression of modification events.
Comments suppressed due to low confidence (2)
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs:912
- [nitpick] Rename
oldFlagto something more descriptive likepreviousRaisesModificationEventsto clarify what the flag represents.
var oldFlag = PortViewModel.NodeViewModel.NodeModel.RaisesModificationEvents;
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs:310
- Add a unit test to verify that autocompleting onto an input port does not trigger graph execution and only marks the node as modified.
if (PortViewModel.PortType == PortType.Input)
src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, marking the source node modified if DNA is triggered on input port makes sense to me.
| @@ -307,6 +307,11 @@ internal void ConsolidateTransientNodes() | |||
|
|
|||
| NodeAutoCompleteUtilities.PostAutoLayoutNodes(node.WorkspaceViewModel.Model, node.NodeModel, transientNodes.Select(x => x.NodeModel), true, true, PortViewModel.PortType, null); | |||
|
|
|||
| if (PortViewModel.PortType == PortType.Input) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually makes sense because the source node is the triggering node, so marking it as modified since we are now fulfilling the input ports makes sense.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
johnpierson
force-pushed
the
feature/DYN-9111
branch
from
9e4d1d8 to
857f243
Compare
|
Rebased from master, but this lgtm. Will merge after checks run |
Purpose
Prevent graph execution on input nodes autocomplete.
output.mp4
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Prevent graph execution on input nodes autocomplete
Reviewers
@johnpierson
@BogdanZavu
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of