Changeset 18578 in josm


Ignore:
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
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

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

    r18211 r18578  
    7676
    7777    /** Default paint properties for this layer */
    78     private final String paint;
     78    private final String paintProperties;
    7979
    8080    /** A source description to be used with this layer. Required for everything <i>but</i> {@link Type#BACKGROUND} */
    … …  
    106106            this.filter = Expression.EMPTY_EXPRESSION;
    107107        }
    108         this.maxZoom = layerInfo.getInt("maxzoom", Integer.MAX_VALUE);
     108        // minZoom <= showable zooms < maxZoom. This should be fractional, but our mapcss implementations expects ints.
    109109        this.minZoom = layerInfo.getInt("minzoom", Integer.MIN_VALUE);
     110        int tMaxZoom = layerInfo.getInt("maxzoom", Integer.MAX_VALUE);
     111        if (tMaxZoom == Integer.MAX_VALUE) {
     112            this.maxZoom = Integer.MAX_VALUE;
     113        } else {
     114            this.maxZoom = Math.max(this.minZoom, Math.max(0, tMaxZoom - 1));
     115        }
    110116        // There is a metadata field (I don't *think* I need it?)
    111117        // source is only optional with {@link Type#BACKGROUND}.
    … …  
    123129                case FILL:
    124130                    // area
    125                     this.paint = parsePaintFill(paintObject);
     131                    this.paintProperties = parsePaintFill(paintObject);
    126132                    break;
    127133                case LINE:
    128134                    // way
    129                     this.paint = parsePaintLine(layoutObject, paintObject);
     135                    this.paintProperties = parsePaintLine(layoutObject, paintObject);
    130136                    break;
    131137                case CIRCLE:
    132138                    // point
    133                     this.paint = parsePaintCircle(paintObject);
     139                    this.paintProperties = parsePaintCircle(paintObject);
    134140                    break;
    135141                case SYMBOL:
    136142                    // point
    137                     this.paint = parsePaintSymbol(layoutObject, paintObject);
     143                    this.paintProperties = parsePaintSymbol(layoutObject, paintObject);
    138144                    break;
    139145                case BACKGROUND:
    140146                    // canvas only
    141                     this.paint = parsePaintBackground(paintObject);
     147                    this.paintProperties = parsePaintBackground(paintObject);
    142148                    break;
    143149                default:
    144                     this.paint = EMPTY_STRING;
     150                    this.paintProperties = EMPTY_STRING;
    145151                }
    146152            } else {
    147                 this.paint = EMPTY_STRING;
     153                this.paintProperties = EMPTY_STRING;
    148154            }
    149155        } else {
    150             this.paint = EMPTY_STRING;
     156            this.paintProperties = EMPTY_STRING;
    151157        }
    152158        this.sourceLayer = layerInfo.getString("source-layer", null);
    … …  
    454460    @Override
    455461    public String toString() {
    456         if (this.filter.toString().isEmpty() && this.paint.isEmpty()) {
     462        if (this.filter.toString().isEmpty() && this.paintProperties.isEmpty()) {
    457463            return EMPTY_STRING;
    458464        } else if (this.type == Type.BACKGROUND) {
    459465            // AFAIK, paint has no zoom levels, and doesn't accept a layer
    460             return "canvas{" + this.paint + "}";
     466            return "canvas{" + this.paintProperties + "}";
    461467        }
    462468
    … …  
    473479            zoomSelector = EMPTY_STRING;
    474480        }
    475         final String commonData = zoomSelector + this.filter.toString() + "::" + this.id + "{" + this.paint + "}";
     481        final String commonData = zoomSelector + this.filter.toString() + "::" + this.id + "{" + this.paintProperties + "}";
    476482
    477483        if (this.type == Type.CIRCLE || this.type == Type.SYMBOL) {
    … …  
    513519              && Objects.equals(this.source, o.source)
    514520              && Objects.equals(this.filter, o.filter)
    515               && Objects.equals(this.paint, o.paint);
     521              && Objects.equals(this.paintProperties, o.paintProperties);
    516522        }
    517523        return false;
    … …  
    521527    public int hashCode() {
    522528        return Objects.hash(this.type, this.minZoom, this.maxZoom, this.id, this.styleId, this.sourceLayer, this.source,
    523           this.filter, this.paint);
     529          this.filter, this.paintProperties);
    524530    }
    525531}

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

    r13924 r18578  
    1717
    1818    /**
    19      * Gets the type of object represented by this object.
     19     * Gets the type of object represented by this object. Note that this should
     20     * return the base primitive type ({@link OsmPrimitiveType#NODE},
     21     * {@link OsmPrimitiveType#WAY}, and {@link OsmPrimitiveType#RELATION}).
    2022     *
    2123     * @return the object type

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

    r18478 r18578  
    7878            if (mergedRelation.getMemberPrimitivesList().stream().allMatch(IWay.class::isInstance)) {
    7979                // This pretty much does the "right" thing
    80                 this.mergeWays(mergedRelation);
     80                mergeWays(mergedRelation);
    8181            } else if (!(primitive instanceof IWay)) {
    8282                // Can't merge, ever (one of the childs is a node/relation)
    8383                mergedRelation.remove(JOSM_MERGE_TYPE_KEY);
    8484            }
    85         } else if (mergedRelation != null && primitive instanceof IRelation) {
     85        } else if (mergedRelation != null && primitive instanceof VectorRelation) {
    8686            // Just add to the relation
    8787            ((VectorRelation) primitive).getMembers().forEach(mergedRelation::addRelationMember);
    … …  
    9999    }
    100100
    101     private VectorPrimitive mergeWays(VectorRelation relation) {
     101    private static VectorPrimitive mergeWays(VectorRelation relation) {
    102102        List<VectorRelationMember> members = RelationSorter.sortMembersByConnectivity(relation.getMembers());
    103103        Collection<VectorWay> relationWayList = members.stream().map(VectorRelationMember::getMember)
    … …  
    268268      Collection<VectorPrimitive> featureObjects, Area area) {
    269269        VectorRelation vectorRelation = new VectorRelation(layer.getName());
    270         for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, area.getPathIterator(null))) {
     270        PathIterator pathIterator = area.getPathIterator(null);
     271        int windingRule = pathIterator.getWindingRule();
     272        for (VectorPrimitive member : pathIteratorToObjects(tile, layer, featureObjects, pathIterator)) {
    271273            final String role;
    272274            if (member instanceof VectorWay && ((VectorWay) member).isClosed()) {
     275                // Area messes up the winding. See #22404.
     276                if (windingRule == PathIterator.WIND_NON_ZERO) {
     277                    VectorWay vectorWay = (VectorWay) member;
     278                    List<VectorNode> nodes = new ArrayList<>(vectorWay.getNodes());
     279                    Collections.reverse(nodes);
     280                    vectorWay.setNodes(nodes);
     281                }
    273282                role = Geometry.isClockwise(((VectorWay) member).getNodes()) ? "outer" : "inner";
    274283            } else {
    … …  
    375384            primitive = pathToWay(tile, layer, featureObjects, (Path2D) shape).stream().findFirst().orElse(null);
    376385        } else if (shape instanceof Area) {
    377             primitive = areaToRelation(tile, layer, featureObjects, (Area) shape);
    378             primitive.put(RELATION_TYPE, MULTIPOLYGON_TYPE);
     386            VectorRelation vectorRelation = areaToRelation(tile, layer, featureObjects, (Area) shape);
     387            if (vectorRelation.getMembersCount() != 1) {
     388                primitive = vectorRelation;
     389                primitive.put(RELATION_TYPE, MULTIPOLYGON_TYPE);
     390            } else {
     391                primitive = vectorRelation.getMember(0).getMember();
     392            }
    379393        } else {
    380394            // We shouldn't hit this, but just in case

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

    r18214 r18578  
    131131    @Override
    132132    public OsmPrimitiveType getType() {
     133        return OsmPrimitiveType.WAY;
     134    }
     135
     136    @Override
     137    public OsmPrimitiveType getDisplayType() {
    133138        return this.isClosed() ? OsmPrimitiveType.CLOSEDWAY : OsmPrimitiveType.WAY;
    134139    }

  • trunk/test/unit/org/openstreetmap/josm/data/imagery/vectortile/mapbox/style/LayersTest.java

    r17879 r18578  
    6262    void testFill() {
    6363        // Test a layer without a source (should fail)
    64         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     64        JsonObject emptyFill = Json.createObjectBuilder()
    6565          .add("type", Layers.Type.FILL.name())
    66           .add("id", "Empty Fill").build()));
     66          .add("id", "Empty Fill").build();
     67        assertThrows(NullPointerException.class, () -> new Layers(emptyFill));
    6768
    6869        // Test an empty fill layer
    … …  
    118119    void testLine() {
    119120        // Test a layer without a source (should fail)
    120         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
    121           .add("type", Layers.Type.LINE.name())
    122           .add("id", "Empty Line").build()));
     121        JsonObject emptyLine = Json.createObjectBuilder()
     122          .add("type", Layers.Type.RASTER.name())
     123          .add("id", "Empty Raster").build();
     124        assertThrows(NullPointerException.class, () -> new Layers(emptyLine));
    123125
    124126        JsonObject allLayoutProperties = Json.createObjectBuilder()
    … …  
    173175    void testSymbol() {
    174176        // Test a layer without a source (should fail)
    175         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
    176           .add("type", Layers.Type.SYMBOL.name())
    177           .add("id", "Empty Symbol").build()));
     177        JsonObject emptySymbol = Json.createObjectBuilder()
     178          .add("type", Layers.Type.RASTER.name())
     179          .add("id", "Empty Raster").build();
     180        assertThrows(NullPointerException.class, () -> new Layers(emptySymbol));
    178181
    179182        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    306309    void testRaster() {
    307310        // Test a layer without a source (should fail)
    308         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     311        JsonObject emptyRaster = Json.createObjectBuilder()
    309312          .add("type", Layers.Type.RASTER.name())
    310           .add("id", "Empty Raster").build()));
     313          .add("id", "Empty Raster").build();
     314        assertThrows(NullPointerException.class, () -> new Layers(emptyRaster));
    311315
    312316        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    349353    void testCircle() {
    350354        // Test a layer without a source (should fail)
    351         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     355        JsonObject emptyCircle = Json.createObjectBuilder()
    352356          .add("type", Layers.Type.CIRCLE.name())
    353           .add("id", "Empty Circle").build()));
     357          .add("id", "Empty Circle").build();
     358        assertThrows(NullPointerException.class, () -> new Layers(emptyCircle));
    354359
    355360        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    400405    void testFillExtrusion() {
    401406        // Test a layer without a source (should fail)
    402         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     407        JsonObject emptyFillExtrusion = Json.createObjectBuilder()
    403408          .add("type", Layers.Type.FILL_EXTRUSION.name())
    404           .add("id", "Empty Fill Extrusion").build()));
     409          .add("id", "Empty Fill Extrusion").build();
     410        assertThrows(NullPointerException.class, () -> new Layers(emptyFillExtrusion));
    405411
    406412        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    439445    void testHeatmap() {
    440446        // Test a layer without a source (should fail)
    441         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     447        JsonObject emptyHeatmap = Json.createObjectBuilder()
    442448          .add("type", Layers.Type.HEATMAP.name())
    443           .add("id", "Empty Heatmap").build()));
     449          .add("id", "Empty Heatmap").build();
     450        assertThrows(NullPointerException.class, () -> new Layers(emptyHeatmap));
    444451
    445452        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    476483    void testHillshade() {
    477484        // Test a layer without a source (should fail)
    478         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     485        JsonObject emptyHillshade = Json.createObjectBuilder()
    479486          .add("type", Layers.Type.HILLSHADE.name())
    480           .add("id", "Empty Hillshade").build()));
     487          .add("id", "Empty Hillshade").build();
     488        assertThrows(NullPointerException.class, () -> new Layers(emptyHillshade));
    481489
    482490        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    516524    void testSky() {
    517525        // Test a layer without a source (should fail)
    518         assertThrows(NullPointerException.class, () -> new Layers(Json.createObjectBuilder()
     526        JsonObject emptySky = Json.createObjectBuilder()
    519527          .add("type", Layers.Type.SKY.name())
    520           .add("id", "Empty Sky").build()));
     528          .add("id", "Empty Sky").build();
     529        assertThrows(NullPointerException.class, () -> new Layers(emptySky));
    521530
    522531        JsonObject allPaintProperties = Json.createObjectBuilder()
    … …  
    580589          .add("maxzoom", 24)
    581590          .build());
    582         assertEquals(MessageFormat.format(baseString, "|z-24"), maxZoomLayer.toString());
     591        assertEquals(MessageFormat.format(baseString, "|z-23"), maxZoomLayer.toString());
    583592
    584593        Layers minMaxZoomLayer = new Layers(Json.createObjectBuilder(baseInformation)
    585594          .add("minzoom", 1)
    586           .add("maxzoom", 2)
     595          .add("maxzoom", 3)
    587596          .build());
    588597        assertEquals(MessageFormat.format(baseString, "|z1-2"), minMaxZoomLayer.toString());
    … …  
    593602          .build());
    594603        assertEquals(MessageFormat.format(baseString, "|z2"), sameMinMaxZoomLayer.toString());
     604        Layers zeroMaxZoom = new Layers(Json.createObjectBuilder(baseInformation)
     605                .add("maxzoom", 0)
     606                .build());
     607        assertEquals(MessageFormat.format(baseString, "|z-0"), zeroMaxZoom.toString());
    595608    }
    596609

  • trunk/test/unit/org/openstreetmap/josm/data/vector/VectorWayTest.java

    r18037 r18578  
    8787        assertFalse(way.isClosed());
    8888        assertEquals(OsmPrimitiveType.WAY, way.getType());
     89        assertEquals(OsmPrimitiveType.WAY, way.getDisplayType());
    8990        List<VectorNode> nodes = new ArrayList<>(way.getNodes());
    9091        nodes.add(nodes.get(0));
    9192        way.setNodes(nodes);
    9293        assertTrue(way.isClosed());
    93         assertEquals(OsmPrimitiveType.CLOSEDWAY, way.getType());
     94        assertEquals(OsmPrimitiveType.WAY, way.getType());
     95        assertEquals(OsmPrimitiveType.CLOSEDWAY, way.getDisplayType());
    9496    }
    9597
Note: See TracChangeset for help on using the changeset viewer.