Improve the java decoder tests #168
Conversation
| assertEquals(mvtFeature.id(), mltFeature.id()); | ||
|
|
||
| var mltGeometry = mltFeature.geometry(); | ||
| var mvtGeometry = mvtFeature.geometry(); | ||
| assertEquals(mvtGeometry, mltGeometry); | ||
| if (!mvtGeometry.toString().equals(mltGeometry.toString())) { |
There was a problem hiding this comment.
Do you get any additonal error with this change?
The Geometry classes used for the equlity tests are from the JTS Topology Suite library, which does proper geometry equality comparison based on the specific geometry types and is battle tested :) Since we are provding no tolerance values for the current equality comparision we are also not safe for floating point rounding errors like in the String variant but we could introduce a value for this to be more robust.
By using toString JTS converts it to WKT.
What are your thoughts on this?
There was a problem hiding this comment.
Yes, I also tried with if (!mvtGeometry.equals(mltGeometry)) { and I get 7 exceptions like:
MltDecoderTest > Decode OMT Tiles > com.mlt.decoder.MltDecoderTest.decodeOMTTiles(String)[13] FAILED
org.locationtech.jts.geom.TopologyException: side location conflict [ (2639.311475409836, 3217.967213114754, NaN) ]
at app//org.locationtech.jts.geomgraph.EdgeEndStar.propagateSideLabels(EdgeEndStar.java:289)
at app//org.locationtech.jts.geomgraph.EdgeEndStar.computeLabelling(EdgeEndStar.java:125)
at app//org.locationtech.jts.operation.relate.RelateComputer.labelNodeEdges(RelateComputer.java:325)
at app//org.locationtech.jts.operation.relate.RelateComputer.computeIM(RelateComputer.java:125)
at app//org.locationtech.jts.operation.relate.RelateOp.getIntersectionMatrix(RelateOp.java:109)
at app//org.locationtech.jts.operation.relate.RelateOp.relate(RelateOp.java:54)
at app//org.locationtech.jts.geom.Geometry.relate(Geometry.java:1035)
at app//org.locationtech.jts.geom.Geometry.equalsTopo(Geometry.java:1089)
at app//org.locationtech.jts.geom.Geometry.equals(Geometry.java:1057)
at app//com.mlt.TestUtils.compareTilesSequential(TestUtils.java:93)
at app//com.mlt.decoder.MltDecoderTest.testTile(MltDecoderTest.java:158)
at app//com.mlt.decoder.MltDecoderTest.decodeOMTTiles(MltDecoderTest.java:122)
Do you also see that locally?
Try changing from:
assertEquals(mvtGeometry, mltGeometry);
To:
import static org.junit.jupiter.api.Assertions.assertTrue;
assertTrue(mvtGeometry.equals(mltGeometry), "MVTGeom != MLTGeom");
There was a problem hiding this comment.
For now in this PR I've reverted this custom handling: [cb75bdc](https://github.com/maplibre/maplibre-tile-spec/pull/168/commits/cb75bdc075ceec0d37dcfdf227177c657d41c42e)
Let's discuss larger issue at #178
| return Stream.of( | ||
| Triple.of(2, 2, 2), | ||
| Triple.of(3, 4, 5), | ||
| Triple.of(4, 8, 10), |
There was a problem hiding this comment.
My thoughts on this were -> by using int values we can dynamically change the file name, e.g. change the separator of xyz in an easy way as i have tiles lying around that have different separators.
This change is ok for me, but maybe we can agree on a common file naming structure for all tiles so using "_" or "-" for example
There was a problem hiding this comment.
Ah, okay. Yes, how about we standardize on - as the separator? z-x-y.mvt? I can make the change in this PR to rename any non-conforming tiles. Does that sound good?
There was a problem hiding this comment.
I can make the change in this PR to rename any non-conforming tiles. Does that sound good?
Actually, it's a rather large change to do the renaming. I'd like to do in a followup PR instead of this one as it will require updating JS side as well.
There was a problem hiding this comment.
ticketed at #179 to ensure we agree and then I can get a PR going after.
| return Stream.of( | ||
| Triple.of(2, 2, 2), | ||
| Triple.of(3, 4, 5), | ||
| Triple.of(4, 8, 10), |
Currently we support both a "Non-vectorized (aka sequential)" decoding path and a "Vectorized" decoding path. In the future we can and should consolidate to just one (vectorized), but for the moment we need to keep two because the JS decoder is targeting the non-vectorized path.
With that in mind we need to ensure we are testing the non-vectorized java decoder.
So, this PR makes improvements to the decoder tests to: