Ticket #13307: improve_MultipolygonTest_v12.patch

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

@@ -5 +5 @@
 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;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 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.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.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;
@@ -66 +71 @@
     public static final int NO_STYLE_POLYGON = 1611;
     /** Area style on outer way */
     public static final int OUTER_STYLE = 1613;
+    /** Multipolygon member repeated (same primitive, same role */
+    public static final int REPEATED_MEMBER_SAME_ROLE = 1614;
+    /** Multipolygon member repeated (same primitive, different role) */
+    public static final int REPEATED_MEMBER_DIFF_ROLE = 1615;
 
     private static volatile ElemStyles styles;
 
@@ -102 +111 @@
         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());
-        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<GeneralPath> createPolygons(List<Multipolygon.PolyData> joinedWays) {
-        List<GeneralPath> result = new ArrayList<>();
-        for (Multipolygon.PolyData way : joinedWays) {
-            result.add(createPath(way.getNodes()));
-        }
-        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)) {
@@ -159 +134 @@
         if (r.isMultipolygon()) {
             checkMembersAndRoles(r);
             checkOuterWay(r);
-
-            // Rest of checks is only for complete multipolygons
-            if (!r.hasIncompleteMembers()) {
-                Multipolygon polygon = MultipolygonCache.getInstance().get(Main.map.mapView, 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) {
+                List<Node> intersectionNodes = checkIntersectionAtNodes(r);
+                // 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, intersectionNodes, rolesWereChecked);
+                }
             }
         }
     }
@@ -195 +171 @@
     }
 
     /**
-     * Create new multipolygon using the logics from CreateMultipolygonAction and see if roles match:<ul>
+     * If simple joining of ways doesn't work, create new multipolygon using the logics from
+     * CreateMultipolygonAction and see if roles match:<ul>
      * <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()) {
                 final Collection<RelationMember> memberInNewMP = newMP.b.getMembersFor(Collections.singleton(member.getMember()));
@@ -219 +198 @@
                 }
             }
         }
+        return newMP != null;
     }
 
     /**
@@ -303 +283 @@
      * </ul>
      * @param r relation
      * @param polygon multipolygon
+     * @param intersectionNodes known nodes where ways of this multipolygon intersect or touch
+     * @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, List<Node> intersectionNodes, boolean rolesWereChecked) {
         List<Node> openNodes = polygon.getOpenEnds();
         if (!openNodes.isEmpty()) {
             errors.add(TestError.builder(this, Severity.WARNING, NON_CLOSED_WAY)
@@ -314 +296 @@
                     .build());
         }
 
-        // 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);
+        HashMap<PolyData, List<PolyData>> crossingPolyMap = checkCrossingWays(r, innerPolygons, outerPolygons);
+
+        //Polygons may intersect without crossing ways when one polygon lies completely inside the other
         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);
+            // report outer polygons which lie inside another outer
+            PolyData outer1 = outerPolygons.get(i);
+            for (int j = 0; j < outerPolygons.size(); j++) {
+                if (i != j && !crossing(crossingPolyMap, outer1, outerPolygons.get(j))) {
+                    EastNorth en1 = null;
+                    // find node which is not an intersection of multipolygon ways
+                    for (int k = 0; k < outerPolygons.get(j).getNodes().size(); k++) {
+                        Node n = outerPolygons.get(j).getNodes().get(k);
+                        if (intersectionNodes.contains(n) == false)
+                            en1 = n.getEastNorth();
+                    }
+                    if (en1 != null && outer1.get().contains(en1.getX(), en1.getY())) {
+                        ArrayList<OsmPrimitive> hilite = new ArrayList<>();
+                        hilite.addAll(outer1.getNodes());
+                        hilite.addAll(outerPolygons.get(j).getNodes());
+                        errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS)
+                                .message(tr("Outer inside outer"))
+                                .primitives(r)
+                                .highlight(hilite)
+                                .build());
+
+                    }
+                }
             }
         }
-        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 (PolyData inner1 : innerPolygons) {
+            for (PolyData inner2 : innerPolygons) {
+                if (inner1 != inner2 && !crossing(crossingPolyMap, inner1, inner2)) {
+                    // we must allow that inner polygons share some nodes
+                    boolean allInside = true;
+                    for (Node innerPoint : inner2.getNodes()) {
+                        EastNorth en = innerPoint.getEastNorth();
+                        if (!inner1.get().contains(en.getX(), en.getY())) {
+                            allInside = false;
+                            break;
+                        }
+                    }
+                    if (allInside) {
+                        ArrayList<OsmPrimitive> hilite = new ArrayList<>();
+                        hilite.addAll(inner1.getNodes());
+                        hilite.addAll(inner2.getNodes());
+                        errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS)
+                                .message(tr("Inner inside inner"))
+                                .primitives(r)
+                                .highlight(hilite)
+                                .build());
+                    }
+                }
             }
-            // 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 (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());
-            }
+            // Find inner polygons which are not inside any outer
+            // MAYBE enable the following line to reduce number of warnings for same problem (and adapt multipolygon.osm)
+//            if (!rolesWereChecked) {
+                boolean outside = true;
+                boolean crossingWithOuter = false;
+                EastNorth innerPoint = inner1.getNodes().get(0).getEastNorth();
+                for (PolyData outer : outerPolygons) {
+                    if (crossing(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());
+                }
+//            }
         }
     }
 
-    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) {
-                errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS)
-                        .message(tr("Intersection between multipolygon ways"))
-                        .primitives(r)
-                        .highlightNodePairs(Arrays.asList(pd.getNodes(), pdOther.getNodes()))
-                        .build());
-            }
+    /**
+     * Check if crossing map contains combination of two given polygons.
+     * @param crossingPolyMap the map
+     * @param pd1 1st polygon
+     * @param pd2 2nd polygon
+     * @return true if the polygons are crossing without sharing a node
+     */
+    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;
     }
 
     /**
@@ -412 +448 @@
         }
     }
 
+    /**
+     * Determine multipolygon ways which are intersecting (crossing without a common node). This is not 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) {
+        /** All way segments, grouped by cells */
+        final Map<Point2D, List<WaySegment>> cellSegments = new HashMap<>(1000);
+        /** The already detected ways in error */
+        final Map<List<Way>, List<WaySegment>> crossingWays = new HashMap<>(50);
+
+        for (Way w : r.getMemberPrimitives(Way.class)) {
+            checkCrossingWay(w, r, cellSegments, crossingWays);
+        }
+        HashMap<PolyData, List<PolyData>> crossingPolygonsMap = new HashMap<>();
+        if (!crossingWays.isEmpty()) {
+            List<PolyData> allPolygons = new ArrayList<>(innerPolygons.size() + outerPolygons.size());
+            allPolygons.addAll(innerPolygons);
+            allPolygons.addAll(outerPolygons);
+
+            for (Entry<List<Way>, List<WaySegment>> entry : crossingWays.entrySet()) {
+                List<Way> ways = entry.getKey();
+                if (ways.size() != 2)
+                    continue;
+                PolyData[] crossingPolys = new PolyData[2];
+                for (int i = 0; i < 2; i++) {
+                    Way w = ways.get(i);
+                    for (PolyData pd : allPolygons) {
+                        if (pd.getWayIds().contains(w.getUniqueId())) {
+                            crossingPolys[i] = pd;
+                            break;
+                        }
+                    }
+                }
+                if (crossingPolys[0] != null && crossingPolys[1] != null) {
+                    List<PolyData> crossingPolygons = crossingPolygonsMap.get(crossingPolys[0]);
+                    if (crossingPolygons == null) {
+                        crossingPolygons = new ArrayList<>();
+                        crossingPolygonsMap.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 crossingWays
+     * @param cellSegments
+     */
+    private void checkCrossingWay(Way w, Relation r, Map<Point2D, List<WaySegment>> cellSegments, Map<List<Way>, List<WaySegment>> crossingWays) {
+        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 : CrossingWays.getSegments(cellSegments, en1, en2)) {
+                for (WaySegment es2 : segments) {
+
+                    List<WaySegment> highlight;
+                    if (es2.way == w)
+                        continue; // reported by CrossingWays.SelfIntersection
+                    if (!es1.intersects(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);
+                        errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS)
+                                .message(tr("Intersection between multipolygon ways"))
+                                .primitives(Arrays.asList(r, es1.way, es2.way))
+                                .highlightWaySegments(highlight)
+                                .build());
+                        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 List<Node> checkIntersectionAtNodes(Relation r) {
+        List<Node> intersectionNodes = new ArrayList<>();
+        List<Pair<Set<Way>,List<Node>>> intersectionErrors = new ArrayList<>();
+        HashMap<Way, String> wayRoleMap = new HashMap<>();
+        for (RelationMember rm : r.getMembers()) {
+            if (rm.isWay())
+                wayRoleMap.put(rm.getWay(), rm.getRole());
+        }
+        Map<Node, List<Way>> 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<Way> 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);
+                    boolean allInner = true;
+                    for (Way w : ways) {
+                        String role = wayRoleMap.get(w);
+                        if (!"inner".equals(role))
+                            allInner = false;
+                    }
+                    if (!allInner) {
+                        Set<Way> errorWays = new HashSet<>(ways);
+                        boolean addNew = true;
+                        for (Pair<Set<Way>, List<Node>> pair : intersectionErrors) {
+                            if (pair.a.size() == errorWays.size() && pair.a.containsAll(errorWays)) {
+                                pair.b.add(n);
+                                addNew = false;
+                                break;
+                            }
+                        }
+                        if (addNew) {
+                            List<Node> errNodes = new ArrayList<>();
+                            errNodes.add(n);
+                            intersectionErrors.add(new Pair<>(errorWays, errNodes));
+                        }
+                    }
+                }
+            }
+        }
+        for (Pair<Set<Way>,List<Node>> pair : intersectionErrors) {
+            // A single shared node between two ways ("touching ways") is considered okay.
+            if (pair.b.size() > 1) {
+                errors.add(TestError.builder(this, Severity.WARNING, CROSSING_WAYS)
+                        .message(tr("Multipolygon ways share node(s)"))
+                        .primitives(pair.a)
+                        .highlight(pair.b)
+                        .build());
+            }
+        }
+        return intersectionNodes;
+    }
+
+    /**
+     * Check for:<ul>
+     * <li>{@link #REPEATED_MEMBER_DIFF_ROLE}: Multipolygon member(s) repeated with different role</li>
+     * <li>{@link #REPEATED_MEMBER_SAME_ROLE}: Multipolygon member(s) repeated with same role</li>
+     * </ul>
+     * @param r relation
+     * @return
+     */
+    private boolean checkRepeatedWayMembers(Relation r) {
+        boolean hasDups = false;
+        Map<OsmPrimitive, List<RelationMember>> seenMemberPrimitives = new HashMap<>();
+        for (RelationMember rm : r.getMembers()) {
+            List<RelationMember> list = seenMemberPrimitives.get(rm.getMember());
+            if (list == null) {
+                list = new ArrayList<>(2);
+                seenMemberPrimitives.put(rm.getMember(), list);
+            } else {
+                hasDups = true;
+            }
+            list.add(rm);
+        }
+        if (hasDups) {
+            List<OsmPrimitive> repeatedSameRole = new ArrayList<>();
+            List<OsmPrimitive> repeatedDiffRole = new ArrayList<>();
+            for (Entry<OsmPrimitive, List<RelationMember>> e : seenMemberPrimitives.entrySet()) {
+                List<RelationMember> visited = e.getValue();
+                if (e.getValue().size() == 1)
+                    continue;
+                // we found a duplicate member, check if the roles differ
+                boolean rolesDiffer = false;
+                RelationMember rm = visited.get(0);
+                List<OsmPrimitive> primitives = new ArrayList<>();
+                for (int i = 1; i < visited.size(); i++) {
+                    RelationMember v = visited.get(i);
+                    primitives.add(rm.getMember());
+                    if (v.getRole().equals(rm.getRole()) == false) {
+                        rolesDiffer = true;
+                    }
+                }
+                if (rolesDiffer)
+                    repeatedDiffRole.addAll(primitives);
+                else
+                    repeatedSameRole.addAll(primitives);
+            }
+            addRepeatedMemberError(r, repeatedDiffRole, REPEATED_MEMBER_DIFF_ROLE, tr("Multipolygon member(s) repeated with different role"));
+            addRepeatedMemberError(r, repeatedSameRole, REPEATED_MEMBER_SAME_ROLE, tr("Multipolygon member(s) repeated with same role"));
+        }
+        return hasDups;
+    }
+
+    private void addRepeatedMemberError(Relation r, List<OsmPrimitive> repeatedMembers, int errorCode, String msg) {
+        if (!repeatedMembers.isEmpty()) {
+            List<OsmPrimitive> prims = new ArrayList<>(1 + repeatedMembers.size());
+            prims.add(r);
+            prims.addAll(repeatedMembers);
+            errors.add(TestError.builder(this, Severity.WARNING, errorCode)
+                    .message(msg)
+                    .primitives(prims)
+                    .highlight(repeatedMembers)
+                    .build());
+        }
+    }
+
+    @Override
+    public Command fixError(TestError testError) {
+        if (testError.getCode() == REPEATED_MEMBER_SAME_ROLE) {
+            ArrayList<OsmPrimitive> primitives = new ArrayList<>(testError.getPrimitives());
+            if (primitives.size() >= 2) {
+                if (primitives.get(0) instanceof Relation) {
+                    Relation oldRel = (Relation) primitives.get(0);
+                    Relation newRel = new Relation(oldRel);
+                    List<OsmPrimitive> repeatedPrims = primitives.subList(1, primitives.size());
+                    List<RelationMember> oldMembers = oldRel.getMembers();
+
+                    List<RelationMember> newMembers = new ArrayList<>();
+                    HashSet<OsmPrimitive> toRemove = new HashSet<>(repeatedPrims);
+                    HashSet<OsmPrimitive> found = new HashSet<>(repeatedPrims.size());
+                    for (RelationMember rm : oldMembers) {
+                        if (toRemove.contains(rm.getMember())) {
+                            if (found.contains(rm.getMember()) == false) {
+                                found.add(rm.getMember());
+                                newMembers.add(rm);
+                            }
+                        } else
+                            newMembers.add(rm);
+                    }
+                    newRel.setMembers(newMembers);
+                    return new ChangeCommand (oldRel, newRel);
+                }
+            }
+        }
+        return null;
+    }
+
+    @Override
+    public boolean isFixable(TestError testError) {
+        if (testError.getCode() == REPEATED_MEMBER_SAME_ROLE)
+            return true;
+        return false;
+    }
 }