DYN-9014: Remove inline WatchNodes while maintaining port connections #16884
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9014
There was a problem hiding this comment.
Pull request overview
This PR fixes deletion behavior for inline Watch nodes by rewiring upstream/downstream connectors and preserving connector pins during the reconnection.
Changes:
- Capture upstream→Watch→downstream connection data (including pin locations) before deletion.
- Recreate direct connectors after deletion and re-add connector pins onto the new connectors.
- Add NUnit unit tests for reconnection behavior and pin preservation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/DynamoCoreTests/Models/DynamoModelEventsTest.cs | Adds unit tests validating reconnection and connector-pin preservation when deleting inline Watch nodes. |
| src/DynamoCore/Graph/Workspaces/UndoRedo.cs | Implements collection of rewire data and creation of replacement connectors (including pin recreation) during deletion. |
| // Recreate pins from both watch connectors on the new connector. | ||
| foreach (var pinLocation in rewire.PinLocations) | ||
| { | ||
| var connectorPinModel = new ConnectorPinModel( | ||
| pinLocation.X, | ||
| pinLocation.Y, | ||
| Guid.NewGuid(), | ||
| connector.GUID); | ||
| connector.AddPin(connectorPinModel); | ||
| undoRecorder.RecordCreationForUndo(connectorPinModel); | ||
| } |
There was a problem hiding this comment.
The implementation recreates pins for every entry in rewire.PinLocations without deduplication, but the PR description says pins are “deduplicating by location”. If PinLocations contains duplicates (likely when input/output share the same pin location), this will create duplicate pins. Deduplicate by (X, Y) before creating ConnectorPinModels (either when collecting or immediately before the loop).
There was a problem hiding this comment.
We rebuild pins from both upstream and downstream sides of the deleted Watch node, and if one upstream connector fans out to multiple downstream connectors, that upstream pin location is intentionally replayed onto each new connector.
So same coordinates across different connectors are not true duplicates.
We should only dedupe when duplicates occur within the same connector (same connector + same location), not globally by (x, y).
| if (node is null) return false; | ||
| return string.Equals(node.GetOriginalName(), "Watch", StringComparison.Ordinal); |
There was a problem hiding this comment.
Identifying inline Watch nodes via GetOriginalName() == "Watch" is brittle (renames/localization/custom nodes can break this). Prefer a type-based check (e.g., node is Watch) or a more stable identifier used elsewhere in Dynamo to distinguish Watch nodes.
| if (node is null) return false; | |
| return string.Equals(node.GetOriginalName(), "Watch", StringComparison.Ordinal); | |
| return node is Watch; |
There was a problem hiding this comment.
UndoRedo is in DynamoCore, which cannot reference CoreNodeModels.Watch directly.
I switched to a runtime-type-name check with GetOriginalName fallback to keep it type-identity based without violating assembly boundaries.
|
1 participant
Purpose
This PR addresses DYN-9014.
Changes:
Declarations
Check these if you believe they are true
Release Notes
Dynamo now preserves port connections when deleting inline Watch nodes by rewiring upstream/downstream connectors.
Reviewers
@zeusongit
@DynamoDS/eidos
FYIs
@dnenov