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.

Sign up for GitHub

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

[PR] [WIP] remove 'spacing.labelNode', add 'nodeLabels.margin' #621

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

[PR] [WIP] remove 'spacing.labelNode', add 'nodeLabels.margin' #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aaf32e1
core: #576 turned 'spacing.labelNode' into 'nodeLabels.margin'
uruuru May 19, 2020
e2bcc04
layered: #576 adjustment after 'labelNode' removal
uruuru May 19, 2020
2402d99
core.nodespacing: #576 adjusted NodeMarginCalculator
uruuru May 19, 2020
c105e7b
core.nodespacing: #576 further adjustments
uruuru May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 38 additions & 44 deletions ...lipse.elk.alg.common/src/org/eclipse/elk/alg/common/nodespacing/NodeMarginCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.eclipse.elk.core.options.CoreOptions;
import org.eclipse.elk.core.options.EdgeLabelPlacement;
import org.eclipse.elk.core.options.PortLabelPlacement;
import org.eclipse.elk.core.util.IndividualSpacings;
import org.eclipse.elk.core.util.adapters.GraphAdapters.EdgeAdapter;
import org.eclipse.elk.core.util.adapters.GraphAdapters.GraphAdapter;
import org.eclipse.elk.core.util.adapters.GraphAdapters.LabelAdapter;
Expand Down Expand Up @@ -109,30 +110,20 @@ public NodeMarginCalculator excludeEdgeHeadTailLabels() {
* Calculates and assigns margins to all nodes.
*/
public void process() {
double spacing = adapter.getProperty(CoreOptions.SPACING_LABEL_NODE);

// Iterate through all nodes
for (NodeAdapter<?> node : adapter.getNodes()) {
processNode(node, spacing);
processNode(node);
}
}

/**
* Calculates and assigns margins to the given node. The node is expected to be part of the graph the calculator
* was created for.
* Calculates and assigns margins to the given node. The node is expected to be part of the graph the calculator was
* created for.
*/
public void processNode(final NodeAdapter<?> node) {
double spacing = adapter.getProperty(CoreOptions.SPACING_LABEL_NODE);
processNode(node, spacing);
}

/**
* Calculates the margin of the given node.
*
* @param node the node whose margin to calculate.
* @param labelSpacing label spacing set on the layered graph.
*/
private void processNode(final NodeAdapter<?> node, final double labelSpacing) {
ElkMargin desiredNodeMargin = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.NODE_LABELS_MARGIN);

// This will be our bounding box. We'll start with one that's the same size
// as our node, and at the same position.
ElkRectangle boundingBox = new ElkRectangle(
Expand Down Expand Up @@ -185,29 +176,33 @@ private void processNode(final NodeAdapter<?> node, final double labelSpacing) {
}

// End labels of edges connected to the port
// TODO cds: layered calls #excludeEdgeHeadTailLabels, is this ever active?
Copy link
Contributor

Choose a reason for hiding this comment

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

ELK Layered does its own end label processing in EndLabelPreprocessor and EndLabelPostprocessor. I think this code wasmeant for algorithms that do not implement their own sophisticated end label processing.

if (includeEdgeHeadTailLabels) {
KVector requiredPortLabelSpace = new KVector(-labelSpacing, -labelSpacing);

// TODO: maybe leave space for manually placed ports
KVector requiredPortLabelSpace = new KVector(-desiredNodeMargin.left, -desiredNodeMargin.top);

// TODO cds: it's not margin but spacing between pairs of labels, right?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, between multiple labels that all belong to the same graph element.

double labelLabelSpacing =
IndividualSpacings.getIndividualOrInherited(node, CoreOptions.SPACING_LABEL_LABEL);
// TODO: maybe leave space for manually placed ports
if (node.getProperty(CoreOptions.PORT_LABELS_PLACEMENT) == PortLabelPlacement.OUTSIDE) {
for (LabelAdapter<?> label : port.getLabels()) {
requiredPortLabelSpace.x += label.getSize().x + labelSpacing;
requiredPortLabelSpace.y += label.getSize().y + labelSpacing;
// TODO cds: why spacing in both directions? Doesn't it depend on the stacking direction?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this looks wrong.

requiredPortLabelSpace.x += label.getSize().x + labelLabelSpacing;
requiredPortLabelSpace.y += label.getSize().y + labelLabelSpacing;
}
}

requiredPortLabelSpace.x = Math.max(requiredPortLabelSpace.x, 0.0);
requiredPortLabelSpace.y = Math.max(requiredPortLabelSpace.y, 0.0);

processEdgeHeadTailLabels(boundingBox, port.getOutgoingEdges(), port.getIncomingEdges(),
node, port, requiredPortLabelSpace, labelSpacing);
processEdgeHeadTailLabels(boundingBox, port.getOutgoingEdges(), port.getIncomingEdges(), node, port,
requiredPortLabelSpace);
}
}

// Process end labels of edges directly connected to the node
if (includeEdgeHeadTailLabels) {
processEdgeHeadTailLabels(boundingBox, node.getOutgoingEdges(), node.getIncomingEdges(),
node, null, null, labelSpacing);
processEdgeHeadTailLabels(boundingBox, node.getOutgoingEdges(), node.getIncomingEdges(), node, null, null);
}

// Reset the margin
Expand All @@ -230,20 +225,18 @@ private void processNode(final NodeAdapter<?> node, final double labelSpacing) {
* @param port the port if the edges are connected to one.
* @param portLabelSpace if the edges are connected to a port, this is the space required to
* place the port's labels.
* @param labelSpacing label spacing.
*/
private void processEdgeHeadTailLabels(final ElkRectangle boundingBox,
final Iterable<EdgeAdapter<?>> outgoingEdges, final Iterable<EdgeAdapter<?>> incomingEdges,
final NodeAdapter<?> node, final PortAdapter<?> port, final KVector portLabelSpace,
final double labelSpacing) {
final NodeAdapter<?> node, final PortAdapter<?> port, final KVector portLabelSpace) {

ElkRectangle labelBox = new ElkRectangle();

// For each edge, the tail labels of outgoing edges ...
for (EdgeAdapter<?> edge : outgoingEdges) {
for (LabelAdapter<?> label : edge.getLabels()) {
if (label.getProperty(CoreOptions.EDGE_LABELS_PLACEMENT) == EdgeLabelPlacement.TAIL) {
computeLabelBox(labelBox, label, false, node, port, portLabelSpace, labelSpacing);
computeLabelBox(labelBox, label, false, node, port, portLabelSpace);
boundingBox.union(labelBox);
}
}
Expand All @@ -253,7 +246,7 @@ private void processEdgeHeadTailLabels(final ElkRectangle boundingBox,
for (EdgeAdapter<?> edge : incomingEdges) {
for (LabelAdapter<?> label : edge.getLabels()) {
if (label.getProperty(CoreOptions.EDGE_LABELS_PLACEMENT) == EdgeLabelPlacement.HEAD) {
computeLabelBox(labelBox, label, true, node, port, portLabelSpace, labelSpacing);
computeLabelBox(labelBox, label, true, node, port, portLabelSpace);
boundingBox.union(labelBox);
}
}
Expand All @@ -272,11 +265,10 @@ private void processEdgeHeadTailLabels(final ElkRectangle boundingBox,
* directly to the node.
* @param portLabelSpace if the edges are connected to a port, this is the space required to
* place the port's labels.
* @param labelSpacing label spacing.
*/
private void computeLabelBox(final ElkRectangle labelBox, final LabelAdapter<?> label,
final boolean incomingEdge, final NodeAdapter<?> node, final PortAdapter<?> port,
final KVector portLabelSpace, final double labelSpacing) {
final KVector portLabelSpace) {

labelBox.x = node.getPosition().x;
labelBox.y = node.getPosition().y;
Expand All @@ -288,48 +280,50 @@ private void computeLabelBox(final ElkRectangle labelBox, final LabelAdapter<?>
labelBox.width = label.getSize().x;
labelBox.height = label.getSize().y;

ElkMargin desiredNodeMargin = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.NODE_LABELS_MARGIN);

if (port == null) {
// The edge is connected directly to the node
if (incomingEdge) {
// Assume the edge enters the node at its western side
labelBox.x -= labelSpacing + label.getSize().x;
labelBox.x -= desiredNodeMargin.left + label.getSize().x;
} else {
// Assume the edge leaves the node at its eastern side
labelBox.x += node.getSize().x + labelSpacing;
labelBox.x += node.getSize().x + desiredNodeMargin.right;
}
} else {
switch (port.getSide()) {
case UNDEFINED:
case EAST:
labelBox.x += port.getSize().x
+ labelSpacing
+ desiredNodeMargin.left
+ portLabelSpace.x
+ labelSpacing;
+ desiredNodeMargin.right; // TODO cds: I feel this is wrong (same below)
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 the idea here was that there's the width of the node, then a bit of space between it and the port labels, then the width of the port labels, and then another bit of space between them and the edge end labels. In fact, this might make the node label margin a little strange, perhaps. It needs to be applied between a node and its port labels, and then again between those and the end labels... Hm...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and outside node labels as well...

break;

case WEST:
labelBox.x -= labelSpacing
labelBox.x -= desiredNodeMargin.left
+ portLabelSpace.x
+ labelSpacing
+ desiredNodeMargin.right
+ label.getSize().x;
break;

case NORTH:
labelBox.x += port.getSize().x
+ labelSpacing;
labelBox.y -= labelSpacing
+ desiredNodeMargin.left;
labelBox.y -= desiredNodeMargin.top
+ portLabelSpace.y
+ labelSpacing
+ desiredNodeMargin.bottom
+ label.getSize().y;
break;

case SOUTH:
labelBox.x += port.getSize().x
+ labelSpacing;
+ desiredNodeMargin.left;
labelBox.y += port.getSize().y
+ labelSpacing
+ desiredNodeMargin.top
+ portLabelSpace.y
+ labelSpacing;
+ desiredNodeMargin.bottom;
break;
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...lipse.elk.alg.common/src/org/eclipse/elk/alg/common/nodespacing/internal/NodeContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public final class NodeContext {
/** Space to leave around the node label area. */
public final ElkPadding nodeLabelsPadding;
/** Space between a node and its outside labels. */
public final double nodeLabelSpacing;
public final ElkMargin nodeLabelsMargin;
/** Space between two labels. */
public final double labelLabelSpacing;
/** Space between two different label cells. */
Expand Down Expand Up @@ -144,7 +144,7 @@ public NodeContext(final GraphAdapter<?> parentGraph, final NodeAdapter<?> node)

// Copy spacings for convenience
nodeLabelsPadding = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.NODE_LABELS_PADDING);
nodeLabelSpacing = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.SPACING_LABEL_NODE);
nodeLabelsMargin = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.NODE_LABELS_MARGIN);
labelLabelSpacing = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.SPACING_LABEL_LABEL);
portPortSpacing = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.SPACING_PORT_PORT);
portLabelSpacing = IndividualSpacings.getIndividualOrInherited(node, CoreOptions.SPACING_LABEL_PORT);
Expand Down
9 changes: 5 additions & 4 deletions ...n/src/org/eclipse/elk/alg/common/nodespacing/internal/algorithm/NodeLabelCellCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,22 +108,23 @@ private static void createNodeLabelCellContainers(final NodeContext nodeContext,
if (!onlyInside) {
StripContainerCell northContainer = new StripContainerCell(
Strip.HORIZONTAL, symmetry, nodeContext.labelCellSpacing);
northContainer.getPadding().bottom = nodeContext.nodeLabelSpacing;
// Note that top margin translates to bottom padding (likewise for the cases below)
northContainer.getPadding().bottom = nodeContext.nodeLabelsMargin.top;
nodeContext.outsideNodeLabelContainers.put(PortSide.NORTH, northContainer);

StripContainerCell southContainer = new StripContainerCell(
Strip.HORIZONTAL, symmetry, nodeContext.labelCellSpacing);
southContainer.getPadding().top = nodeContext.nodeLabelSpacing;
southContainer.getPadding().top = nodeContext.nodeLabelsMargin.bottom;
nodeContext.outsideNodeLabelContainers.put(PortSide.SOUTH, southContainer);

StripContainerCell westContainer = new StripContainerCell(
Strip.VERTICAL, symmetry, nodeContext.labelCellSpacing);
westContainer.getPadding().right = nodeContext.nodeLabelSpacing;
westContainer.getPadding().right = nodeContext.nodeLabelsMargin.left;
nodeContext.outsideNodeLabelContainers.put(PortSide.WEST, westContainer);

StripContainerCell eastContainer = new StripContainerCell(
Strip.VERTICAL, symmetry, nodeContext.labelCellSpacing);
eastContainer.getPadding().left = nodeContext.nodeLabelSpacing;
eastContainer.getPadding().left = nodeContext.nodeLabelsMargin.right;
nodeContext.outsideNodeLabelContainers.put(PortSide.EAST, eastContainer);
}
}
Expand Down
6 changes: 2 additions & 4 deletions ...g/eclipse/elk/alg/common/nodespacing/internal/algorithm/PortLabelPlacementCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,10 @@ private static void constrainedInsidePortLabelPlacement(final NodeContext nodeCo
ElkRectangle labelContainerRect = insidePortLabelContainer.getCellRectangle();
double leftBorder = labelContainerRect.x + ElkMath.maxd(
insidePortLabelContainer.getPadding().left,
nodeContext.surroundingPortMargins.left,
nodeContext.nodeLabelSpacing);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this completely here as the code handles ports inside of the node.

nodeContext.surroundingPortMargins.left);
double rightBorder = labelContainerRect.x + labelContainerRect.width - ElkMath.maxd(
insidePortLabelContainer.getPadding().right,
nodeContext.surroundingPortMargins.right,
nodeContext.nodeLabelSpacing);
nodeContext.surroundingPortMargins.right);

// Obtain a rectangle strip overlap remover, which will actually do most of the work
RectangleStripOverlapRemover overlapRemover = RectangleStripOverlapRemover
Expand Down
4 changes: 2 additions & 2 deletions plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/Layered.melk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ algorithm layered(LayeredLayoutProvider) {
supports org.eclipse.elk.spacing.edgeNode
supports org.eclipse.elk.spacing.labelLabel
supports org.eclipse.elk.spacing.labelPort
supports org.eclipse.elk.spacing.labelNode
supports org.eclipse.elk.spacing.nodeNode
supports org.eclipse.elk.spacing.nodeSelfLoop
supports org.eclipse.elk.spacing.portPort
Expand Down Expand Up @@ -103,8 +102,9 @@ algorithm layered(LayeredLayoutProvider) {
supports org.eclipse.elk.nodeSize.constraints
supports org.eclipse.elk.nodeSize.options
supports org.eclipse.elk.direction = Direction.UNDEFINED
supports org.eclipse.elk.nodeLabels.placement
supports org.eclipse.elk.nodeLabels.margin
supports org.eclipse.elk.nodeLabels.padding
supports org.eclipse.elk.nodeLabels.placement
supports org.eclipse.elk.portLabels.placement
supports org.eclipse.elk.portLabels.nextToPortIfPossible
supports org.eclipse.elk.portLabels.treatAsGroup
Expand Down
2 changes: 1 addition & 1 deletion plugins/org.eclipse.elk.alg.layered/src/org/eclipse/elk/alg/layered/options/Spacings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private void precalculateNodeTypeSpacings() {
nodeTypeSpacing(NodeType.NORTH_SOUTH_PORT, NodeType.EXTERNAL_PORT,
LayeredOptions.SPACING_EDGE_EDGE); // TODO
nodeTypeSpacing(NodeType.NORTH_SOUTH_PORT, NodeType.LABEL,
LayeredOptions.SPACING_LABEL_NODE);
LayeredOptions.SPACING_NODE_NODE_BETWEEN_LAYERS);

// external
nodeTypeSpacing(NodeType.EXTERNAL_PORT,
Expand Down
24 changes: 13 additions & 11 deletions plugins/org.eclipse.elk.core/src/org/eclipse/elk/core/Core.melk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,16 +337,6 @@ group spacing {
targets parents
}

option labelNode: double {
label "Label Node Spacing"
description
"Spacing to be preserved between labels and the border of node they are associated with.
Note that the placement of a label is influenced by the 'nodelabels.placement' option."
default = 5
lowerBound = 0.0
targets parents
}

option labelPort: double {
label "Label Port Spacing"
description
Expand Down Expand Up @@ -430,11 +420,23 @@ group partitioning {

// --- NODE LABELS
group nodeLabels {

advanced option margin: ElkMargin {
label "Node Label Margin"
description
"Define margin for node labels that are placed outside of a node (see 'nodeLabels.placement').
The margin affects not only the node's direct labels but also the labels of the node's
ports and certain edges. "
default = new ElkMargin(5)
targets nodes
}

advanced option padding: ElkPadding {
label "Node Label Padding"
description
"Define padding for node labels that are placed inside of a node."
"Define padding for node labels that are placed inside of a node (see 'nodeLabels.placement').
The padding affects not only the node's direct labels but also the labels of the node's
ports and certain edges. "
default = new ElkPadding(5)
targets nodes
}
Expand Down