| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8905: Adapt layout algorithm for both input and output #16286
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-8905
|
The first image looks great! I think that is ready to go for input ports. The second image; we would really like the nodes to be over to the right. We don't want them to wrap around like that. So I think it is a matter of setting the offset to be bigger? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 adapts the layout algorithm to handle both input and output nodes by changing method signatures and adjusting node positions accordingly. Key changes include updating parameters from booleans (e.g., reuseUndoGroup) to a PortType value, initializing new node position properties, and extending the layout algorithm to account for output node adjustments.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/NodeAutoCompleteViewExtension/ViewModels/NodeAutoCompleteBarViewModel.cs | Updated method call arguments to use PortViewModel.PortType instead of a boolean flag. |
| src/Engine/GraphLayout/GraphLayout.cs | Added new InitialX property and initialized it in the constructor. |
| src/DynamoCoreWpf/ViewModels/Search/NodeSearchElementViewModel.cs | Updated node auto layout call by replacing a boolean parameter with portModel.PortType. |
| src/DynamoCoreWpf/Utilities/NodeAutoCompleteUtilities.cs | Modified parameters and documentation to reflect using PortType in place of a boolean. |
| src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs | Extended auto layout methods for autocomplete to support port type adjustments and introduced anchor-based overlap avoidance. |
Comments suppressed due to low confidence (1)
src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs:114
- [nitpick] Consider adding inline documentation here to explain the rationale behind computing 'minX', 'targetMinX', and 'displacement' when 'portType' is Output, as this would improve maintainability and aid future debugging.
if (portType == PortType.Output)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 looks pretty good, but the input layouts still feel off to me.
| /// <param name="newNodes"></param> | ||
| /// <param name="misplacedNodes"></param> | ||
| /// <param name="portType"></param> | ||
| internal static List<GraphLayout.Graph> DoGraphAutoLayoutAutocomplete(this WorkspaceModel workspace, |
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.
Not so sure about having a different layout function for autocomplete. Ideally it should be the same - if changes are made to regular autolayout then we'll need to make adaptations to autocomplete also. But if we really have to do it then we'll do it.
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.
As a direct consequence of the above placing a single node from the new experience should result in the same layout arrangement as when placing the same node node from the old experience.
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.
Separated both because otherwise they get too many interspersed ifs. Ultimately i also feel more safe making adjustments to the cluster layout algorithms that i know wont modify the the manual layout algorithm and vice versa. Both are similar in behavior but also have important differences. Both decide differently which updates to save, what nodes to consider (by selected or by transient), etc.
Hm. once you add the third element it has overlapping nodes because the "pushing" algorithm will only run once. This is intended to not move the rest of the graph too much. We could make it run more times but we should keep that in mind. |
Purpose
Adapt layout algorithm for both input and output. Output nodes will always relayout to the right when confirmed, but further manual relayouts may wrap them around if it sees so fit by the Sugiyama algorithm.
Input:
2025-06-09.15-59-18.mp4
Output (showing effects of then doing a manual Ctrl+L relayout):
2025-06-13.16-05-49.mp4
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Adapt layout algorithm for both input and output.
Reviewers
@johnpierson
@BogdanZavu
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of