Ticket #13307: improve_MultipolygonTest_v5.patch

src/org/openstreetmap/josm/data/osm/WaySegment.java

@@ -114 +114 @@
                 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
+     * @since r10819
+     */
+    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 + ']';

src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java

@@ -4 +4 @@
 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.Path2D;
+import java.awt.geom.Point2D;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 
 import org.openstreetmap.josm.Main;
 import org.openstreetmap.josm.actions.CreateMultipolygonAction;
+import org.openstreetmap.josm.data.coor.EastNorth;
+import org.openstreetmap.josm.data.coor.LatLon;
 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.osm.visitor.paint.relations.MultipolygonCache;
 import org.openstreetmap.josm.data.validation.OsmValidator;
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.data.validation.util.ValUtil;
 import org.openstreetmap.josm.gui.DefaultNameFormatter;
 import org.openstreetmap.josm.gui.mappaint.ElemStyles;
 import org.openstreetmap.josm.gui.mappaint.MapPaintStyles;
@@ -71 +78 @@
     private static volatile ElemStyles styles;
 
     private final Set<String> keysCheckedByAnotherTest = new HashSet<>();
+    /** All way segments, grouped by cells */
+    private final Map<Point2D, List<WaySegment>> cellSegments = new HashMap<>(1000);
+    /** The already detected ways in error */
+    private final Map<List<Way>, List<WaySegment>> crossingWays = new HashMap<>(50);
 
     /**
      * Constructs a new {@code MultipolygonTest}.
@@ -103 +114 @@
         super.endTest();
     }
 
-    private static GeneralPath createPath(List<Node> nodes) {
-        GeneralPath result = new GeneralPath();
-        result.moveTo((float) nodes.get(0).getCoor().lat(), (float) nodes.get(0).getCoor().lon());
+    private static Path2D.Double createPath(List<Node> nodes) {
+        Path2D.Double result = new Path2D.Double();
+        result.moveTo(nodes.get(0).getCoor().lon(), nodes.get(0).getCoor().lat());
         for (int i = 1; i < nodes.size(); i++) {
             Node n = nodes.get(i);
-            result.lineTo((float) n.getCoor().lat(), (float) n.getCoor().lon());
+            result.lineTo(n.getCoor().lon(), n.getCoor().lat());
         }
         return result;
     }
 
-    private static List<GeneralPath> createPolygons(List<Multipolygon.PolyData> joinedWays) {
-        List<GeneralPath> result = new ArrayList<>();
+    private static List<Path2D.Double> createPolygons(List<Multipolygon.PolyData> joinedWays) {
+        List<Path2D.Double> result = new ArrayList<>();
         for (Multipolygon.PolyData way : joinedWays) {
             result.add(createPath(way.getNodes()));
         }
@@ -121 +132 @@
         return result;
     }
 
-    private static Intersection getPolygonIntersection(GeneralPath outer, List<Node> 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)) {
@@ -155 +150 @@
     @Override
     public void visit(Relation r) {
         if (r.isMultipolygon()) {
+            crossingWays.clear();
+            cellSegments.clear();
+
             checkMembersAndRoles(r);
             checkOuterWay(r);
 
@@ -163 +161 @@
                 Multipolygon polygon = MultipolygonCache.getInstance().get(Main.map.mapView, r);
 
                 // Create new multipolygon using the logics from CreateMultipolygonAction and see if roles match.
-                checkMemberRoleCorrectness(r);
+                boolean rolesWereChecked = checkMemberRoleCorrectness(r);
                 checkStyleConsistency(r, polygon);
-                checkGeometry(r, polygon);
+                checkGeometry(r, polygon, rolesWereChecked);
             }
         }
     }
@@ -194 +192 @@
      * <li>{@link #WRONG_MEMBER_ROLE}: Role for ''{0}'' should be ''{1}''</li>
      * </ul>
      * @param r relation
+     * @return true if member roles were checked
      */
-    private void checkMemberRoleCorrectness(Relation r) {
+    private boolean checkMemberRoleCorrectness(Relation r) {
         final Pair<Relation, Relation> newMP = CreateMultipolygonAction.createMultipolygonRelation(r.getMemberPrimitives(Way.class), false);
         if (newMP != null) {
             for (RelationMember member : r.getMembers()) {
@@ -216 +215 @@
                 }
             }
         }
+        return newMP != null;
     }
 
     /**
@@ -285 +285 @@
         }
     }
 
+    private static class LatLonPolyData {
+        final PolyData pd;
+        final Path2D.Double latLonPath;
+
+        LatLonPolyData(PolyData polyData, Path2D.Double path) {
+            this.pd = polyData;
+            this.latLonPath = path;
+        }
+    }
+
     /**
      * Various geometry-related checks:<ul>
      * <li>{@link #NON_CLOSED_WAY}: Multipolygon is not closed</li>
@@ -293 +303 @@
      * </ul>
      * @param r relation
      * @param polygon multipolygon
+     * @param rolesWereChecked might be used to skip most of the tests below
      */
-    private void checkGeometry(Relation r, Multipolygon polygon) {
+    private void checkGeometry(Relation r, Multipolygon polygon, boolean rolesWereChecked) {
         List<Node> openNodes = polygon.getOpenEnds();
         if (!openNodes.isEmpty()) {
             List<OsmPrimitive> primitives = new LinkedList<>();
             primitives.add(r);
             primitives.addAll(openNodes);
-            addError(r, new TestError(this, Severity.WARNING, tr("Multipolygon is not closed"), NON_CLOSED_WAY, primitives, openNodes));
+            addError(r, new TestError(this, Severity.WARNING, tr("Multipolygon is not closed"), NON_CLOSED_WAY,
+                    primitives, openNodes));
         }
+        List<PolyData> innerPolygons = polygon.getInnerPolygons();
+        List<PolyData> outerPolygons = polygon.getOuterPolygons();
 
+        HashMap<PolyData, List<PolyData>> crossingPolyMap = checkCrossingWays(r, innerPolygons, outerPolygons);
+
+        //Polygons may intersect without crossing ways when one polygon lies completely inside the other
+        List<LatLonPolyData> inner = calcLatLonPolygons(innerPolygons);
+        List<LatLonPolyData> outer = calcLatLonPolygons(outerPolygons);
         // For painting is used Polygon class which works with ints only. For validation we need more precision
-        List<PolyData> innerPolygons = polygon.getInnerPolygons();
-        List<PolyData> outerPolygons = polygon.getOuterPolygons();
-        List<GeneralPath> innerPolygonsPaths = innerPolygons.isEmpty() ? Collections.<GeneralPath>emptyList() : createPolygons(innerPolygons);
-        List<GeneralPath> outerPolygonsPaths = createPolygons(outerPolygons);
-        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);
+        for (int i = 0; i + 1 < outer.size(); i++) {
+            // report outer polygons which lie inside another outer
+            LatLonPolyData outer1 = outer.get(i);
+            for (int j = 0; j < outer.size(); j++) {
+                if (i != j && !crossing(crossingPolyMap, outer1.pd, outer.get(j).pd)) {
+                    LatLon c = outer.get(j).pd.getNodes().get(0).getCoor();
+                    if (outer1.latLonPath.contains(c.lon(), c.lat())) {
+                        addError(r,
+                                new TestError(this, Severity.WARNING, tr("Intersection between multipolygon ways"),
+                                        CROSSING_WAYS, Collections.singletonList(r),
+                                        Arrays.asList(outer1.pd.getNodes(), outer.get(j).pd.getNodes())));
+                    }
+                }
             }
         }
-        for (int i = 0; i < innerPolygons.size(); i++) {
-            PolyData pdInner = innerPolygons.get(i);
-            // Check for intersection between inner members
-            for (int j = i+1; j < innerPolygons.size(); j++) {
-                checkCrossingWays(r, innerPolygons, innerPolygonsPaths, pdInner, j);
+        for (int i = 0; i < inner.size(); i++) {
+            LatLonPolyData inner1 = inner.get(i);
+            LatLon innerPoint = inner1.pd.getNodes().get(0).getCoor();
+            for (int j = 0; j < inner.size(); j++) {
+                LatLonPolyData inner2 = inner.get(j);
+                if (i != j && !crossing(crossingPolyMap, inner1.pd, inner2.pd)) {
+                    if (inner.get(j).latLonPath.contains(innerPoint.lon(), innerPoint.lat())) {
+                        addError(r,
+                                new TestError(this, Severity.WARNING, tr("Intersection between multipolygon ways"),
+                                        CROSSING_WAYS, Collections.singletonList(r),
+                                        Arrays.asList(inner1.pd.getNodes(), inner2.pd.getNodes())));
+                    }
+                }
             }
-            // Check for intersection between inner and outer members
+
+            // Find inner polygons which are not inside any outer
             boolean outside = true;
-            for (int o = 0; o < outerPolygons.size(); o++) {
-                outside &= checkCrossingWays(r, outerPolygons, outerPolygonsPaths, pdInner, o) == Intersection.OUTSIDE;
+            boolean crossingWithOuter = false;
+
+            for (int o = 0; o < outer.size(); o++) {
+                if (crossing(crossingPolyMap, inner1.pd, outer.get(o).pd)) {
+                    crossingWithOuter = true;
+                    break;
+                }
+                outside &= outer.get(o).latLonPath.contains(innerPoint.lon(), innerPoint.lat()) == false;
+                if (!outside)
+                    break;
             }
-            if (outside) {
+            if (outside && !crossingWithOuter) {
                 addError(r, new TestError(this, Severity.WARNING, tr("Multipolygon inner way is outside"),
-                        INNER_WAY_OUTSIDE, Collections.singletonList(r), Arrays.asList(pdInner.getNodes())));
+                        INNER_WAY_OUTSIDE, Collections.singletonList(r), Arrays.asList(inner1.pd.getNodes())));
             }
         }
     }
 
-    private Intersection checkCrossingWays(Relation r, List<PolyData> polygons, List<GeneralPath> 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) {
-                addError(r, new TestError(this, Severity.WARNING, tr("Intersection between multipolygon ways"),
-                        CROSSING_WAYS, Collections.singletonList(r), Arrays.asList(pd.getNodes(), pdOther.getNodes())));
-            }
+    private boolean crossing(HashMap<PolyData, List<PolyData>> crossingPolyMap, PolyData pd1, PolyData pd2) {
+        List<PolyData> crossingWithFirst = crossingPolyMap.get(pd1);
+        if (crossingWithFirst != null) {
+            if (crossingWithFirst.contains(pd2))
+                return true;
         }
-        return intersection;
+        List<PolyData> crossingWith2nd = crossingPolyMap.get(pd2);
+        if (crossingWith2nd != null) {
+            if (crossingWith2nd.contains(pd1))
+                return true;
+        }
+        return false;
     }
 
+    private List<LatLonPolyData> calcLatLonPolygons(List<PolyData> polygons) {
+        if (polygons == null || polygons.isEmpty())
+            return Collections.emptyList();
+        List<LatLonPolyData> latLonPolygons = new ArrayList<>();
+        List<Path2D.Double> polygonsPaths = createPolygons(polygons);
+        for (int i = 0; i < polygons.size(); i++) {
+            latLonPolygons.add(new LatLonPolyData(polygons.get(i), polygonsPaths.get(i)));
+        }
+        return latLonPolygons;
+    }
+
     /**
      * Check for:<ul>
      * <li>{@link #WRONG_MEMBER_ROLE}: No useful role for multipolygon member</li>
@@ -389 +442 @@
         addRelationIfNeeded(error, r);
         errors.add(error);
     }
+
+    /**
+     * Determine multipolygon ways which are intersecting. This is now allowed.
+     * See {@link CrossingWays}
+     * @param r the relation (for error reporting)
+     * @param innerPolygons list of inner polygons
+     * @param outerPolygons list of outer polygons
+     * @return map of crossing polygons (including polygons touching outer)
+     */
+    private HashMap<PolyData, List<PolyData>> checkCrossingWays(Relation r, List<PolyData> innerPolygons,
+            List<PolyData> outerPolygons) {
+        List<Way> innerWays = new ArrayList<>();
+        List<Way> outerWays = new ArrayList<>();
+        for (Way w : r.getMemberPrimitives(Way.class)) {
+            for (PolyData pd : innerPolygons) {
+                if (pd.getWayIds().contains(w.getUniqueId())) {
+                    innerWays.add(w);
+                    break;
+                }
+            }
+            for (PolyData pd : outerPolygons) {
+                if (pd.getWayIds().contains(w.getUniqueId())) {
+                    outerWays.add(w);
+                    break;
+                }
+            }
+        }
+        for (Way w : innerWays) {
+            checkCrossingWay(w, r, true /* allow shared ways */);
+        }
+        for (Way w : outerWays) {
+            checkCrossingWay(w, r, false/* don't allow shared ways */);
+        }
+        HashMap<PolyData, List<PolyData>> crossingPolygonsMap = new HashMap<>();
+        for (Entry<List<Way>, List<WaySegment>> entry : crossingWays.entrySet()) {
+            List<Way> ways = entry.getKey();
+            PolyData[] crossingPolys = new PolyData[2];
+            for (Way w : ways) {
+                for (int j = 0; j < crossingPolys.length; j++) {
+                    for (PolyData pd : innerPolygons) {
+                        if (pd.getWayIds().contains(w.getUniqueId())) {
+                            crossingPolys[j] = pd;
+                            break;
+                        }
+                    }
+                    if (crossingPolys[j] != null)
+                        break;
+                    for (PolyData pd : outerPolygons) {
+                        if (pd.getWayIds().contains(w.getUniqueId())) {
+                            crossingPolys[j] = pd;
+                            break;
+                        }
+                    }
+                }
+            }
+            if (crossingPolys[0] != null && crossingPolys[1] != null) {
+                List<PolyData> x = crossingPolygonsMap.get(crossingPolys[0]);
+                if (x == null) {
+                    x = new ArrayList<>();
+                    crossingPolygonsMap.put(crossingPolys[0], x);
+                }
+                x.add(crossingPolys[1]);
+            }
+        }
+        return crossingPolygonsMap;
+    }
+
+    /**
+     *
+     * @param w way that is member of the relation
+     * @param r the relation (used for error messages)
+     * @param allowSharedWaySegment false: treat similar way segment as crossing
+     */
+    private void checkCrossingWay(Way w, Relation r, boolean allowSharedWaySegment) {
+
+        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<WaySegment> segments : getSegments(en1, en2)) {
+                for (WaySegment es2 : segments) {
+
+                    List<WaySegment> highlight;
+
+                    if (!es1.intersects(es2)) {
+                        if (allowSharedWaySegment || !es1.isSimilar(es2))
+                            continue;
+                    }
+
+                    List<Way> prims = Arrays.asList(es1.way, es2.way);
+                    if ((highlight = crossingWays.get(prims)) == null) {
+                        highlight = new ArrayList<>();
+                        highlight.add(es1);
+                        highlight.add(es2);
+
+                        addError(r, new TestError(this, Severity.WARNING, tr("Intersection between multipolygon ways"),
+                                CROSSING_WAYS, Arrays.asList(r, es1.way, es2.way), highlight));
+                        crossingWays.put(prims, highlight);
+                    } else {
+                        highlight.add(es1);
+                        highlight.add(es2);
+                    }
+                }
+                segments.add(es1);
+            }
+        }
+    }
+
+    /**
+     * Returns all the cells this segment crosses.  Each cell contains the list
+     * of segments already processed
+     *
+     * @param n1 The first EastNorth
+     * @param n2 The second EastNorth
+     * @return A list with all the cells the segment crosses
+     */
+    private List<List<WaySegment>> getSegments(EastNorth n1, EastNorth n2) {
+
+        List<List<WaySegment>> cells = new ArrayList<>();
+        for (Point2D cell : ValUtil.getSegmentCells(n1, n2, OsmValidator.griddetail)) {
+            List<WaySegment> segments = cellSegments.get(cell);
+            if (segments == null) {
+                segments = new ArrayList<>();
+                cellSegments.put(cell, segments);
+            }
+            cells.add(segments);
+        }
+        return cells;
+    }
 }