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.

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

Node AutoLayout #11164

Closed
wants to merge 4 commits into from
Closed

Node AutoLayout #11164

wants to merge 4 commits into from

Conversation

QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Oct 6, 2020

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

Auto layout the newly created node from node auto complete.

Instead of the existing auto layout algorithm we are using for clean up layout which makes the initial node jumpy, I am simply putting the new node right next to the input port and arrange some Y offset based on inputport index.

AutoLayout_v3

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@QilongTang QilongTang changed the title trial commit Node AutoLayout Oct 6, 2020
@QilongTang QilongTang added the WIP label Oct 6, 2020
@QilongTang QilongTang removed the WIP label Oct 9, 2020
@QilongTang QilongTang marked this pull request as ready for review October 9, 2020 01:10
// Placing the new node to the left of initial node
var adjustedX = initialNode.X - 200;
dynamoViewModel.ExecuteCommand(new DynamoModel.CreateAndConnectNodeCommand(id, portModel.Owner.GUID,
nodeCreationName, 0, portModel.Index, adjustedX, adjustedY, false, false));
Copy link
Contributor

@aparajit-pratap aparajit-pratap Oct 9, 2020

Choose a reason for hiding this comment

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

Can we split this step into a CreateNodeCommand step and then add a new command called something like PlaceAndConnectCommand? That way we can first create the new node, get its height and width, and use the height and width to place it relative to the original node. In other words, the PlaceAndConnectCommand
will be a combination of 2 steps: 1) place the node based on a calculated X and Y position, and 2) call the
MakeConnectionCommand. This way we might be able to do away with these hard-coded values.

Copy link
Contributor Author

@QilongTang QilongTang Oct 9, 2020

Choose a reason for hiding this comment

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

@aparajit-pratap I did thought about that before implementation but the height and width for the new nodes are always default to 100 for node model so not really useful to get the actual height and width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
@aparajit-pratap This is what I am referring to. I already considered this solution before finalizing the PR. For now there isn't a way I can see we can get the actual height and width for the newly placed node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another slightly less important reason is about undo/redo, the current implementation only requires one undo to get back to the previous state versus the split way will need to be done by two undos.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying something else, will get back to you on this PR, I may commit directly to this PR if I can get something working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't get it to work. In any case this is the problem with this approach right now:
create_and_connect_nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, placing any slider or any giant node will result in such problem but I dont see a way to be smarter yet..

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

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

@QilongTang I think we can close this PR :)

@QilongTang QilongTang closed this Oct 14, 2020
@QilongTang QilongTang deleted the NodeAucoLayout branch October 27, 2020 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants