Index: trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (revision 15959) @@ -428,3 +428,47 @@ return cells; } + + /** + * Find ways which are crossing without sharing a node. + * @param w way that is to be checked + * @param cellSegments map with already collected way segments + * @param crossingWays map to collect crossing ways and related segments + * @param findSharedWaySegments true: find shared way segments instead of crossings + */ + public static void findIntersectingWay(Way w, 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) { + Logging.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 // reported by CrossingWays.SelfIntersection + || (findSharedWaySegments && !es1.isSimilar(es2)) + || (!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); + } + } + } + } Index: trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 15959) @@ -44,4 +44,5 @@ import org.openstreetmap.josm.data.osm.Tag; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.WaySegment; import org.openstreetmap.josm.data.preferences.sources.SourceEntry; import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; @@ -93,6 +94,7 @@ */ public class MapCSSTagChecker extends Test.TagTest { - MapCSSTagCheckerIndex indexData; - final Set tested = new HashSet<>(); + private MapCSSTagCheckerIndex indexData; + private final Set tested = new HashSet<>(); + final Map mpAreaCache = new HashMap<>(); /** @@ -202,5 +204,5 @@ @Override public Command createCommand(OsmPrimitive p, Selector matchingSelector) { - final Tag tag = Tag.ofString(evaluateObject(obj, p, matchingSelector)); + final Tag tag = Tag.ofString(FixCommand.evaluateObject(obj, p, matchingSelector)); return new ChangePropertyCommand(p, tag.getKey(), tag.getValue()); } @@ -223,5 +225,5 @@ @Override public Command createCommand(OsmPrimitive p, Selector matchingSelector) { - final String key = evaluateObject(obj, p, matchingSelector); + final String key = FixCommand.evaluateObject(obj, p, matchingSelector); return new ChangePropertyCommand(p, key, ""); } @@ -620,8 +622,21 @@ errorBuilder = errorBuilder.fix(() -> fix); } + // check if we have special information about highlighted objects */ + boolean hiliteFound = false; if (env.intersections != null) { Area is = env.intersections.get(c); if (is != null) { errorBuilder = errorBuilder.highlight(is); + hiliteFound = true; + } + } + if (env.crossingWaysMap != null && !hiliteFound) { + Map, List> is = env.crossingWaysMap.get(c); + if (is != null) { + Set toHilite = new HashSet<>(); + for (List wsList : is.values()) { + toHilite.addAll(wsList); + } + errorBuilder = errorBuilder.highlightWaySegments(toHilite); } } @@ -709,4 +724,6 @@ Environment env = new Environment(p, new MultiCascade(), Environment.DEFAULT_LAYER, null); + env.mpAreaCache = mpAreaCache; + // the declaration indices are sorted, so it suffices to save the last used index Declaration lastDeclUsed = null; @@ -775,8 +792,10 @@ } - private static Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity, + private Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity, Collection> checksCol) { + // this variant is only used by the assertion tests final List r = new ArrayList<>(); final Environment env = new Environment(p, new MultiCascade(), Environment.DEFAULT_LAYER, null); + env.mpAreaCache = mpAreaCache; for (Set schecks : checksCol) { for (TagCheck check : schecks) { @@ -992,4 +1011,5 @@ } tested.clear(); + mpAreaCache.clear(); } @@ -1023,7 +1043,9 @@ tested.clear(); } - super.endTest(); // no need to keep the index, it is quickly build and doubles the memory needs indexData = null; + // always clear the cache to make sure that we catch changes in geometry + mpAreaCache.clear(); + super.endTest(); } } Index: trunk/src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java (revision 15959) @@ -37,5 +37,4 @@ import org.openstreetmap.josm.tools.Geometry; import org.openstreetmap.josm.tools.Geometry.PolygonIntersection; -import org.openstreetmap.josm.tools.Logging; /** @@ -626,51 +625,8 @@ for (Way w: r.getMemberPrimitives(Way.class)) { if (!w.hasIncompleteNodes()) { - findIntersectingWay(w, cellSegments, crossingWays, findSharedWaySegments); + CrossingWays.findIntersectingWay(w, cellSegments, crossingWays, findSharedWaySegments); } } return crossingWays; - } - - /** - * Find ways which are crossing without sharing a node. - * @param w way that is member of the relation - * @param cellSegments map with already collected way segments - * @param crossingWays map to collect crossing ways and related segments - * @param findSharedWaySegments true: find shared way segments instead of crossings - */ - private static void findIntersectingWay(Way w, 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) { - Logging.warn("Crossing ways test (MP) skipped " + es1); - continue; - } - for (List segments : CrossingWays.getSegments(cellSegments, en1, en2)) { - for (WaySegment es2 : segments) { - - List highlight; - if (es2.way == w // reported by CrossingWays.SelfIntersection - || (findSharedWaySegments && !es1.isSimilar(es2)) - || (!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); - } - } } Index: trunk/src/org/openstreetmap/josm/gui/mappaint/Environment.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/mappaint/Environment.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/gui/mappaint/Environment.java (revision 15959) @@ -3,6 +3,6 @@ import java.awt.geom.Area; -import java.util.HashMap; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; import java.util.Set; @@ -10,4 +10,6 @@ import org.openstreetmap.josm.data.osm.IPrimitive; import org.openstreetmap.josm.data.osm.Relation; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.WaySegment; import org.openstreetmap.josm.gui.mappaint.mapcss.Condition.Context; import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.LinkSelector; @@ -72,7 +74,17 @@ /** + * Crossing ways result from CrossingFinder, filled for incomplete ways/relations + */ + public Map, List>> crossingWaysMap; + + /** * Intersection areas (only filled with CrossingFinder if children is not null) */ public Map intersections; + + /** + * Cache for multipolygon areas, can be null, used with CrossingFinder + */ + public Map mpAreaCache; /** @@ -126,5 +138,7 @@ this.context = other.getContext(); this.children = other.children == null ? null : new LinkedHashSet<>(other.children); - this.intersections = other.intersections == null ? null : new HashMap<>(other.intersections); + this.intersections = other.intersections; + this.crossingWaysMap = other.crossingWaysMap; + this.mpAreaCache = other.mpAreaCache; } @@ -294,4 +308,5 @@ children = null; intersections = null; + crossingWaysMap = null; } Index: trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (revision 15959) @@ -5,4 +5,5 @@ import java.awt.geom.Area; +import java.awt.geom.Point2D; import java.text.MessageFormat; import java.util.ArrayList; @@ -12,4 +13,5 @@ import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.function.IntFunction; @@ -26,6 +28,9 @@ import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.osm.Relation; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.WaySegment; import org.openstreetmap.josm.data.osm.visitor.PrimitiveVisitor; import org.openstreetmap.josm.data.osm.visitor.paint.relations.MultipolygonCache; +import org.openstreetmap.josm.data.validation.tests.CrossingWays; import org.openstreetmap.josm.gui.mappaint.Environment; import org.openstreetmap.josm.gui.mappaint.Range; @@ -303,28 +308,90 @@ private final String layer; private Area area; + /** Will contain all way segments, grouped by cells */ + Map> cellSegments; private CrossingFinder(Environment e) { super(e); - CheckParameterUtil.ensureThat(e.osm instanceof IWay, "Only ways are supported"); + CheckParameterUtil.ensureThat(isArea(e.osm), "Only areas are supported"); layer = OsmUtils.getLayer(e.osm); } - @Override - public void visit(IWay w) { - if (Objects.equals(layer, OsmUtils.getLayer(w)) - && left.matches(new Environment(w).withParent(e.osm)) - && e.osm instanceof IWay) { - if (area == null) { - area = Geometry.getAreaEastNorth(e.osm); - } - Pair is = Geometry.polygonIntersectionResult( - Geometry.getAreaEastNorth(w), area, Geometry.INTERSECTION_EPS_EAST_NORTH); - if (Geometry.PolygonIntersection.CROSSING == is.a) { - addToChildren(e, w); - // store intersection area to improve highlight and zoom to problem - if (e.intersections == null) { - e.intersections = new HashMap<>(); + private Area getAreaEastNorth(IPrimitive p, Environment e) { + if (e.mpAreaCache != null && p.isMultipolygon()) { + Area a = e.mpAreaCache.get(p); + if (a == null) { + a = Geometry.getAreaEastNorth(p); + e.mpAreaCache.put(p, a); + } + return a; + } + return Geometry.getAreaEastNorth(p); + } + + private Map, List> findCrossings(IPrimitive area, + Map> cellSegments) { + /** The detected crossing ways */ + Map, List> crossingWays = new HashMap<>(50); + if (area instanceof Way) { + CrossingWays.findIntersectingWay((Way) area, cellSegments, crossingWays, false); + } else if (area instanceof Relation && area.isMultipolygon()) { + Relation r = (Relation) area; + for (Way w : r.getMemberPrimitives(Way.class)) { + if (!w.hasIncompleteNodes()) { + CrossingWays.findIntersectingWay(w, cellSegments, crossingWays, false); } - e.intersections.put(w, is.b); + } + } + return crossingWays; + } + + @Override + public void visit(Collection primitives) { + List toIgnore; + if (e.osm instanceof Relation) { + toIgnore = ((IRelation) e.osm).getMemberPrimitivesList(); + } else { + toIgnore = null; + } + + for (IPrimitive p : primitives) { + if (isPrimitiveUsable(p) && Objects.equals(layer, OsmUtils.getLayer(p)) + && left.matches(new Environment(p).withParent(e.osm)) && isArea(p) + && (toIgnore == null || !toIgnore.contains(p))) { + if (area == null) { + area = getAreaEastNorth(e.osm, e); + } + Area otherArea = getAreaEastNorth(p, e); + if (area.isEmpty() || otherArea.isEmpty()) { + if (cellSegments == null) { + // lazy initialisation + cellSegments = new HashMap<>(); + findCrossings(e.osm, cellSegments); // ignore self intersections etc. here + } + // need a copy + final Map> tmpCellSegments = new HashMap<>(cellSegments); + // calculate all crossings between e.osm and p + Map, List> crossingWays = findCrossings(p, tmpCellSegments); + if (!crossingWays.isEmpty()) { + addToChildren(e, p); + if (e.crossingWaysMap == null) { + e.crossingWaysMap = new HashMap<>(); + } + e.crossingWaysMap.put(p, crossingWays); + } + } else { + // we have complete data. This allows to find intersections with shared nodes + // See #16707 + Pair is = Geometry.polygonIntersectionResult( + otherArea, area, Geometry.INTERSECTION_EPS_EAST_NORTH); + if (Geometry.PolygonIntersection.CROSSING == is.a) { + addToChildren(e, p); + // store intersection area to improve highlight and zoom to problem + if (e.intersections == null) { + e.intersections = new HashMap<>(); + } + e.intersections.put(p, is.b); + } + } } } @@ -453,11 +520,10 @@ return ChildOrParentSelectorType.SUPERSET_OR_EQUAL == type ? e.children != null : e.children == null; - } else if (ChildOrParentSelectorType.CROSSING == type && e.osm instanceof IWay) { + } else if (ChildOrParentSelectorType.CROSSING == type) { e.parent = e.osm; - if (right instanceof OptimizedGeneralSelector - && e.osm.getDataSet() != null - && ((OptimizedGeneralSelector) right).matchesBase(OsmPrimitiveType.WAY)) { + if (e.osm.getDataSet() != null && isArea(e.osm)) { final CrossingFinder crossingFinder = new CrossingFinder(e); - crossingFinder.visit(e.osm.getDataSet().searchWays(e.osm.getBBox())); + visitBBox(e, crossingFinder); + return e.children != null; } return e.children != null; Index: trunk/src/org/openstreetmap/josm/tools/Geometry.java =================================================================== --- trunk/src/org/openstreetmap/josm/tools/Geometry.java (revision 15958) +++ trunk/src/org/openstreetmap/josm/tools/Geometry.java (revision 15959) @@ -562,13 +562,14 @@ return Geometry.getArea(((Way) p).getNodes()); } - if (p instanceof Relation && p.isMultipolygon() && !p.isIncomplete() - && !((Relation) p).hasIncompleteMembers()) { + if (p instanceof Relation && p.isMultipolygon() && !p.isIncomplete()) { Multipolygon mp = MultipolygonCache.getInstance().get((Relation) p); - Path2D path = new Path2D.Double(); - path.setWindingRule(Path2D.WIND_EVEN_ODD); - for (PolyData pd : mp.getCombinedPolygons()) { - path.append(pd.get(), false); - } - return new Area(path); + if (mp.getOpenEnds().isEmpty()) { + Path2D path = new Path2D.Double(); + path.setWindingRule(Path2D.WIND_EVEN_ODD); + for (PolyData pd : mp.getCombinedPolygons()) { + path.append(pd.get(), false); + } + return new Area(path); + } } return new Area(); Index: trunk/test/data/regress/13165/13165.osm =================================================================== --- trunk/test/data/regress/13165/13165.osm (revision 15959) +++ trunk/test/data/regress/13165/13165.osm (revision 15959) @@ -0,0 +1,99 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 15958) +++ trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 15959) @@ -357,3 +357,37 @@ } + /** + * Non-regression test for Bug #13165. + * @throws Exception if an error occurs + */ + @Test + public void testTicket13165() throws Exception { + final MapCSSTagChecker test = buildTagChecker( + "area:closed[tag(\"landuse\") = parent_tag(\"landuse\")] ⧉ area:closed[landuse] {" + + "throwWarning: tr(\"Overlapping Identical Landuses\");" + + "}"); + try (InputStream is = TestUtils.getRegressionDataStream(13165, "13165.osm")) { + test.visit(OsmReader.parseDataSet(is, null).allPrimitives()); + List errors = test.getErrors(); + assertEquals(3, errors.size()); + } + } + + /** + * Non-regression test for Bug #13165. + * @throws Exception if an error occurs + */ + @Test + public void testTicket13165IncompleteMP() throws Exception { + final MapCSSTagChecker test = buildTagChecker( + "area:closed[tag(\"landuse\") = parent_tag(\"landuse\")] ⧉ area:closed[landuse] {" + + "throwWarning: tr(\"Overlapping Identical Landuses\");" + + "}"); + try (InputStream is = TestUtils.getRegressionDataStream(13165, "13165-incomplete.osm.bz2")) { + test.visit(OsmReader.parseDataSet(is, null).allPrimitives()); + List errors = test.getErrors(); + assertEquals(3, errors.size()); + } + } + }