Changeset 18578 in josm for trunk/src/org

Timestamp:

2022-10-18T00:43:38+02:00 (2 years ago)

Author:

taylor.smock

Message:

Fix #22404: MVT background layer: Polygons not drawn

This was a two-part problem:

1. Converting shapes to areas does not always keep the same winding.
2. MVT styles are minZoom <= showable zooms < maxZoom. We were doing minZoom <= showable zooms <= maxZoom.

This also fixes some sonarlint issues in the modified files.

Location:

trunk/src/org/openstreetmap/josm/data

Files:

• imagery/vectortile/mapbox/style/Layers.java (modified) (7 diffs)
• imagery/vectortile/mapbox/style/package-info.java (added)
• osm/PrimitiveId.java (modified) (1 diff)
• vector/VectorDataStore.java (modified) (4 diffs)
• vector/VectorWay.java (modified) (1 diff)
• vector/package-info.java (added)

trunk/src/org/openstreetmap/josm/data/imagery/vectortile/mapbox/style/Layers.java

@@ -76 +76 @@
 
     /** Default paint properties for this layer */
-    private final String paint;
+    private final String paintProperties;
 
     /** A source description to be used with this layer. Required for everything <i>but</i> {@link Type#BACKGROUND} */
@@ -106 +106 @@
             this.filter = Expression.EMPTY_EXPRESSION;
         }
-        this.maxZoom = layerInfo.getInt("maxzoom", Integer.MAX_VALUE);
+        // minZoom <= showable zooms < maxZoom. This should be fractional, but our mapcss implementations expects ints.
         this.minZoom = layerInfo.getInt("minzoom", Integer.MIN_VALUE);
+        int tMaxZoom = layerInfo.getInt("maxzoom", Integer.MAX_VALUE);
+        if (tMaxZoom == Integer.MAX_VALUE) {
+            this.maxZoom = Integer.MAX_VALUE;
+        } else {
+            this.maxZoom = Math.max(this.minZoom, Math.max(0, tMaxZoom - 1));
+        }
         // There is a metadata field (I don't *think* I need it?)
         // source is only optional with {@link Type#BACKGROUND}.
@@ -123 +129 @@
                 case FILL:
                     // area
-                    this.paint = parsePaintFill(paintObject);
+                    this.paintProperties = parsePaintFill(paintObject);
                     break;
                 case LINE:
                     // way
-                    this.paint = parsePaintLine(layoutObject, paintObject);
+                    this.paintProperties = parsePaintLine(layoutObject, paintObject);
                     break;
                 case CIRCLE:
                     // point
-                    this.paint = parsePaintCircle(paintObject);
+                    this.paintProperties = parsePaintCircle(paintObject);
                     break;
                 case SYMBOL:
                     // point
-                    this.paint = parsePaintSymbol(layoutObject, paintObject);
+                    this.paintProperties = parsePaintSymbol(layoutObject, paintObject);
                     break;
                 case BACKGROUND:
                     // canvas only
-                    this.paint = parsePaintBackground(paintObject);
+                    this.paintProperties = parsePaintBackground(paintObject);
                     break;
                 default:
-                    this.paint = EMPTY_STRING;
+                    this.paintProperties = EMPTY_STRING;
                 }
             } else {
-                this.paint = EMPTY_STRING;
+                this.paintProperties = EMPTY_STRING;
             }
         } else {
-            this.paint = EMPTY_STRING;
+            this.paintProperties = EMPTY_STRING;
         }
         this.sourceLayer = layerInfo.getString("source-layer", null);
@@ -454 +460 @@
     @Override
     public String toString() {
-        if (this.filter.toString().isEmpty() && this.paint.isEmpty()) {
+        if (this.filter.toString().isEmpty() && this.paintProperties.isEmpty()) {
             return EMPTY_STRING;
         } else if (this.type == Type.BACKGROUND) {
             // AFAIK, paint has no zoom levels, and doesn't accept a layer
-            return "canvas{" + this.paint + "}";
+            return "canvas{" + this.paintProperties + "}";
         }
 
@@ -473 +479 @@
             zoomSelector = EMPTY_STRING;
         }
-        final String commonData = zoomSelector + this.filter.toString() + "::" + this.id + "{" + this.paint + "}";
+        final String commonData = zoomSelector + this.filter.toString() + "::" + this.id + "{" + this.paintProperties + "}";
 
         if (this.type == Type.CIRCLE || this.type == Type.SYMBOL) {
@@ -513 +519 @@
               && Objects.equals(this.source, o.source)
               && Objects.equals(this.filter, o.filter)
-              && Objects.equals(this.paint, o.paint);
+              && Objects.equals(this.paintProperties, o.paintProperties);
         }
         return false;
@@ -521 +527 @@
     public int hashCode() {
         return Objects.hash(this.type, this.minZoom, this.maxZoom, this.id, this.styleId, this.sourceLayer, this.source,
-          this.filter, this.paint);
+          this.filter, this.paintProperties);
     }
 }

trunk/src/org/openstreetmap/josm/data/osm/PrimitiveId.java

@@ -17 +17 @@
 
     /**
-     * Gets the type of object represented by this object.
+     * Gets the type of object represented by this object. Note that this should
+     * return the base primitive type ({@link OsmPrimitiveType#NODE},
+     * {@link OsmPrimitiveType#WAY}, and {@link OsmPrimitiveType#RELATION}).
      *
      * @return the object type

trunk/src/org/openstreetmap/josm/data/vector/VectorDataStore.java

@@ -78 +78 @@
             if (mergedRelation.getMemberPrimitivesList().stream().allMatch(IWay.class::isInstance)) {
                 // This pretty much does the "right" thing
-                this.mergeWays(mergedRelation);
+                mergeWays(mergedRelation);
             } else if (!(primitive instanceof IWay)) {
                 // Can't merge, ever (one of the childs is a node/relation)
                 mergedRelation.remove(JOSM_MERGE_TYPE_KEY);
             }
-        } else if (mergedRelation != null && primitive instanceof IRelation) {
+        } else if (mergedRelation != null && primitive instanceof VectorRelation) {
             // Just add to the relation
             ((VectorRelation) primitive).getMembers().forEach(mergedRelation::addRelationMember);
@@ -99 +99 @@
     }
 
-    private VectorPrimitive mergeWays(VectorRelation relation) {
+    private static VectorPrimitive mergeWays(VectorRelation relation) {
         List<VectorRelationMember> members = RelationSorter.sortMembersByConnectivity(relation.getMembers());
         Collection<VectorWay> relationWayList = members.stream().map(VectorRelationMember::getMember)
@@ -268 +268 @@
       Collection<VectorPrimitive> featureObjects, Area area) {
         VectorRelation vectorRelation = new VectorRelation(layer.getName());
-        for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, area.getPathIterator(null))) {
+        PathIterator pathIterator = area.getPathIterator(null);
+        int windingRule = pathIterator.getWindingRule();
+        for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, pathIterator)) {
             final String role;
             if (member instanceof VectorWay && ((VectorWay) member).isClosed()) {
+                // Area messes up the winding. See #22404.
+                if (windingRule == PathIterator.WIND_NON_ZERO) {
+                    VectorWay vectorWay = (VectorWay) member;
+                    List<VectorNode> nodes = new ArrayList<>(vectorWay.getNodes());
+                    Collections.reverse(nodes);
+                    vectorWay.setNodes(nodes);
+                }
                 role = Geometry.isClockwise(((VectorWay) member).getNodes()) ? "outer" : "inner";
             } else {
@@ -375 +384 @@
             primitive = pathToWay(tile, layer, featureObjects, (Path2D) shape).stream().findFirst().orElse(null);
         } else if (shape instanceof Area) {
-            primitive = areaToRelation(tile, layer, featureObjects, (Area) shape);
-            primitive.put(RELATION_TYPE, MULTIPOLYGON_TYPE);
+            VectorRelation vectorRelation = areaToRelation(tile, layer, featureObjects, (Area) shape);
+            if (vectorRelation.getMembersCount() != 1) {
+                primitive = vectorRelation;
+                primitive.put(RELATION_TYPE, MULTIPOLYGON_TYPE);
+            } else {
+                primitive = vectorRelation.getMember(0).getMember();
+            }
         } else {
             // We shouldn't hit this, but just in case

trunk/src/org/openstreetmap/josm/data/vector/VectorWay.java

@@ -131 +131 @@
     @Override
     public OsmPrimitiveType getType() {
+        return OsmPrimitiveType.WAY;
+    }
+
+    @Override
+    public OsmPrimitiveType getDisplayType() {
         return this.isClosed() ? OsmPrimitiveType.CLOSEDWAY : OsmPrimitiveType.WAY;
     }