Changeset 18539 in josm


Ignore:
Timestamp:
2022-08-16T15:50:52+02:00 (3 years ago)
Author:
taylor.smock
Message:

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

Location:
trunk
Files:
3 edited

Legend:

Unmodified
Added
Removed

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

    r17375 r18539  
    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,
    … …  
    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                                    }
    … …  
    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                                    }
    … …  
    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;
    … …  
    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;
    … …  
    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

  • trunk/src/org/openstreetmap/josm/data/osm/DataSet.java

    r18468 r18539  
    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         }
     537            children = ((Relation) primitive).getMembers().stream().map(RelationMember::getMember);
     538        } else {
     539            children = Stream.empty();
     540        }
     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    }

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

    r17375 r18539  
    22package org.openstreetmap.josm.command;
    33
     4import static org.junit.jupiter.api.Assertions.assertAll;
    45import static org.junit.jupiter.api.Assertions.assertEquals;
    56import static org.junit.jupiter.api.Assertions.assertFalse;
    … …  
    1213import java.util.Collections;
    1314import java.util.Iterator;
     15import java.util.List;
    1416import java.util.Optional;
     17import java.util.stream.Stream;
    1518
    1619import org.junit.jupiter.api.Test;
    1720import org.junit.jupiter.api.extension.RegisterExtension;
     21import org.junit.jupiter.params.ParameterizedTest;
     22import org.junit.jupiter.params.provider.Arguments;
     23import org.junit.jupiter.params.provider.MethodSource;
     24import org.junit.jupiter.params.provider.ValueSource;
    1825import org.openstreetmap.josm.TestUtils;
    1926import org.openstreetmap.josm.command.SplitWayCommand.Strategy;
    … …  
    2734import org.openstreetmap.josm.data.osm.RelationMember;
    2835import org.openstreetmap.josm.data.osm.Way;
     36import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType;
     37import org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator;
    2938import org.openstreetmap.josm.io.IllegalDataException;
    3039import org.openstreetmap.josm.io.OsmReader;
    … …  
    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    /**
    … …  
    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));
    … …  
    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);
    197         }
    198     }
    199 
    200     private void doIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne,
     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));
     205        }
     206        return builder.build();
     207    }
     208
     209    @ParameterizedTest
     210    @MethodSource
     211    void testIncompleteMembersOrderedRelationCorrectOrderTest(final boolean reverseWayOne,
    201212                                                                    final boolean reverseWayTwo,
    202213                                                                    final int indexOfWayToKeep) {
    … …  
    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}
Note: See TracChangeset for help on using the changeset viewer.