Changeset 19197 in josm

Timestamp:

2024-08-16T15:58:12+02:00 (5 months ago)

Author:

taylor.smock

Message:

Fix #21856: Split way: Wrong position of new member in PTv2 relation splitting a loop

Location:

trunk

Files:

• src/org/openstreetmap/josm/command/SplitWayCommand.java (modified) (9 diffs)
• test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java

@@ -46 +46 @@
 import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.Logging;
+
 /**
  * Splits a way into multiple ways (all identical except for their node list).
- *
+ * <p>
  * Ways are just split at the selected nodes.  The nodes remain in their
  * original order.  Selected nodes at the end of a way are ignored.
@@ -77 +78 @@
     private final List<Way> newWays;
 
+    private static final String RESTRICTION = "restriction";
     /** Map&lt;Restriction type, type to treat it as&gt; */
     private static final Map<String, String> relationSpecialTypes = new HashMap<>();
     static {
-        relationSpecialTypes.put("restriction", "restriction");
-        relationSpecialTypes.put("destination_sign", "restriction");
-        relationSpecialTypes.put("connectivity", "restriction");
+        relationSpecialTypes.put(RESTRICTION, RESTRICTION);
+        relationSpecialTypes.put("destination_sign", RESTRICTION);
+        relationSpecialTypes.put("connectivity", RESTRICTION);
     }
 
@@ -167 +169 @@
      * Splits the nodes of {@code wayToSplit} into a list of node sequences
      * which are separated at the nodes in {@code splitPoints}.
-     *
+     * <p>
      * This method displays warning messages if {@code wayToSplit} and/or
      * {@code splitPoints} aren't consistent.
-     *
+     * <p>
      * Returns null, if building the split chunks fails.
      *
@@ -250 +252 @@
      * Splits the way {@code way} into chunks of {@code wayChunks} and replies
      * the result of this process in an instance of {@link SplitWayCommand}.
-     *
+     * <p>
      * Note that changes are not applied to the data yet. You have to
      * submit the command first, i.e. {@code UndoRedoHandler.getInstance().add(result)}.
@@ -463 +465 @@
                 if (rm.getMember() == way) {
                     boolean insert = true;
-                    if (relationSpecialTypes.containsKey(type) && "restriction".equals(relationSpecialTypes.get(type))) {
+                    if (relationSpecialTypes.containsKey(type) && RESTRICTION.equals(relationSpecialTypes.get(type))) {
                         RelationInformation rValue = treatAsRestriction(r, rm, c, newWays, way, changedWayNodes);
                         if (rValue.warnme) warnings.add(WarningType.GENERIC);
@@ -715 +717 @@
             Direction direction = relationAnalysis.getDirection();
 
-            int position = -1;
             for (int i = 0; i < relation.getMembersCount(); i++) {
                 // search for identical member (can't use indexOf() as it uses equals()
                 if (rm == relation.getMember(i)) {
-                    position = i;
+                    addSortedWays(i, indexOfWayToKeep, direction, newWays, relation);
                     break;
-                }
-            }
-
-            // sanity check
-            if (position < 0) {
-                throw new AssertionError("Relation member not found");
-            }
-
-            int j = position;
-            final List<Way> waysToAddBefore = newWays.subList(0, indexOfWayToKeep);
-            for (Way wayToAdd : waysToAddBefore) {
-                RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
-                j++;
-                if (direction == Direction.BACKWARDS) {
-                    relation.addMember(position + 1, em);
-                } else {
-                    relation.addMember(j - 1, em);
-                }
-            }
-            final List<Way> waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size());
-            for (Way wayToAdd : waysToAddAfter) {
-                RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
-                j++;
-                if (direction == Direction.BACKWARDS) {
-                    relation.addMember(position, em);
-                } else {
-                    relation.addMember(j, em);
                 }
             }
@@ -779 +753 @@
                     newWays
             );
+    }
+
+    /**
+     * Ad ways in a sorted manner
+     * @param position The position of the relation member we are operating on
+     * @param indexOfWayToKeep The index of the way that is keeping history if it were in {@code newWays}
+     * @param direction The direction of the ways
+     * @param newWays The ways that are being added to the relation
+     * @param relation The relation we are operating on
+     */
+    private static void addSortedWays(final int position, final int indexOfWayToKeep, final Direction direction,
+                                      final List<Way> newWays, final Relation relation) {
+        // sanity check
+        if (position < 0) {
+            throw new AssertionError("Relation member not found");
+        }
+        final RelationMember rm = relation.getMember(position);
+        final boolean reverse = direction == Direction.BACKWARDS || needToReverseSplit(position, indexOfWayToKeep, relation, newWays);
+        final List<Way> waysToAddBefore = newWays.subList(0, indexOfWayToKeep);
+        int j = position;
+        for (Way wayToAdd : waysToAddBefore) {
+            RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
+            j++;
+            if (reverse) {
+                relation.addMember(position + 1, em);
+            } else {
+                relation.addMember(j - 1, em);
+            }
+        }
+        final List<Way> waysToAddAfter = newWays.subList(indexOfWayToKeep, newWays.size());
+        for (Way wayToAdd : waysToAddAfter) {
+            RelationMember em = new RelationMember(rm.getRole(), wayToAdd);
+            j++;
+            if (reverse) {
+                relation.addMember(position, em);
+            } else {
+                relation.addMember(j, em);
+            }
+        }
+    }
+
+    /**
+     * This is only strictly necessary when we are splitting a route where it starts to loop back.
+     * Example: way1 -> way2 -> way2 -> way1
+     *
+     * @param position         The position of the original way in the relation
+     * @param indexOfWayToKeep The index of the way to keep in relation to {@code newWays}
+     * @param relation         The relation we are working on
+     * @param newWays          The ways that are being added
+     * @return {@code true} if we need to reverse the direction of the ways
+     */
+    private static boolean needToReverseSplit(final int position, int indexOfWayToKeep, final Relation relation, final List<Way> newWays) {
+        final RelationMember rm = relation.getMember(position);
+        if (!rm.isWay()) {
+            return false;
+        }
+        final RelationMember previous = position <= 0 ? null : relation.getMember(position - 1);
+        final RelationMember next = position + 1 >= relation.getMembersCount() ? null : relation.getMember(position + 1);
+        final Way first = indexOfWayToKeep == 0 ? rm.getWay() : newWays.get(0);
+        final Way last = indexOfWayToKeep == newWays.size() ? rm.getWay() : newWays.get(newWays.size() - 1);
+        if (previous != null && previous.isWay() && previous.getWay().isUsable()) {
+            final Way compare = previous.getWay();
+            if (!(compare.isFirstLastNode(first.firstNode()) || compare.isFirstLastNode(first.lastNode()))) {
+                return true;
+            }
+        }
+        if (next != null && next.isWay() && next.getWay().isUsable()) {
+            final Way compare = next.getWay();
+            return !(compare.isFirstLastNode(last.firstNode()) || compare.isFirstLastNode(last.lastNode()));
+        }
+        return false;
     }
 
@@ -833 +878 @@
             switch (type) {
             case "connectivity":
-            case "restriction":
+            case RESTRICTION:
                 return r.findRelationMembers("via");
             case "destination_sign":
@@ -849 +894 @@
      * Splits the way {@code way} at the nodes in {@code atNodes} and replies
      * the result of this process in an instance of {@link SplitWayCommand}.
-     *
+     * <p>
      * Note that changes are not applied to the data yet. You have to
      * submit the command first, i.e. {@code UndoRedoHandler.getInstance().add(result)}.
-     *
+     * <p>
      * Replies null if the way couldn't be split at the given nodes.
      *

trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java

@@ -468 +468 @@
         dataSet.setSelected(splitNode);
         // Sanity check (preconditions -- the route should be well-formed already)
-        WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator();
-        List<WayConnectionType> links = connectionTypeCalculator.updateLinks(route, route.getMembers());
-        assertAll("All links should be connected (forward)",
-                links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));
-        assertAll("All links should be connected (backward)",
-                links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev)));
+        assertWellFormedRoute(route);
         final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
                 splitWay,
@@ -479 +474 @@
                 new ArrayList<>(),
                 Strategy.keepLongestChunk(),
-                // This split requires additional downloads but problem occured before the download
+                // This split requires additional downloads but problem occurred before the download
                 SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
         );
@@ -485 +480 @@
         result.get().executeCommand();
         // Actual check
-        connectionTypeCalculator = new WayConnectionTypeCalculator();
-        links = connectionTypeCalculator.updateLinks(route, route.getMembers());
+        assertWellFormedRoute(route);
+    }
+
+    @Test
+    void testTicket21856DoublePoints() {
+        final Way incomplete = new Way(1082474948, 10);
+        final Way way1 = TestUtils.newWay("highway=residential", new Node(new LatLon(47.9971473, 8.1274441)),
+                new Node(new LatLon(48.0011535, 8.1363531)));
+        final Way way2 =  TestUtils.newWay("highway=residential", new Node(new LatLon(48.0012294, 8.136414)),
+                new Node(new LatLon(48.0042513, 8.1378392)));
+        final Way splitWay = TestUtils.newWay("highway=residential", new Node(new LatLon(48.0011817, 8.1363763)),
+                new Node(new LatLon(48.0012086, 8.1363974)));
+        final Relation ptRelation = TestUtils.newRelation("type=route route=bus public_transport:version=2",
+                new RelationMember("", incomplete), new RelationMember("", way1),
+                new RelationMember("", splitWay), new RelationMember("", splitWay),
+                new RelationMember("", way1), new RelationMember("", incomplete));
+        final List<Node> splitLocations = splitWay.getNodes();
+        final DataSet ds = new DataSet();
+        way1.setOsmId(289122842, 10);
+        way2.setOsmId(30239125, 18);
+        splitWay.setOsmId(1082474946, 1);
+        ds.addPrimitiveRecursive(way1);
+        ds.addPrimitiveRecursive(way2);
+        ds.addPrimitiveRecursive(splitWay);
+        ds.addPrimitiveRecursive(incomplete);
+        ds.addPrimitive(ptRelation);
+        splitWay.addNode(0, way1.lastNode());
+        splitWay.addNode(way2.firstNode());
+
+        ds.setSelected(splitLocations);
+        assertWellFormedRoute(ptRelation);
+        final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
+                splitWay,
+                SplitWayCommand.buildSplitChunks(splitWay, splitLocations),
+                new ArrayList<>(),
+                Strategy.keepLongestChunk(),
+                SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
+        );
+        assertTrue(result.isPresent());
+        result.get().executeCommand();
+        // Actual check
+        assertWellFormedRoute(ptRelation);
+    }
+
+    @Test
+    void testTicket21856DoublePointsRouteMiddle() {
+        final Way incomplete = new Way(1082474948, 10);
+        final Way way1 = TestUtils.newWay("highway=residential", new Node(new LatLon(47.9971473, 8.1274441)),
+                new Node(new LatLon(48.0011535, 8.1363531)));
+        final Way way2 =  TestUtils.newWay("highway=residential", new Node(new LatLon(48.0012294, 8.136414)),
+                new Node(new LatLon(48.0042513, 8.1378392)));
+        final Way splitWay = TestUtils.newWay("highway=residential", new Node(new LatLon(48.0011817, 8.1363763)),
+                new Node(new LatLon(48.0012086, 8.1363974)));
+        final Relation ptRelation = TestUtils.newRelation("type=route route=bus public_transport:version=2",
+                new RelationMember("", incomplete), new RelationMember("", way1),
+                new RelationMember("", splitWay), new RelationMember("", way2),
+                new RelationMember("", way2), new RelationMember("", splitWay),
+                new RelationMember("", way1), new RelationMember("", incomplete));
+        final List<Node> splitLocations = splitWay.getNodes();
+        final DataSet ds = new DataSet();
+        way1.setOsmId(289122842, 10);
+        way2.setOsmId(30239125, 18);
+        splitWay.setOsmId(1082474946, 1);
+        ds.addPrimitiveRecursive(way1);
+        ds.addPrimitiveRecursive(way2);
+        ds.addPrimitiveRecursive(splitWay);
+        ds.addPrimitiveRecursive(incomplete);
+        ds.addPrimitive(ptRelation);
+        splitWay.addNode(0, way1.lastNode());
+        splitWay.addNode(way2.firstNode());
+
+        ds.setSelected(splitLocations);
+        assertWellFormedRoute(ptRelation);
+        final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
+                splitWay,
+                SplitWayCommand.buildSplitChunks(splitWay, splitLocations),
+                new ArrayList<>(),
+                Strategy.keepLongestChunk(),
+                SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
+        );
+        assertTrue(result.isPresent());
+        result.get().executeCommand();
+        // Actual check
+        assertWellFormedRoute(ptRelation);
+    }
+
+
+    private static void assertWellFormedRoute(Relation route) {
+        WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator();
+        List<WayConnectionType> links = connectionTypeCalculator.updateLinks(route, route.getMembers());
+        // NONE is the default, and is most often found on incomplete ways
+        links.removeIf(link -> link.direction == WayConnectionType.Direction.NONE);
         assertAll("All links should be connected (forward)",
                 links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));