Index: /trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java =================================================================== --- /trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 19196) +++ /trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (revision 19197) @@ -46,7 +46,8 @@ 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). - * + *

* 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,10 +78,11 @@ private final List newWays; + private static final String RESTRICTION = "restriction"; /** Map<Restriction type, type to treat it as> */ private static final Map 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,8 +169,8 @@ * Splits the nodes of {@code wayToSplit} into a list of node sequences * which are separated at the nodes in {@code splitPoints}. - * + *

* This method displays warning messages if {@code wayToSplit} and/or * {@code splitPoints} aren't consistent. - * + *

* Returns null, if building the split chunks fails. * @@ -250,5 +252,5 @@ * Splits the way {@code way} into chunks of {@code wayChunks} and replies * the result of this process in an instance of {@link SplitWayCommand}. - * + *

* 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,5 +465,5 @@ 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,37 +717,9 @@ 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 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 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,4 +753,75 @@ 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 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 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 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 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,5 +878,5 @@ switch (type) { case "connectivity": - case "restriction": + case RESTRICTION: return r.findRelationMembers("via"); case "destination_sign": @@ -849,8 +894,8 @@ * Splits the way {@code way} at the nodes in {@code atNodes} and replies * the result of this process in an instance of {@link SplitWayCommand}. - * + *

* Note that changes are not applied to the data yet. You have to * submit the command first, i.e. {@code UndoRedoHandler.getInstance().add(result)}. - * + *

* Replies null if the way couldn't be split at the given nodes. * Index: /trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 19196) +++ /trunk/test/unit/org/openstreetmap/josm/command/SplitWayCommandTest.java (revision 19197) @@ -468,10 +468,5 @@ dataSet.setSelected(splitNode); // Sanity check (preconditions -- the route should be well-formed already) - WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator(); - List 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 result = SplitWayCommand.splitWay( splitWay, @@ -479,5 +474,5 @@ 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,6 +480,96 @@ 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 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 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 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 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 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)));