Index: src/org/openstreetmap/josm/data/osm/WaySegment.java =================================================================== --- src/org/openstreetmap/josm/data/osm/WaySegment.java (revision 11179) +++ src/org/openstreetmap/josm/data/osm/WaySegment.java (working copy) @@ -114,6 +114,19 @@ s2.getSecondNode().getEastNorth().east(), s2.getSecondNode().getEastNorth().north()); } + /** + * Checks whether this segment and another way segment share the same points + * @param s2 The other segment + * @return true if other way segment is the same or reverse + */ + public boolean isSimilar(WaySegment s2) { + if (getFirstNode().equals(s2.getFirstNode()) && getSecondNode().equals(s2.getSecondNode())) + return true; + if (getFirstNode().equals(s2.getSecondNode()) && getSecondNode().equals(s2.getFirstNode())) + return true; + return false; + } + @Override public String toString() { return "WaySegment [way=" + way.getUniqueId() + ", lowerIndex=" + lowerIndex + ']'; Index: src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java (revision 11179) +++ src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java (working copy) @@ -5,7 +5,7 @@ import static org.openstreetmap.josm.tools.I18n.tr; import static org.openstreetmap.josm.tools.I18n.trn; -import java.awt.geom.GeneralPath; +import java.awt.geom.Point2D; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -17,17 +17,19 @@ import java.util.Map.Entry; import java.util.Set; +import org.openstreetmap.josm.Main; import org.openstreetmap.josm.actions.CreateMultipolygonAction; import org.openstreetmap.josm.command.ChangeCommand; import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.data.coor.EastNorth; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.osm.Relation; import org.openstreetmap.josm.data.osm.RelationMember; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.WaySegment; import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon; import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.PolyData; -import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.PolyData.Intersection; import org.openstreetmap.josm.data.validation.OsmValidator; import org.openstreetmap.josm.data.validation.Severity; import org.openstreetmap.josm.data.validation.Test; @@ -74,8 +76,6 @@ /** Multipolygon member repeated (same primitive, different role) */ public static final int REPEATED_MEMBER_DIFF_ROLE = 1615; - private static volatile ElemStyles styles; - private final Set keysCheckedByAnotherTest = new HashSet<>(); /** @@ -88,7 +88,6 @@ @Override public void initialize() { - styles = MapPaintStyles.getStyles(); } @Override @@ -109,40 +108,6 @@ super.endTest(); } - private static GeneralPath createPath(List nodes) { - GeneralPath result = new GeneralPath(); - result.moveTo((float) nodes.get(0).getCoor().lat(), (float) nodes.get(0).getCoor().lon()); - for (int i = 1; i < nodes.size(); i++) { - Node n = nodes.get(i); - result.lineTo((float) n.getCoor().lat(), (float) n.getCoor().lon()); - } - return result; - } - - private static List createPolygons(List joinedWays) { - List result = new ArrayList<>(); - for (Multipolygon.PolyData way : joinedWays) { - result.add(createPath(way.getNodes())); - } - return result; - } - - private static Intersection getPolygonIntersection(GeneralPath outer, List inner) { - boolean inside = false; - boolean outside = false; - - for (Node n : inner) { - boolean contains = outer.contains(n.getCoor().lat(), n.getCoor().lon()); - inside = inside | contains; - outside = outside | !contains; - if (inside & outside) { - return Intersection.CROSSING; - } - } - - return inside ? Intersection.INSIDE : Intersection.OUTSIDE; - } - @Override public void visit(Way w) { if (!w.isArea() && ElemStyles.hasOnlyAreaElemStyle(w)) { @@ -166,16 +131,15 @@ if (r.isMultipolygon()) { checkMembersAndRoles(r); checkOuterWay(r); - checkRepeatedWayMembers(r); - - // Rest of checks is only for complete multipolygons - if (!r.hasIncompleteMembers()) { - Multipolygon polygon = new Multipolygon(r); - - // Create new multipolygon using the logics from CreateMultipolygonAction and see if roles match. - checkMemberRoleCorrectness(r); - checkStyleConsistency(r, polygon); - checkGeometry(r, polygon); + boolean hasRepeatedMembers = checkRepeatedWayMembers(r); + if (!hasRepeatedMembers) { + // Rest of checks is only for complete multipolygons + if (!r.hasIncompleteMembers()) { + boolean rolesWereChecked = checkMemberRoleCorrectness(r); + Multipolygon polygon = new Multipolygon(r); + checkStyleConsistency(r, polygon); + checkGeometry(r, polygon, rolesWereChecked); + } } } } @@ -203,13 +167,16 @@ } /** - * Create new multipolygon using the logics from CreateMultipolygonAction and see if roles match:
    + * If simple joining of ways doesn't work, create new multipolygon using the logics from + * CreateMultipolygonAction and see if roles match:
      *
    • {@link #WRONG_MEMBER_ROLE}: Role for ''{0}'' should be ''{1}''
      • *
    * @param r relation + * @return true if member roles were checked */ - private void checkMemberRoleCorrectness(Relation r) { + private boolean checkMemberRoleCorrectness(Relation r) { final Pair newMP = CreateMultipolygonAction.createMultipolygonRelation(r.getMemberPrimitives(Way.class), false); + if (newMP != null) { for (RelationMember member : r.getMembers()) { final Collection memberInNewMP = newMP.b.getMembersFor(Collections.singleton(member.getMember())); @@ -227,6 +194,7 @@ } } } + return newMP != null; } /** @@ -240,6 +208,7 @@ * @param polygon multipolygon */ private void checkStyleConsistency(Relation r, Multipolygon polygon) { + ElemStyles styles = MapPaintStyles.getStyles(); if (styles != null && !"boundary".equals(r.get("type"))) { AreaElement area = ElemStyles.getAreaElemStyle(r, false); boolean areaStyle = area != null; @@ -311,8 +280,9 @@ *
* @param r relation * @param polygon multipolygon + * @param rolesWereChecked is used to skip some tests */ - private void checkGeometry(Relation r, Multipolygon polygon) { + private void checkGeometry(Relation r, Multipolygon polygon, boolean rolesWereChecked) { List openNodes = polygon.getOpenEnds(); if (!openNodes.isEmpty()) { errors.add(TestError.builder(this, Severity.WARNING, NON_CLOSED_WAY) @@ -321,56 +291,308 @@ .highlight(openNodes) .build()); } - - // For painting is used Polygon class which works with ints only. For validation we need more precision + Collection intersectionNodes = calcIntersectionAtNodes(r); List innerPolygons = polygon.getInnerPolygons(); List outerPolygons = polygon.getOuterPolygons(); - List innerPolygonsPaths = innerPolygons.isEmpty() ? Collections.emptyList() : createPolygons(innerPolygons); - List outerPolygonsPaths = createPolygons(outerPolygons); + Map> crossingPolyMap = findIntersectingWays(r, innerPolygons, outerPolygons); + + + // Polygons may intersect without crossing ways when one polygon lies completely inside the other + // or when they cross at shared nodes for (int i = 0; i < outerPolygons.size(); i++) { - PolyData pdOuter = outerPolygons.get(i); - // Check for intersection between outer members - for (int j = i+1; j < outerPolygons.size(); j++) { - checkCrossingWays(r, outerPolygons, outerPolygonsPaths, pdOuter, j); + PolyData outer1 = outerPolygons.get(i); + for (int j = i + 1; j < outerPolygons.size(); j++) { + PolyData outer2 = outerPolygons.get(j); + if (!checkProblemMap(crossingPolyMap, outer1, outer2)) { + checkSharedNodes(r, outer1, outer2, intersectionNodes); + } } } + + for (int i = 0; i < innerPolygons.size(); i++) { - PolyData pdInner = innerPolygons.get(i); - // Check for intersection between inner members + PolyData inner1 = innerPolygons.get(i); + if (!intersectionNodes.isEmpty()) { + for (PolyData outer: outerPolygons) { + checkSharedNodes(r, outer, inner1, intersectionNodes); + } + } + for (int j = i+1; j < innerPolygons.size(); j++) { - checkCrossingWays(r, innerPolygons, innerPolygonsPaths, pdInner, j); + PolyData inner2 = innerPolygons.get(j); + if (!checkProblemMap(crossingPolyMap, inner1, inner2)) { + checkSharedNodes(r, inner1, inner2, intersectionNodes); + } } - // Check for intersection between inner and outer members - boolean outside = true; - for (int o = 0; o < outerPolygons.size(); o++) { - outside &= checkCrossingWays(r, outerPolygons, outerPolygonsPaths, pdInner, o) == Intersection.OUTSIDE; + if (!rolesWereChecked) { + // Find inner polygons which are not inside any outer + boolean outside = true; + boolean crossingWithOuter = false; + EastNorth innerPoint = inner1.getNodes().get(0).getEastNorth(); + for (PolyData outer : outerPolygons) { + if (checkProblemMap(crossingPolyMap, inner1, outer)) { + crossingWithOuter = true; + break; + } + outside &= !outer.get().contains(innerPoint.getX(), innerPoint.getY()); + if (!outside) { + break; + } + } + if (outside && !crossingWithOuter) { + errors.add(TestError.builder(this, Severity.WARNING, INNER_WAY_OUTSIDE) + .message(tr("Multipolygon inner way is outside")) + .primitives(r) + .highlightNodePairs(Collections.singletonList(inner1.getNodes())) + .build()); + } } - if (outside) { - errors.add(TestError.builder(this, Severity.WARNING, INNER_WAY_OUTSIDE) - .message(tr("Multipolygon inner way is outside")) - .primitives(r) - .highlightNodePairs(Collections.singletonList(pdInner.getNodes())) - .build()); - } } } - private Intersection checkCrossingWays(Relation r, List polygons, List polygonsPaths, PolyData pd, int idx) { - Intersection intersection = getPolygonIntersection(polygonsPaths.get(idx), pd.getNodes()); - if (intersection == Intersection.CROSSING) { - PolyData pdOther = polygons.get(idx); - if (pdOther != null) { + /** + * Check two polygons for intersections at shared nodes. Only create an error if the intersection of the two + * polygons is neither equal to p1 nor to p2. + * @param r relation + * @param p1 1st polygon + * @param p2 2nd polygon + * @param intersectionNodes collection of all intersection nodes in the multipolygon relation + */ + private void checkSharedNodes(Relation r, PolyData p1, PolyData p2, Collection intersectionNodes) { + if (intersectionNodes.isEmpty()) + return; + + boolean p1IsInsideP2 = false; + boolean p1IsOutsideP2 = false; + Set sharedNodes = new HashSet<>(); + boolean foundInOutIntersection = false; + for (Node n : p1.getNodes()) { + if (intersectionNodes.contains(n) && p2.getNodes().contains(n)) { + sharedNodes.add(n); + } else { + if (checkIfNodeIsInsidePolygon(n, p2)) { + p1IsInsideP2 = true; + } else { + p1IsOutsideP2 = true; + } + } + } + + if (!sharedNodes.isEmpty()) { + if (p1IsOutsideP2) { + if (p1IsInsideP2) { + foundInOutIntersection = true; + } else { + // find out if p2 is fully contained in p1 or fully outside (only sharing nodes) + boolean isInsideP1 = false; + boolean isOutsideP1 = false; + for (Node n : p2.getNodes()) { + if (sharedNodes.contains(n)) + continue; + if (checkIfNodeIsInsidePolygon(n, p1)) { + isInsideP1 = true; + } else { + isOutsideP1 = true; + } + if (isInsideP1 && isOutsideP1) { + foundInOutIntersection = true; + break; + } + } + } + } + if (foundInOutIntersection) { errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS) .message(tr("Intersection between multipolygon ways")) .primitives(r) - .highlightNodePairs(Arrays.asList(pd.getNodes(), pdOther.getNodes())) + .highlight(sharedNodes) .build()); } } - return intersection; } /** + * Check if the node is inside the polygon according to the insideness rules of Shape. + * @param n the node + * @param p the polygon + * @return true if the node is inside the polygon + */ + private boolean checkIfNodeIsInsidePolygon(Node n, PolyData p) { + EastNorth en = n.getEastNorth(); + return (en != null && en.isValid() && p.get().contains(en.getX(), en.getY())); + } + + /** + * Determine multipolygon ways which are intersecting (crossing without a common node) or sharing one or more way segments. + * See also {@link CrossingWays} + * @param r the relation (for error reporting) + * @param innerPolygons list of inner polygons + * @param outerPolygons list of outer polygons + * @return map with crossing polygons + */ + private Map> findIntersectingWays(Relation r, List innerPolygons, + List outerPolygons) { + HashMap> crossingPolygonsMap = new HashMap<>(); + HashMap> sharedWaySegmentsPolygonsMap = new HashMap<>(); + + for (int loop = 0; loop < 2; loop++) { + /** All way segments, grouped by cells */ + final Map> cellSegments = new HashMap<>(1000); + /** The already detected ways in error */ + final Map, List> problemWays = new HashMap<>(50); + + Map> problemPolygonMap = (loop == 0) ? crossingPolygonsMap : sharedWaySegmentsPolygonsMap; + + for (Way w : r.getMemberPrimitives(Way.class)) { + findIntersectingWay(w, r, cellSegments, problemWays, loop == 1); + } + + if (!problemWays.isEmpty()) { + List allPolygons = new ArrayList<>(innerPolygons.size() + outerPolygons.size()); + allPolygons.addAll(innerPolygons); + allPolygons.addAll(outerPolygons); + + for (Entry, List> entry : problemWays.entrySet()) { + List ways = entry.getKey(); + if (ways.size() != 2) + continue; + PolyData[] crossingPolys = new PolyData[2]; + boolean allInner = true; + for (int i = 0; i < 2; i++) { + Way w = ways.get(i); + for (int j = 0; j < allPolygons.size(); j++) { + PolyData pd = allPolygons.get(j); + if (pd.getWayIds().contains(w.getUniqueId())) { + crossingPolys[i] = pd; + if (j >= innerPolygons.size()) + allInner = false; + break; + } + } + } + if (loop == 0 || (loop == 1 && !allInner)) { + String msg = (loop == 0) ? tr("Intersection between multipolygon ways") + : tr("Multipolygon ways share segments"); + errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS) + .message(msg) + .primitives(Arrays.asList(r, ways.get(0), ways.get(1))) + .highlightWaySegments(entry.getValue()) + .build()); + } + if (crossingPolys[0] != null && crossingPolys[1] != null) { + List crossingPolygons = problemPolygonMap.get(crossingPolys[0]); + if (crossingPolygons == null) { + crossingPolygons = new ArrayList<>(); + problemPolygonMap.put(crossingPolys[0], crossingPolygons); + } + crossingPolygons.add(crossingPolys[1]); + } + } + } + } + return crossingPolygonsMap; + } + + /** + * Find ways which are crossing without sharing a node. + * @param w way that is member of the relation + * @param r the relation (used for error messages) + * @param cellSegments map with already collected way segments + * @param crossingWays list to collect crossing ways + * @param findSharedWaySegments true: find shared way segments instead of crossings + */ + private void findIntersectingWay(Way w, Relation r, Map> cellSegments, + Map, List> crossingWays, boolean findSharedWaySegments) { + int nodesSize = w.getNodesCount(); + for (int i = 0; i < nodesSize - 1; i++) { + final WaySegment es1 = new WaySegment(w, i); + final EastNorth en1 = es1.getFirstNode().getEastNorth(); + final EastNorth en2 = es1.getSecondNode().getEastNorth(); + if (en1 == null || en2 == null) { + Main.warn("Crossing ways test skipped " + es1); + continue; + } + for (List segments : CrossingWays.getSegments(cellSegments, en1, en2)) { + for (WaySegment es2 : segments) { + + List highlight; + if (es2.way == w) + continue; // reported by CrossingWays.SelfIntersection + if (findSharedWaySegments && !es1.isSimilar(es2)) + continue; + if (!findSharedWaySegments && !es1.intersects(es2)) + continue; + + List prims = Arrays.asList(es1.way, es2.way); + if ((highlight = crossingWays.get(prims)) == null) { + highlight = new ArrayList<>(); + highlight.add(es1); + highlight.add(es2); + crossingWays.put(prims, highlight); + } else { + highlight.add(es1); + highlight.add(es2); + } + } + segments.add(es1); + } + } + } + + /** + * Detect intersections of multipolygon ways at nodes. If any way node is used by more than two ways + * or two times in one way and at least once in another way we found an intersection. + * @param r the relation + * @return List of nodes were ways intersect + */ + private Set calcIntersectionAtNodes(Relation r) { + Set intersectionNodes = new HashSet<>(); + Map> nodeMap = new HashMap<>(); + for (RelationMember rm : r.getMembers()) { + if (!rm.isWay()) + continue; + int numNodes = rm.getWay().getNodesCount(); + for (int i = 0; i < numNodes; i++) { + Node n = rm.getWay().getNode(i); + if (n.getReferrers().size() <= 1) { + continue; // cannot be a problem node + } + List ways = nodeMap.get(n); + if (ways == null) { + ways = new ArrayList<>(); + nodeMap.put(n, ways); + } + ways.add(rm.getWay()); + if (ways.size() > 2 || (ways.size() == 2 && i != 0 && i + 1 != numNodes)) { + intersectionNodes.add(n); + } + } + } + return intersectionNodes; + } + + /** + * Check if map contains combination of two given polygons. + * @param problemPolyMap the map + * @param pd1 1st polygon + * @param pd2 2nd polygon + * @return true if the combination of polygons is found in the map + */ + private boolean checkProblemMap(Map> problemPolyMap, PolyData pd1, PolyData pd2) { + List crossingWithFirst = problemPolyMap.get(pd1); + if (crossingWithFirst != null) { + if (crossingWithFirst.contains(pd2)) + return true; + } + List crossingWith2nd = problemPolyMap.get(pd2); + if (crossingWith2nd != null) { + if (crossingWith2nd.contains(pd1)) + return true; + } + return false; + } + + /** * Check for:
    *
  • {@link #WRONG_MEMBER_ROLE}: No useful role for multipolygon member
    • *
  • {@link #WRONG_MEMBER_TYPE}: Non-Way in multipolygon
    • Index: test/unit/org/openstreetmap/josm/data/validation/tests/MultipolygonTestTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/validation/tests/MultipolygonTestTest.java (revision 11179) +++ test/unit/org/openstreetmap/josm/data/validation/tests/MultipolygonTestTest.java (working copy) @@ -83,6 +83,6 @@ public void testMultipolygonFile() throws Exception { ValidatorTestUtils.testSampleFile("data_nodist/multipolygon.osm", ds -> ds.getRelations().stream().filter(Relation::isMultipolygon).collect(Collectors.toList()), - name -> name.startsWith("06") || name.startsWith("07"), MULTIPOLYGON_TEST, RELATION_TEST); + name -> name.startsWith("06") || name.startsWith("07") || name.startsWith("08"), MULTIPOLYGON_TEST, RELATION_TEST); } }