search

maplibre / maplibre-tile-spec

Next generation vector tiles format
Apache License 2.0
158 stars 13 forks source link

Upgrade no.ecc.vectortile:java-vector-tile #57

Closed springmeyer closed 3 months ago

springmeyer commented 4 months ago

I noticed that the MltDecoderTest depends on 'no.ecc.vectortile:java-vector-tile:1.3.1' (https://github.com/maplibre/maplibre-tile-spec/blob/1d08bd24c7661883a3b0fd2b518880dcc0fcecf6/converter/java/build.gradle#L23) which is a release that is 6 years old: https://github.com/ElectronicChartCentre/java-vector-tile/tree/v1.3.1

The latest version is commented in the code (https://github.com/ElectronicChartCentre/java-vector-tile/releases/tag/v1.3.23) and when enabled throws errors in the tests like:

MltDecoderTest > decodeMlTile_Z2() FAILED
    java.lang.IllegalArgumentException: holes must not contain null elements
        at org.locationtech.jts.geom.Polygon.<init>(Polygon.java:122)
        at org.locationtech.jts.geom.GeometryFactory.createPolygon(GeometryFactory.java:467)
        at com.mlt.decoder.GeometryDecoder.decodeGeometry(GeometryDecoder.java:202)
        at com.mlt.decoder.MltDecoder.decodeMlTile(MltDecoder.java:67)
        at com.mlt.decoder.MltDecoderTest.testTile(MltDecoderTest.java:82)
        at com.mlt.decoder.MltDecoderTest.decodeMlTile_Z2(MltDecoderTest.java:31)

MltDecoderTest > decodeMlTile_Z4() FAILED
    java.lang.IllegalArgumentException: holes must not contain null elements
        at org.locationtech.jts.geom.Polygon.<init>(Polygon.java:122)
        at org.locationtech.jts.geom.GeometryFactory.createPolygon(GeometryFactory.java:467)
        at com.mlt.decoder.GeometryDecoder.decodeGeometry(GeometryDecoder.java:202)
        at com.mlt.decoder.MltDecoder.decodeMlTile(MltDecoder.java:67)
        at com.mlt.decoder.MltDecoderTest.testTile(MltDecoderTest.java:82)
        at com.mlt.decoder.MltDecoderTest.decodeMlTile_Z4(MltDecoderTest.java:37)

MltDecoderTest > decodeMlTile_Z5() FAILED
    java.lang.IllegalArgumentException: geometries must not contain null elements
        at org.locationtech.jts.geom.GeometryCollection.<init>(GeometryCollection.java:54)
        at org.locationtech.jts.geom.MultiPolygon.<init>(MultiPolygon.java:64)
        at org.locationtech.jts.geom.GeometryFactory.createMultiPolygon(GeometryFactory.java:346)
        at com.mlt.decoder.GeometryDecoder.decodeGeometry(GeometryDecoder.java:203)
        at com.mlt.decoder.MltDecoder.decodeMlTile(MltDecoder.java:67)
        at com.mlt.decoder.MltDecoderTest.testTile(MltDecoderTest.java:82)
        at com.mlt.decoder.MltDecoderTest.decodeMlTile_Z5(MltDecoderTest.java:46)

Given these errors it seems possible that our code is writing null geometries, which could be a bug in our code? So it seems desirable to upgrade to the latest version of no.ecc.vectortile:java-vector-tile (assuming it is the most reliable) and improve our code to work with it robustly.

/cc @mactrem @ebrelsford @nyurik

mactrem commented 4 months ago

when i started this project, i think this version was not available yet. When i recently tried to upgrade, i got the errors mentioned, but I didn't investigate them further yet, therefore i commented them out as a reminder . But i also double checked it some time ago with a second java mvt lib see https://github.com/maplibre/maplibre-tile-spec/blob/main/converter/java/src/main/java/com/mlt/converter/mvt/MvtUtils.java#L34 and https://github.com/maplibre/maplibre-tile-spec/blob/main/converter/java/src/main/java/com/mlt/converter/mvt/MvtUtils.java#L115. I think both libs had some minor issues so i had to combine them, which is now with the new version not necessary anymore i think. So yes we should upgrade to the latest version. I will have a look at it

springmeyer commented 3 months ago

I've found the root cause of the geometry errors. It looks like the old version of https://github.com/ElectronicChartCentre/java-vector-tile we are currently using did not encode MultiPolygons correctly. So, this fix https://github.com/ElectronicChartCentre/java-vector-tile/issues/18 breaks our expectations.

springmeyer commented 3 months ago

To clarify this issue I've created a new issue to track: https://github.com/maplibre/maplibre-tile-spec/issues/112. In the meantime https://github.com/maplibre/maplibre-tile-spec/pull/111 upgrades enough to deal with the feature id issue.