From a3446693e20b19e3de22bf3bbd62c967de31bfd1 Mon Sep 17 00:00:00 2001 From: Laurent Redor Date: Fri, 7 Feb 2025 13:51:46 +0100 Subject: [PATCH] [fix] Fix another ELK rounded coordinates problem Problem is described here https://github.com/eclipse-elk/elk/issues/1126. There is no simple reproduction use case because the issue was observed in a complex customer diagram. Change-Id: Ib70266ce1039040a19a1d09e2d93283c500c53ea --- .../diagram/elk/GmfLayoutEditPolicy.java | 7 ++- .../sirius/diagram/elk/SiriusElkUtil.java | 63 +++++++++++++++++++ .../META-INF/MANIFEST.MF | 3 +- .../diagram/layout/SimpleELKLayoutTest.java | 46 +++++++++++++- 4 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/SiriusElkUtil.java diff --git a/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/GmfLayoutEditPolicy.java b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/GmfLayoutEditPolicy.java index c44b4ab920..7898cb91d2 100644 --- a/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/GmfLayoutEditPolicy.java +++ b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/GmfLayoutEditPolicy.java @@ -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()); diff --git a/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/SiriusElkUtil.java b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/SiriusElkUtil.java new file mode 100644 index 0000000000..a389194b79 --- /dev/null +++ b/plugins/org.eclipse.sirius.diagram.elk/src/org/eclipse/sirius/diagram/elk/SiriusElkUtil.java @@ -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 elkShape + */ + 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; + } +} diff --git a/plugins/org.eclipse.sirius.tests.junit/META-INF/MANIFEST.MF b/plugins/org.eclipse.sirius.tests.junit/META-INF/MANIFEST.MF index 4b77ce7c81..3d0d318cac 100644 --- a/plugins/org.eclipse.sirius.tests.junit/META-INF/MANIFEST.MF +++ b/plugins/org.eclipse.sirius.tests.junit/META-INF/MANIFEST.MF @@ -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 diff --git a/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java b/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java index 99d445adf0..e50360591e 100644 --- a/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java +++ b/plugins/org.eclipse.sirius.tests.junit/src/org/eclipse/sirius/tests/unit/diagram/layout/SimpleELKLayoutTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2020, 2024 Obeo. + * Copyright (c) 2020, 2025 Obeo. * All rights reserved. * * Contributors: @@ -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; @@ -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; @@ -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();