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

Tree Layout with vertical position constraints #960

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

Eddykasp
Copy link
Contributor

@Eddykasp Eddykasp commented Oct 11, 2023

resolves #861
Implementation of a tree layout that can handle vertical position constraints for nodes. Created in the scope of the bachelor thesis of cln.

Todos:

  • considerNodeModelOrder option is broken is some situations
  • node sizes are not correctly determined in some cases (e.g. when using node micro layout)
  • add tests

Sample ELK graph:

node wrapper {
  algorithm: org.eclipse.elk.vertiflex
  spacing.nodeNode: 5
  layerDistance: 40
  elk.padding: "[top=0.0,left=0.0,bottom=10.0,right=0.0]"
  layoutStrategy: BEND

  node n1 {
    verticalConstraint: 115
    label "n1"
  }
  node n2 {
    verticalConstraint: 400
    label "n2"
  }
  node n3 {
    verticalConstraint: 160
    label "n3"
  }

  edge n1 -> n2
  edge n1 -> n3

  node n4 {
    verticalConstraint: 300
    label "n4"
  }
  node n5 {
    verticalConstraint: 280
    label "n5"
  }
  node n6 {
    label "n6"
  }
  node n7 {
    label "n7"
  }
  node n8 {
    label "n8"
  }

  edge n3 -> n4
  edge n3 -> n5
  edge n3 -> n6
  edge n3 -> n7
  edge n3 -> n8
}

Straight edges vs preserved model order

tree-comparison

ClaasN and others added 25 commits August 18, 2023 12:37
add missing documentation
implement quick fixes
mark remaining issues with TODO
spacing.nodeNode is now considered when constructing outline nodes in
addition to any existing node margins.
For invalid graphs throw UnsupportedConfigurationException instead
of IllegalArgumentException
replace string with more appropriate enum edge layout strategy
remove image include
make layer distance property
add descriptions to properties
refactors all qualified names and references to the new name
Correctly compute graph size and consider padding instead of magicnumber
Number phase packages to correspond to the phases declared in the
phases enum.
@Eddykasp Eddykasp marked this pull request as ready for review October 19, 2023 13:19
- Correct the constraint validity computation
- Correct the y placement calculation for nodes without constraint
- Add more documentation
change rol and lol to right and left outline in vars and comments
allows using the node model as a tie breaker during the x placement
of nodes to avoid certain ono cases
The avoided situations are:
- mirrored layouts that are obviously better
- nodes that have the same height do not maintain the model order
plugins/org.eclipse.elk.alg.vertiflex/build.properties Outdated Show resolved Hide resolved

}

/** Split nodes into two lists, while maintaining a sensible model order for nodes on the same height. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a full javadoc if you need one?

}

/** Combine outlines of multiple siblings. */
private void bundleChildren(final ElkNode leftSubtree, final ElkNode a, final ElkNode b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method deserves a bit more of a javadoc to explain what it does.

@soerendomroes
Copy link
Contributor

@Eddykasp could you add a check-list to highlight what is still missing here?

Comment on lines +87 to +88
x1 = o1.getRelativeX();
x2 = o2.getRelativeX();
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a problem, if the outline nodes already have positions from the previous run?

// now change o1
o1 = o1.getNext();
if (o1 != null) {
x1 += o1.getRelativeX();
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding somehting to the x-coordinate may also be your problem.

@soerendomroes
Copy link
Contributor

Could you add a test for the "I do layout twice and everything breaks" problem?

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.

Tree layout with height constraints for each node
4 participants