Skip to content

Commit 0f1bb9b

Browse files
author
taylor.smock
committed
Fix #21856: Split way: Wrong position of new member in PTv2 relation splitting a loop
git-svn-id: https://josm.openstreetmap.de/svn/trunk@18539 0c6e7542-c601-0410-84e7-c038aed88b3b
1 parent 37a1972 commit 0f1bb9b

File tree

3 files changed

+125
-33
lines changed

3 files changed

+125
-33
lines changed

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

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ static Analysis analyseSplit(Way way,
487487
if (isOrderedRelation) {
488488
if (way.lastNode() == way.firstNode()) {
489489
// Self-closing way.
490-
direction = Direction.IRRELEVANT;
490+
direction = direction.merge(Direction.IRRELEVANT);
491491
} else {
492492
// For ordered relations, looking beyond the nearest neighbour members is not required,
493493
// and can even cause the wrong direction to be guessed (with closed loops).
@@ -497,9 +497,9 @@ static Analysis analyseSplit(Way way,
497497
missingWays.add(w);
498498
else {
499499
if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
500-
direction = Direction.FORWARDS;
500+
direction = direction.merge(Direction.FORWARDS);
501501
} else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
502-
direction = Direction.BACKWARDS;
502+
direction = direction.merge(Direction.BACKWARDS);
503503
}
504504
}
505505
}
@@ -509,9 +509,9 @@ static Analysis analyseSplit(Way way,
509509
missingWays.add(w);
510510
else {
511511
if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
512-
direction = Direction.BACKWARDS;
512+
direction = direction.merge(Direction.BACKWARDS);
513513
} else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
514-
direction = Direction.FORWARDS;
514+
direction = direction.merge(Direction.FORWARDS);
515515
}
516516
}
517517
}
@@ -528,18 +528,18 @@ static Analysis analyseSplit(Way way,
528528
if (ir - k >= 0 && r.getMember(ir - k).isWay()) {
529529
Way w = r.getMember(ir - k).getWay();
530530
if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
531-
direction = Direction.FORWARDS;
531+
direction = direction.merge(Direction.FORWARDS);
532532
} else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
533-
direction = Direction.BACKWARDS;
533+
direction = direction.merge(Direction.BACKWARDS);
534534
}
535535
break;
536536
}
537537
if (ir + k < r.getMembersCount() && r.getMember(ir + k).isWay()) {
538538
Way w = r.getMember(ir + k).getWay();
539539
if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
540-
direction = Direction.BACKWARDS;
540+
direction = direction.merge(Direction.BACKWARDS);
541541
} else if (w.firstNode() == way.lastNode() || w.lastNode() == way.lastNode()) {
542-
direction = Direction.FORWARDS;
542+
direction = direction.merge(Direction.FORWARDS);
543543
}
544544
break;
545545
}
@@ -951,7 +951,30 @@ enum Direction {
951951
FORWARDS,
952952
BACKWARDS,
953953
UNKNOWN,
954-
IRRELEVANT
954+
IRRELEVANT;
955+
956+
/**
957+
* Merge directions (this helps avoid overriding {@link #FORWARDS} with {@link #BACKWARDS}).
958+
* @param other The other direction to merge. {@link #UNKNOWN} will be overridden.
959+
* @return The merged direction
960+
*/
961+
Direction merge(Direction other) {
962+
if (this == other) {
963+
return this;
964+
}
965+
if (this == IRRELEVANT || other == IRRELEVANT ||
966+
(this == FORWARDS && other == BACKWARDS) ||
967+
(other == FORWARDS && this == BACKWARDS)) {
968+
return IRRELEVANT;
969+
}
970+
if (this == UNKNOWN) {
971+
return other;
972+
}
973+
if (other == UNKNOWN) {
974+
return this;
975+
}
976+
return UNKNOWN;
977+
}
955978
}
956979

957980
enum WarningType {

‎src/org/openstreetmap/josm/data/osm/DataSet.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -530,11 +530,16 @@ public void addPrimitive(OsmPrimitive primitive) {
530530
* @since 17981
531531
*/
532532
public void addPrimitiveRecursive(OsmPrimitive primitive) {
533+
Stream<OsmPrimitive> children;
533534
if (primitive instanceof Way) {
534-
((Way) primitive).getNodes().forEach(n -> addPrimitiveRecursive(n));
535+
children = ((Way) primitive).getNodes().stream().map(OsmPrimitive.class::cast);
535536
} else if (primitive instanceof Relation) {
536-
((Relation) primitive).getMembers().forEach(m -> addPrimitiveRecursive(m.getMember()));
537+
children = ((Relation) primitive).getMembers().stream().map(RelationMember::getMember);
538+
} else {
539+
children = Stream.empty();
537540
}
541+
// Relations may have the same member multiple times.
542+
children.filter(p -> !Objects.equals(this, p.getDataSet())).forEach(this::addPrimitiveRecursive);
538543
addPrimitive(primitive);
539544
}
540545

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

Lines changed: 85 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// License: GPL. For details, see LICENSE file.
22
package org.openstreetmap.josm.command;
33

4+
import static org.junit.jupiter.api.Assertions.assertAll;
45
import static org.junit.jupiter.api.Assertions.assertEquals;
56
import static org.junit.jupiter.api.Assertions.assertFalse;
67
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -11,10 +12,16 @@
1112
import java.util.Arrays;
1213
import java.util.Collections;
1314
import java.util.Iterator;
15+
import java.util.List;
1416
import java.util.Optional;
17+
import java.util.stream.Stream;
1518

1619
import org.junit.jupiter.api.Test;
1720
import org.junit.jupiter.api.extension.RegisterExtension;
21+
import org.junit.jupiter.params.ParameterizedTest;
22+
import org.junit.jupiter.params.provider.Arguments;
23+
import org.junit.jupiter.params.provider.MethodSource;
24+
import org.junit.jupiter.params.provider.ValueSource;
1825
import org.openstreetmap.josm.TestUtils;
1926
import org.openstreetmap.josm.command.SplitWayCommand.Strategy;
2027
import org.openstreetmap.josm.data.UndoRedoHandler;
@@ -26,6 +33,8 @@
2633
import org.openstreetmap.josm.data.osm.Relation;
2734
import org.openstreetmap.josm.data.osm.RelationMember;
2835
import org.openstreetmap.josm.data.osm.Way;
36+
import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType;
37+
import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator;
2938
import org.openstreetmap.josm.io.IllegalDataException;
3039
import org.openstreetmap.josm.io.OsmReader;
3140
import org.openstreetmap.josm.testutils.JOSMTestRules;
@@ -42,7 +51,7 @@ final class SplitWayCommandTest {
4251
*/
4352
@RegisterExtension
4453
@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
45-
public JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
54+
static JOSMTestRules test = new JOSMTestRules().main().projection().preferences();
4655

4756
/**
4857
* Unit test of {@link SplitWayCommand#findVias}.
@@ -76,22 +85,21 @@ void testFindVias() {
7685
assertEquals(Collections.singletonList(via), SplitWayCommand.findVias(r, "destination_sign"));
7786
}
7887

88+
static Stream<Arguments> testRouteRelation() {
89+
Stream.Builder<Arguments> builder = Stream.builder();
90+
for (int i = 0; i < 4; i++) {
91+
builder.add(Arguments.of(false, i));
92+
builder.add(Arguments.of(true, i));
93+
}
94+
return builder.build();
95+
}
96+
7997
/**
8098
* Unit tests of route relations.
8199
*/
82-
@Test
83-
void testRouteRelation() {
84-
doTestRouteRelation(false, 0);
85-
doTestRouteRelation(false, 1);
86-
doTestRouteRelation(false, 2);
87-
doTestRouteRelation(false, 3);
88-
doTestRouteRelation(true, 0);
89-
doTestRouteRelation(true, 1);
90-
doTestRouteRelation(true, 2);
91-
doTestRouteRelation(true, 3);
92-
}
93-
94-
void doTestRouteRelation(final boolean wayIsReversed, final int indexOfWayToKeep) {
100+
@ParameterizedTest
101+
@MethodSource
102+
void testRouteRelation(final boolean wayIsReversed, final int indexOfWayToKeep) {
95103
final DataSet dataSet = new DataSet();
96104
final Node n1 = new Node(new LatLon(1, 0));
97105
final Node n2 = new Node(new LatLon(2, 0));
@@ -186,18 +194,21 @@ void testOneMemberOrderedRelationShowsWarningTest() {
186194
assertFalse(result.isPresent());
187195
}
188196

189-
@Test
190-
void testDoIncompleteMembersOrderedRelationCorrectOrderTest() {
197+
static Stream<Arguments> testIncompleteMembersOrderedRelationCorrectOrderTest() {
198+
Stream.Builder<Arguments> builder = Stream.builder();
191199
for (int i = 0; i < 2; i++) {
192200
// All these permutations should result in a split that keeps the new parts in order.
193-
doIncompleteMembersOrderedRelationCorrectOrderTest(false, false, i);
194-
doIncompleteMembersOrderedRelationCorrectOrderTest(true, false, i);
195-
doIncompleteMembersOrderedRelationCorrectOrderTest(true, true, i);
196-
doIncompleteMembersOrderedRelationCorrectOrderTest(false, true, i);
201+
builder.add(Arguments.of(false, false, i));
202+
builder.add(Arguments.of(true, false, i));
203+
builder.add(Arguments.of(true, true, i));
204+
builder.add(Arguments.of(false, true, i));
197205
}
206+
return builder.build();
198207
}
199208

200-
private void doIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne,
209+
@ParameterizedTest
210+
@MethodSource
211+
void testIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne,
201212
final boolean reverseWayTwo,
202213
final int indexOfWayToKeep) {
203214
final DataSet dataSet = new DataSet();
@@ -433,4 +444,57 @@ void testTicket20163() throws IOException, IllegalDataException {
433444
}
434445
}
435446

447+
/**
448+
* Test case: smart ordering in routes
449+
* See #21856
450+
*/
451+
@ParameterizedTest
452+
@ValueSource(booleans = {false, true})
453+
void testTicket21856(boolean reverse) {
454+
Way way1 = TestUtils.newWay("highway=residential", TestUtils.newNode(""), TestUtils.newNode(""));
455+
way1.setOsmId(23_968_090, 1);
456+
way1.lastNode().setOsmId(6_823_898_683L, 1);
457+
Way way2 = TestUtils.newWay("highway=residential", way1.lastNode(), TestUtils.newNode(""));
458+
way2.setOsmId(728_199_307, 1);
459+
way2.lastNode().setOsmId(6_823_898_684L, 1);
460+
Node splitNode = TestUtils.newNode("");
461+
splitNode.setOsmId(6_823_906_290L, 1);
462+
Way splitWay = TestUtils.newWay("highway=service", way2.firstNode(), splitNode, TestUtils.newNode(""), way2.lastNode());
463+
// The behavior should be the same regardless of the direction of the way
464+
if (reverse) {
465+
List<Node> nodes = new ArrayList<>(splitWay.getNodes());
466+
Collections.reverse(nodes);
467+
splitWay.setNodes(nodes);
468+
}
469+
splitWay.setOsmId(728_199_306, 1);
470+
Relation route = TestUtils.newRelation("type=route route=bus", new RelationMember("", way1), new RelationMember("", splitWay),
471+
new RelationMember("", way2), new RelationMember("", way1));
472+
DataSet dataSet = new DataSet();
473+
dataSet.addPrimitiveRecursive(route);
474+
dataSet.setSelected(splitNode);
475+
// Sanity check (preconditions -- the route should be well-formed already)
476+
WayConnectionTypeCalculator connectionTypeCalculator = new WayConnectionTypeCalculator();
477+
List<WayConnectionType> links = connectionTypeCalculator.updateLinks(route, route.getMembers());
478+
assertAll("All links should be connected (forward)",
479+
links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));
480+
assertAll("All links should be connected (backward)",
481+
links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev)));
482+
final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
483+
splitWay,
484+
SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
485+
new ArrayList<>(),
486+
Strategy.keepLongestChunk(),
487+
// This split requires additional downloads but problem occured before the download
488+
SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
489+
);
490+
assertTrue(result.isPresent());
491+
result.get().executeCommand();
492+
// Actual check
493+
connectionTypeCalculator = new WayConnectionTypeCalculator();
494+
links = connectionTypeCalculator.updateLinks(route, route.getMembers());
495+
assertAll("All links should be connected (forward)",
496+
links.subList(0, links.size() - 2).stream().map(link -> () -> assertTrue(link.linkNext)));
497+
assertAll("All links should be connected (backward)",
498+
links.subList(1, links.size() - 1).stream().map(link -> () -> assertTrue(link.linkPrev)));
499+
}
436500
}

0 commit comments

Comments
 (0)