Changeset 16781 in josm for trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java

Timestamp:

2020-07-17T12:07:19+02:00 (4 years ago)

Author:

GerdP

Message:

fix #19432: Split way: AIOOB: Problem with member check with duplicate members

• don't store position of member, instead always search it
• improve calculation of missing(incomplete) way members and store the result instead of calculating it twice
• add unit test

File:

• trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java (modified) (16 diffs)

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

@@ -355 +355 @@
         // If there are relations that cannot be split properly without downloading more members,
         // present the user with an option to do so, or to abort the split.
-        List<Relation> relationsNeedingMoreMembers = new ArrayList<>();
+        Set<Relation> relationsNeedingMoreMembers = new HashSet<>();
         Set<OsmPrimitive> incompleteMembers = new HashSet<>();
         for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
-            if (!relationAnalysis.needsMoreMembers()) continue;
-
-            Relation relation = relationAnalysis.getRelation();
-            int position = relationAnalysis.getPosition();
-            int membersCount = relation.getMembersCount();
-
-            // Mark the neighbouring members for downloading if these are ways too.
-            relationsNeedingMoreMembers.add(relation);
-            RelationMember rmPrev = position == 0
-                    ? relation.getMember(membersCount - 1)
-                    : relation.getMember(position - 1);
-            RelationMember rmNext = position == membersCount - 1
-                    ? relation.getMember(0)
-                    : relation.getMember(position + 1);
-
-            if (rmPrev != null && rmPrev.isWay()) {
-                incompleteMembers.add(rmPrev.getWay());
-            }
-            if (rmNext != null && rmNext.isWay()) {
-                incompleteMembers.add(rmNext.getWay());
+            if (!relationAnalysis.getNeededIncompleteMembers().isEmpty()) {
+                incompleteMembers.addAll(relationAnalysis.getNeededIncompleteMembers());
+                relationsNeedingMoreMembers.add(relationAnalysis.getRelation());
             }
         }
@@ -397 +380 @@
                     } else {
                         // Ask the user.
-                        missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers);
+                        missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers.size());
                     }
                     break;
@@ -469 +452 @@
             boolean isOrderedRelation = "route".equals(type) || "multipolygon".equals(type) || "boundary".equals(type);
 
-            int ic = 0;
-            int ir = 0;
-            List<RelationMember> relationMembers = r.getMembers();
-            for (RelationMember rm : relationMembers) {
+            for (int ir = 0; ir < r.getMembersCount(); ir++) {
+                RelationMember rm = r.getMember(ir);
                 if (rm.getMember() == way) {
                     boolean insert = true;
@@ -496 +477 @@
                         // Attempt to determine the direction the ways in the relation are ordered.
                         Direction direction = Direction.UNKNOWN;
+                        Set<Way> missingWays = new HashSet<>();
                         if (isOrderedRelation) {
                             if (way.lastNode() == way.firstNode()) {
@@ -501 +483 @@
                                 direction = Direction.IRRELEVANT;
                             } else {
-                                boolean previousWayMemberMissing = true;
-                                boolean nextWayMemberMissing = true;
-
                                 // For ordered relations, looking beyond the nearest neighbour members is not required,
                                 // and can even cause the wrong direction to be guessed (with closed loops).
-                                if (ir - 1 >= 0 && relationMembers.get(ir - 1).isWay()) {
-                                    Way w = relationMembers.get(ir - 1).getWay();
-                                    if (!w.isIncomplete()) {
-                                        previousWayMemberMissing = false;
+                                if (ir - 1 >= 0 && r.getMember(ir - 1).isWay()) {
+                                    Way w = r.getMember(ir - 1).getWay();
+                                    if (w.isIncomplete())
+                                        missingWays.add(w);
+                                    else {
                                         if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
                                             direction = Direction.FORWARDS;
@@ -516 +496 @@
                                         }
                                     }
-                                } else {
-                                    previousWayMemberMissing = false;
                                 }
-                                if (ir + 1 < relationMembers.size() && relationMembers.get(ir + 1).isWay()) {
-                                    Way w = relationMembers.get(ir + 1).getWay();
-                                    if (!w.isIncomplete()) {
-                                        nextWayMemberMissing = false;
+                                if (ir + 1 < r.getMembersCount() && r.getMember(ir + 1).isWay()) {
+                                    Way w = r.getMember(ir + 1).getWay();
+                                    if (w.isIncomplete())
+                                        missingWays.add(w);
+                                    else {
                                         if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
                                             direction = Direction.BACKWARDS;
@@ -529 +508 @@
                                         }
                                     }
-                                } else {
-                                    nextWayMemberMissing = false;
                                 }
 
-                                if (direction == Direction.UNKNOWN
-                                        && !previousWayMemberMissing
-                                        && !nextWayMemberMissing) {
-                                    // If both the next and previous way member in the relation are already known at
-                                    // this point, and they are not connected to this one, then we can safely
-                                    // assume that the direction doesn't matter. Downloading any more members
-                                    // won't help in any case.
-                                    direction = Direction.IRRELEVANT;
-                                }
-                                if (direction == Direction.UNKNOWN && !way.getDataSet().getDataSourceBounds().isEmpty()
-                                        && !(way.firstNode().isOutsideDownloadArea()
-                                                || way.lastNode().isOutsideDownloadArea())) {
-                                    // we know the connected ways, downloading more members will not help
+                                if (direction == Direction.UNKNOWN && missingWays.isEmpty()) {
+                                    // we cannot detect the direction and no way is missing.
+                                    // We can safely assume that the direction doesn't matter.
                                     direction = Direction.IRRELEVANT;
                                 }
@@ -551 +518 @@
                         } else {
                             int k = 1;
-                            while (ir - k >= 0 || ir + k < relationMembers.size()) {
-                                if (ir - k >= 0 && relationMembers.get(ir - k).isWay()) {
-                                    Way w = relationMembers.get(ir - k).getWay();
+                            while (ir - k >= 0 || ir + k < r.getMembersCount()) {
+                                if (ir - k >= 0 && r.getMember(ir - k).isWay()) {
+                                    Way w = r.getMember(ir - k).getWay();
                                     if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
                                         direction = Direction.FORWARDS;
@@ -561 +528 @@
                                     break;
                                 }
-                                if (ir + k < relationMembers.size() && relationMembers.get(ir + k).isWay()) {
-                                    Way w = relationMembers.get(ir + k).getWay();
+                                if (ir + k < r.getMembersCount() && r.getMember(ir + k).isWay()) {
+                                    Way w = r.getMember(ir + k).getWay();
                                     if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
                                         direction = Direction.BACKWARDS;
@@ -574 +541 @@
                         }
 
-                        // We don't have enough information to determine the order of the new ways in this relation.
-                        // This may cause relations to be saved with the two new way sections in reverse order.
-                        //
-                        // This often breaks routes.
-                        //
-                        // The user should be asked to download more members, or to abort the split operation.
-                        boolean needsMoreMembers = isOrderedRelation
-                                && direction == Direction.UNKNOWN
-                                && relationMembers.size() > 1
-                                && r.hasIncompleteMembers();
-
-                        relationAnalyses.add(new RelationAnalysis(c, rm, direction, needsMoreMembers, ic));
-                        ic += indexOfWayToKeep;
+                        if (direction == Direction.UNKNOWN) {
+                            // We don't have enough information to determine the order of the new ways in this relation.
+                            // This may cause relations to be saved with the two new way sections in reverse order.
+                            //
+                            // This often breaks routes.
+                            //
+                        } else {
+                            missingWays = Collections.emptySet();
+                        }
+                        relationAnalyses.add(new RelationAnalysis(c, rm, direction, missingWays));
                     }
                 }
-                ic++;
-                ir++;
             }
 
@@ -635 +597 @@
 
     static MissingMemberStrategy offerToDownloadMissingMembersIfNeeded(Analysis analysis,
-                                                                       List<Relation> relationsNeedingMoreMembers) {
+                                                                       int numRelationsNeedingMoreMembers) {
         String[] options = {
                 tr("Yes, download the missing members"),
@@ -652 +614 @@
         if (analysis.getNumberOfRelations() == 1) {
             msgReferToRelations = tr("this relation");
-        } else if (analysis.getNumberOfRelations() == relationsNeedingMoreMembers.size()) {
+        } else if (analysis.getNumberOfRelations() == numRelationsNeedingMoreMembers) {
             msgReferToRelations = tr("these relations");
         } else {
@@ -658 +620 @@
                     "one relation",
                     "{0} relations",
-                    relationsNeedingMoreMembers.size(),
-                    relationsNeedingMoreMembers.size()
+                    numRelationsNeedingMoreMembers,
+                    numRelationsNeedingMoreMembers
             );
         }
@@ -733 +695 @@
             Relation relation = relationAnalysis.getRelation();
             Direction direction = relationAnalysis.getDirection();
-            int position = relationAnalysis.getPosition();
+
+            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;
+                    break;
+                }
+            }
+
+            // sanity check
+            if (position < 0) {
+                throw new AssertionError("Relation member not found");
+            }
 
             int j = position;
@@ -918 +893 @@
         private final RelationMember relationMember;
         private final Direction direction;
-        private final boolean needsMoreMembers;
-        private final int position;
+        private final Set<Way> neededIncompleteMembers;
 
         RelationAnalysis(Relation relation,
                                 RelationMember relationMember,
                                 Direction direction,
-                                boolean needsMoreMembers,
-                                int position) {
+                                Set<Way> neededIncompleteMembers) {
             this.relation = relation;
             this.relationMember = relationMember;
             this.direction = direction;
-            this.needsMoreMembers = needsMoreMembers;
-            this.position = position;
+            this.neededIncompleteMembers = neededIncompleteMembers;
         }
 
@@ -941 +913 @@
         }
 
-        boolean needsMoreMembers() {
-            return needsMoreMembers;
+        public Set<Way> getNeededIncompleteMembers() {
+            return neededIncompleteMembers;
         }
 
         Relation getRelation() {
             return relation;
-        }
-
-        int getPosition() {
-            return position;
         }
     }