Skip to content

Commit

Permalink
[fix] Fix another ELK rounded coordinates problem
Browse files Browse the repository at this point in the history
Problem is described here
eclipse-elk/elk#1126.
There is no simple reproduction use case because the issue was observed
in a complex customer diagram.

Change-Id: Ib70266ce1039040a19a1d09e2d93283c500c53ea
  • Loading branch information
lredor committed Feb 7, 2025
1 parent 84bb95c commit a344669
Show file tree
Hide file tree
Showing 4 changed files with 116 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,13 @@ private void addShapeLayout(final GmfLayoutCommand command, final ElkShape elkSh

View view = (View) editPart.getModel();

// The ELK coordinates use floating point, but GMF uses Integer coordinates. The accumulation of rounding could
// have side effects (switch of one pixel). To avoid the problem, we try to find the more appropriated rounding,
// above or below, according to parents coordinates.

PrecisionPoint roundedCoordinates = SiriusElkUtil.getRoundedCoordinatesAccordingToParents(elkShape);
// Compute the new location
Point newLocation = new PrecisionPoint(elkShape.getX() * scale, elkShape.getY() * scale);
Point newLocation = new PrecisionPoint(roundedCoordinates.preciseX() * scale, roundedCoordinates.preciseY() * scale);
// Border nodes are not concerned by insets.
if (view instanceof Node && !(elkShape instanceof ElkPort) && !(isRegion(editPart))) {
newLocation.translate(GMFHelper.getContainerTopLeftInsets((Node) view, true).getNegated());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*******************************************************************************
* Copyright (c) 2025 Obeo.
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Obeo - initial API and implementation
*******************************************************************************/
package org.eclipse.sirius.diagram.elk;

import org.eclipse.draw2d.geometry.Dimension;
import org.eclipse.draw2d.geometry.PrecisionPoint;
import org.eclipse.elk.core.math.KVector;
import org.eclipse.elk.core.util.ElkUtil;
import org.eclipse.elk.graph.ElkShape;

/**
* Utility methods for ELK layout-related things.
*
* @author lredor
*/
public class SiriusElkUtil {

/**
* Prevents instanciation.
*/
private SiriusElkUtil() {
}

/**
* The ELK coordinates use floating point, but GMF uses Integer coordinates. The accumulation of rounding could have
* side effects on absolute location (switch of one pixel for example). To avoid the problem, we try to find the
* more appropriated rounding, above or below, according to parents coordinates. The return value is a
* PrecisionPoint but the coordinates are Integer. PrecisionPoint is used for convenience in the following usages.
*
* @param elkShape
* The shape for which we want to get the rounded coordinates.
* @return The rounded coordinates of <code>elkShape</code>
*/
public static PrecisionPoint getRoundedCoordinatesAccordingToParents(ElkShape elkShape) {
PrecisionPoint defaultRoundedCoordinates = new PrecisionPoint(Math.toIntExact(Math.round(elkShape.getX())), Math.toIntExact(Math.round(elkShape.getY())));
if (elkShape.eContainer() instanceof ElkShape parent) {
KVector absoluteELKCoordinates = ElkUtil.absolutePosition(elkShape);
PrecisionPoint parentRoundedAbsoluteCoordinates = getRoundedAbsoluteCoordinates(parent);
PrecisionPoint roundedAbsoluteCoordinates = (PrecisionPoint) defaultRoundedCoordinates.getTranslated(parentRoundedAbsoluteCoordinates);
Dimension delta = new Dimension((int) (absoluteELKCoordinates.x - roundedAbsoluteCoordinates.preciseX()), (int) (absoluteELKCoordinates.y - roundedAbsoluteCoordinates.preciseY()));
defaultRoundedCoordinates.translate(delta);
}
return defaultRoundedCoordinates;
}

private static PrecisionPoint getRoundedAbsoluteCoordinates(ElkShape elkShape) {
PrecisionPoint roundedCoordinates = getRoundedCoordinatesAccordingToParents(elkShape);
if (elkShape.eContainer() instanceof ElkShape parent) {
roundedCoordinates.translate(getRoundedAbsoluteCoordinates(parent));
}
return roundedCoordinates;
}
}
3 changes: 2 additions & 1 deletion plugins/org.eclipse.sirius.tests.junit/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ Require-Bundle: org.eclipse.sirius.tests.sample.benchmark,
org.eclipse.elk.alg.layered;bundle-version="0.6.1",
org.apache.batik.anim;bundle-version="1.14.0",
org.apache.batik.constants;bundle-version="1.14.0",
org.eclipse.elk.core;bundle-version="0.9.0"
org.eclipse.elk.core;bundle-version="0.9.0",
org.eclipse.elk.graph;bundle-version="0.9.0"
Bundle-Activator: org.eclipse.sirius.tests.SiriusTestsPlugin
Eclipse-LazyStart: true
Bundle-Localization: plugin
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2020, 2024 Obeo.
* Copyright (c) 2020, 2025 Obeo.
* All rights reserved.
*
* Contributors:
Expand Down Expand Up @@ -27,7 +27,10 @@
import org.eclipse.draw2d.geometry.Dimension;
import org.eclipse.draw2d.geometry.Point;
import org.eclipse.draw2d.geometry.PointList;
import org.eclipse.draw2d.geometry.PrecisionPoint;
import org.eclipse.draw2d.geometry.Rectangle;
import org.eclipse.elk.graph.ElkNode;
import org.eclipse.elk.graph.util.ElkGraphUtil;
import org.eclipse.emf.common.util.TreeIterator;
import org.eclipse.emf.common.util.URI;
import org.eclipse.emf.ecore.EAttribute;
Expand Down Expand Up @@ -93,6 +96,7 @@
import org.eclipse.sirius.diagram.description.EnumLayoutValue;
import org.eclipse.sirius.diagram.description.LayoutOptionTarget;
import org.eclipse.sirius.diagram.elk.GmfLayoutCommand;
import org.eclipse.sirius.diagram.elk.SiriusElkUtil;
import org.eclipse.sirius.diagram.elk.migration.EmptyJunctionPointsStringValueStyleMigrationParticipant;
import org.eclipse.sirius.diagram.tools.api.preferences.SiriusDiagramPreferencesKeys;
import org.eclipse.sirius.diagram.tools.internal.commands.PinElementsCommand;
Expand Down Expand Up @@ -1906,6 +1910,46 @@ public void testMigrationIsNeededOnData() {
assertTrue("The migration must be required on test data.", migrationVersion.compareTo(loadedVersion) > 0); //$NON-NLS-1$
}

/**
* Check that when floating coordinates are returned by ELK, the rounded conversion is OK and that the delta of
* absolute location is never higher than 0.5.
*/
public void testRoundedCoordinates() {
PrecisionPoint location = new PrecisionPoint(10.5, 10.5);
ElkNode parent = createNode(null, "parent", location);
ElkNode child1 = createNode(parent, "child1", location);
ElkNode child2 = createNode(child1, "child2", location);
ElkNode child3 = createNode(child2, "child3", location);
PrecisionPoint parentLocation = SiriusElkUtil.getRoundedCoordinatesAccordingToParents(parent);
assertEquals("The rounded coordinates are not the expected ones for " + parent.getIdentifier(), new PrecisionPoint(11, 11), parentLocation);
PrecisionPoint child1Location = SiriusElkUtil.getRoundedCoordinatesAccordingToParents(child1);
assertEquals("The rounded coordinates are not the expected ones for " + child1.getIdentifier(), new PrecisionPoint(10, 10), child1Location);
assertEquals("The absolute coordinates are not the expected ones for " + child1.getIdentifier(), new PrecisionPoint(21, 21), (PrecisionPoint) child1Location.translate(parentLocation), 0.5);
PrecisionPoint child2Location = SiriusElkUtil.getRoundedCoordinatesAccordingToParents(child2);
assertEquals("The rounded coordinates are not the expected ones for " + child2.getIdentifier(), new PrecisionPoint(11, 11), child2Location);
assertEquals("The absolute coordinates are not the expected ones for " + child1.getIdentifier(), new PrecisionPoint(31.5, 31.5), (PrecisionPoint) child2Location.translate(child1Location),
0.5);
PrecisionPoint child3Location = SiriusElkUtil.getRoundedCoordinatesAccordingToParents(child3);
assertEquals("The rounded coordinates are not the expected ones for " + child3.getIdentifier(), new PrecisionPoint(10, 10), child3Location);
assertEquals("The absolute coordinates are not the expected ones for " + child3.getIdentifier(), new PrecisionPoint(42, 42), (PrecisionPoint) child3Location.translate(child2Location), 0.5);
}

private ElkNode createNode(ElkNode parent, String identifier, PrecisionPoint location) {
ElkNode node = ElkGraphUtil.createNode(parent);
node.setIdentifier(identifier);
node.setLocation(location.preciseX(), location.preciseY());
return node;
}

/**
* Asserts that two precision points are equal concerning a delta. If they are not an AssertionFailedError is thrown
* with the given message. If the expected value, x or y, is infinity then the delta value is ignored.
*/
private void assertEquals(String message, PrecisionPoint expected, PrecisionPoint actual, double delta) {
assertEquals(message, expected.preciseX(), actual.preciseX(), delta);
assertEquals(message, expected.preciseY(), actual.preciseY(), delta);
}

private void checkRoutingStyle(AbstractDiagramEdgeEditPart edgeEditPart, String message, String edgeName, int expectedRoutingStyle) {
String fullMessage = MessageFormat.format(message, edgeName);
View notationView = edgeEditPart.getNotationView();
Expand Down

0 comments on commit a344669

Please sign in to comment.