Changeset 16781 in josm for trunk


Ignore:
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
Location:
trunk
Files:
2 added
2 edited

Legend:

Unmodified
Added
Removed

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

    r16302 r16781  
    355355        // If there are relations that cannot be split properly without downloading more members,
    356356        // present the user with an option to do so, or to abort the split.
    357         List<Relation> relationsNeedingMoreMembers = new ArrayList<>();
     357        Set<Relation> relationsNeedingMoreMembers = new HashSet<>();
    358358        Set<OsmPrimitive> incompleteMembers = new HashSet<>();
    359359        for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
    360             if (!relationAnalysis.needsMoreMembers()) continue;
    361 
    362             Relation relation = relationAnalysis.getRelation();
    363             int position = relationAnalysis.getPosition();
    364             int membersCount = relation.getMembersCount();
    365 
    366             // Mark the neighbouring members for downloading if these are ways too.
    367             relationsNeedingMoreMembers.add(relation);
    368             RelationMember rmPrev = position == 0
    369                     ? relation.getMember(membersCount - 1)
    370                     : relation.getMember(position - 1);
    371             RelationMember rmNext = position == membersCount - 1
    372                     ? relation.getMember(0)
    373                     : relation.getMember(position + 1);
    374 
    375             if (rmPrev != null && rmPrev.isWay()) {
    376                 incompleteMembers.add(rmPrev.getWay());
    377             }
    378             if (rmNext != null && rmNext.isWay()) {
    379                 incompleteMembers.add(rmNext.getWay());
     360            if (!relationAnalysis.getNeededIncompleteMembers().isEmpty()) {
     361                incompleteMembers.addAll(relationAnalysis.getNeededIncompleteMembers());
     362                relationsNeedingMoreMembers.add(relationAnalysis.getRelation());
    380363            }
    381364        }
    … …  
    397380                    } else {
    398381                        // Ask the user.
    399                         missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers);
     382                        missingMemberStrategy = offerToDownloadMissingMembersIfNeeded(analysis, relationsNeedingMoreMembers.size());
    400383                    }
    401384                    break;
    … …  
    469452            boolean isOrderedRelation = "route".equals(type) || "multipolygon".equals(type) || "boundary".equals(type);
    470453
    471             int ic = 0;
    472             int ir = 0;
    473             List<RelationMember> relationMembers = r.getMembers();
    474             for (RelationMember rm : relationMembers) {
     454            for (int ir = 0; ir < r.getMembersCount(); ir++) {
     455                RelationMember rm = r.getMember(ir);
    475456                if (rm.getMember() == way) {
    476457                    boolean insert = true;
    … …  
    496477                        // Attempt to determine the direction the ways in the relation are ordered.
    497478                        Direction direction = Direction.UNKNOWN;
     479                        Set<Way> missingWays = new HashSet<>();
    498480                        if (isOrderedRelation) {
    499481                            if (way.lastNode() == way.firstNode()) {
    … …  
    501483                                direction = Direction.IRRELEVANT;
    502484                            } else {
    503                                 boolean previousWayMemberMissing = true;
    504                                 boolean nextWayMemberMissing = true;
    505 
    506485                                // For ordered relations, looking beyond the nearest neighbour members is not required,
    507486                                // and can even cause the wrong direction to be guessed (with closed loops).
    508                                 if (ir - 1 >= 0 && relationMembers.get(ir - 1).isWay()) {
    509                                     Way w = relationMembers.get(ir - 1).getWay();
    510                                     if (!w.isIncomplete()) {
    511                                         previousWayMemberMissing = false;
     487                                if (ir - 1 >= 0 && r.getMember(ir - 1).isWay()) {
     488                                    Way w = r.getMember(ir - 1).getWay();
     489                                    if (w.isIncomplete())
     490                                        missingWays.add(w);
     491                                    else {
    512492                                        if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    513493                                            direction = Direction.FORWARDS;
    … …  
    516496                                        }
    517497                                    }
    518                                 } else {
    519                                     previousWayMemberMissing = false;
    520498                                }
    521                                 if (ir + 1 < relationMembers.size() && relationMembers.get(ir + 1).isWay()) {
    522                                     Way w = relationMembers.get(ir + 1).getWay();
    523                                     if (!w.isIncomplete()) {
    524                                         nextWayMemberMissing = false;
     499                                if (ir + 1 < r.getMembersCount() && r.getMember(ir + 1).isWay()) {
     500                                    Way w = r.getMember(ir + 1).getWay();
     501                                    if (w.isIncomplete())
     502                                        missingWays.add(w);
     503                                    else {
    525504                                        if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    526505                                            direction = Direction.BACKWARDS;
    … …  
    529508                                        }
    530509                                    }
    531                                 } else {
    532                                     nextWayMemberMissing = false;
    533510                                }
    534511
    535                                 if (direction == Direction.UNKNOWN
    536                                         && !previousWayMemberMissing
    537                                         && !nextWayMemberMissing) {
    538                                     // If both the next and previous way member in the relation are already known at
    539                                     // this point, and they are not connected to this one, then we can safely
    540                                     // assume that the direction doesn't matter. Downloading any more members
    541                                     // won't help in any case.
    542                                     direction = Direction.IRRELEVANT;
    543                                 }
    544                                 if (direction == Direction.UNKNOWN && !way.getDataSet().getDataSourceBounds().isEmpty()
    545                                         && !(way.firstNode().isOutsideDownloadArea()
    546                                                 || way.lastNode().isOutsideDownloadArea())) {
    547                                     // we know the connected ways, downloading more members will not help
     512                                if (direction == Direction.UNKNOWN && missingWays.isEmpty()) {
     513                                    // we cannot detect the direction and no way is missing.
     514                                    // We can safely assume that the direction doesn't matter.
    548515                                    direction = Direction.IRRELEVANT;
    549516                                }
    … …  
    551518                        } else {
    552519                            int k = 1;
    553                             while (ir - k >= 0 || ir + k < relationMembers.size()) {
    554                                 if (ir - k >= 0 && relationMembers.get(ir - k).isWay()) {
    555                                     Way w = relationMembers.get(ir - k).getWay();
     520                            while (ir - k >= 0 || ir + k < r.getMembersCount()) {
     521                                if (ir - k >= 0 && r.getMember(ir - k).isWay()) {
     522                                    Way w = r.getMember(ir - k).getWay();
    556523                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    557524                                        direction = Direction.FORWARDS;
    … …  
    561528                                    break;
    562529                                }
    563                                 if (ir + k < relationMembers.size() && relationMembers.get(ir + k).isWay()) {
    564                                     Way w = relationMembers.get(ir + k).getWay();
     530                                if (ir + k < r.getMembersCount() && r.getMember(ir + k).isWay()) {
     531                                    Way w = r.getMember(ir + k).getWay();
    565532                                    if (w.lastNode() == way.firstNode() || w.firstNode() == way.firstNode()) {
    566533                                        direction = Direction.BACKWARDS;
    … …  
    574541                        }
    575542
    576                         // We don't have enough information to determine the order of the new ways in this relation.
    577                         // This may cause relations to be saved with the two new way sections in reverse order.
    578                         //
    579                         // This often breaks routes.
    580                         //
    581                         // The user should be asked to download more members, or to abort the split operation.
    582                         boolean needsMoreMembers = isOrderedRelation
    583                                 && direction == Direction.UNKNOWN
    584                                 && relationMembers.size() > 1
    585                                 && r.hasIncompleteMembers();
    586 
    587                         relationAnalyses.add(new RelationAnalysis(c, rm, direction, needsMoreMembers, ic));
    588                         ic += indexOfWayToKeep;
     543                        if (direction == Direction.UNKNOWN) {
     544                            // We don't have enough information to determine the order of the new ways in this relation.
     545                            // This may cause relations to be saved with the two new way sections in reverse order.
     546                            //
     547                            // This often breaks routes.
     548                            //
     549                        } else {
     550                            missingWays = Collections.emptySet();
     551                        }
     552                        relationAnalyses.add(new RelationAnalysis(c, rm, direction, missingWays));
    589553                    }
    590554                }
    591                 ic++;
    592                 ir++;
    593555            }
    594556
    … …  
    635597
    636598    static MissingMemberStrategy offerToDownloadMissingMembersIfNeeded(Analysis analysis,
    637                                                                        List<Relation> relationsNeedingMoreMembers) {
     599                                                                       int numRelationsNeedingMoreMembers) {
    638600        String[] options = {
    639601                tr("Yes, download the missing members"),
    … …  
    652614        if (analysis.getNumberOfRelations() == 1) {
    653615            msgReferToRelations = tr("this relation");
    654         } else if (analysis.getNumberOfRelations() == relationsNeedingMoreMembers.size()) {
     616        } else if (analysis.getNumberOfRelations() == numRelationsNeedingMoreMembers) {
    655617            msgReferToRelations = tr("these relations");
    656618        } else {
    … …  
    658620                    "one relation",
    659621                    "{0} relations",
    660                     relationsNeedingMoreMembers.size(),
    661                     relationsNeedingMoreMembers.size()
     622                    numRelationsNeedingMoreMembers,
     623                    numRelationsNeedingMoreMembers
    662624            );
    663625        }
    … …  
    733695            Relation relation = relationAnalysis.getRelation();
    734696            Direction direction = relationAnalysis.getDirection();
    735             int position = relationAnalysis.getPosition();
     697
     698            int position = -1;
     699            for (int i = 0; i < relation.getMembersCount(); i++) {
     700                // search for identical member (can't use indexOf() as it uses equals()
     701                if (rm == relation.getMember(i)) {
     702                    position = i;
     703                    break;
     704                }
     705            }
     706
     707            // sanity check
     708            if (position < 0) {
     709                throw new AssertionError("Relation member not found");
     710            }
    736711
    737712            int j = position;
    … …  
    918893        private final RelationMember relationMember;
    919894        private final Direction direction;
    920         private final boolean needsMoreMembers;
    921         private final int position;
     895        private final Set<Way> neededIncompleteMembers;
    922896
    923897        RelationAnalysis(Relation relation,
    924898                                RelationMember relationMember,
    925899                                Direction direction,
    926                                 boolean needsMoreMembers,
    927                                 int position) {
     900                                Set<Way> neededIncompleteMembers) {
    928901            this.relation = relation;
    929902            this.relationMember = relationMember;
    930903            this.direction = direction;
    931             this.needsMoreMembers = needsMoreMembers;
    932             this.position = position;
     904            this.neededIncompleteMembers = neededIncompleteMembers;
    933905        }
    934906
    … …  
    941913        }
    942914
    943         boolean needsMoreMembers() {
    944             return needsMoreMembers;
     915        public Set<Way> getNeededIncompleteMembers() {
     916            return neededIncompleteMembers;
    945917        }
    946918
    947919        Relation getRelation() {
    948920            return relation;
    949         }
    950 
    951         int getPosition() {
    952             return position;
    953921        }
    954922    }

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

    r16200 r16781  
    367367        }
    368368    }
     369
     370    /**
     371     * Non-regression test for issue #19432 (AIOOB: Problem with member check with duplicate members)
     372     *
     373     * @throws IOException if any I/O error occurs
     374     * @throws IllegalDataException if OSM parsing fails
     375     */
     376    @Test
     377    public void testTicket19432() throws IOException, IllegalDataException {
     378        try (InputStream is = TestUtils.getRegressionDataStream(19432, "josm_split_way_exception_example.osm.bz2")) {
     379            DataSet ds = OsmReader.parseDataSet(is, null);
     380
     381            Way splitWay = (Way) ds.getPrimitiveById(632576744L, OsmPrimitiveType.WAY);
     382            Node splitNode = (Node) ds.getPrimitiveById(1523436358L, OsmPrimitiveType.NODE);
     383
     384            final Optional<SplitWayCommand> result = SplitWayCommand.splitWay(
     385                    splitWay,
     386                    SplitWayCommand.buildSplitChunks(splitWay, Collections.singletonList(splitNode)),
     387                    new ArrayList<>(),
     388                    Strategy.keepLongestChunk(),
     389                    // This split requires no additional downloads. If any are needed, this command will fail.
     390                    SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY
     391            );
     392
     393            // Should not result in aborting the split.
     394            assertTrue(result.isPresent());
     395        }
     396    }
    369397}
Note: See TracChangeset for help on using the changeset viewer.