Add optional pre triangulation for polygons when creating MLT #383
Conversation
…y column - Add unit tests
nyurik
requested review from
mactrem,
springmeyer and
msbarry
and removed request for
springmeyer
| @@ -27,11 +27,13 @@ dependencies { | |||
| implementation 'org.apache.orc:orc-core:1.8.1' | |||
| implementation 'com.github.davidmoten:hilbert-curve:0.2.3' | |||
| implementation 'com.carrotsearch:hppc:0.10.0' | |||
| implementation "io.github.earcut4j:earcut4j:2.2.2" | |||
There was a problem hiding this comment.
We already depend on JTS, which has a few built-in polygon triangulation utilities: https://github.com/locationtech/jts/tree/master/modules/core/src/main/java/org/locationtech/jts/triangulate/polygon - and I think PolygonEarClipper is similar to earcut. Do you have any quality/performance comparison of those vs. earcut4j? If earcut4j performs better then adding a new dependency seems fine, but if not I'd default to the dependencies we already have.
| @@ -266,7 +272,8 @@ private static byte[] encodePropertyColumns( | |||
| FeatureTableOptimizations featureTableOptimizations, | |||
| List<Feature> sortedFeatures, | |||
| List<Feature> mvtFeatures, | |||
| PhysicalLevelTechnique physicalLevelTechnique) { | |||
| PhysicalLevelTechnique physicalLevelTechnique, | |||
| Boolean triangulatePolygons) { | |||
There was a problem hiding this comment.
use a primitive boolean here
| @@ -33,12 +34,15 @@ private GeometryEncoder() {} | |||
| public static EncodedGeometryColumn encodeGeometryColumn( | |||
| List<Geometry> geometries, | |||
| PhysicalLevelTechnique physicalLevelTechnique, | |||
| SortSettings sortSettings) { | |||
| SortSettings sortSettings, | |||
| boolean triangulatePolygons) { | |||
There was a problem hiding this comment.
Could we make it possible this take the triangulated polygons as an input to this function instead of embedding the triangulation inside of it?
On planetiler I'm considering some options for fixing invalid polygons that produce triangulated geometries as an intermediate step and I'd like for it to at least be possible to pass those triangulated polygons into this routine without forcing them to be computed again.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class TriangulatedPolygon { |
There was a problem hiding this comment.
If I'm reading build.gradle correctly, it looks like we're compiling against java 17. If that's right could this class just be record TriangulatedPolygon(List<Integer> indexBuffer, int numTrianglesPerPolygon) {} ?
| var geometry = feature.getGeometry().toString(); | ||
| if (geometry.contains(Geometry.TYPENAME_MULTIPOLYGON.toUpperCase())) { | ||
| triangulateMultiPolygon((MultiPolygon) feature.getGeometry()); | ||
| } else if (geometry.contains(Geometry.TYPENAME_POLYGON.toUpperCase())) { | ||
| triangulatePolygon((Polygon) feature.getGeometry()); | ||
| } |
There was a problem hiding this comment.
Can we change these places that switch based on type of geometry to:
| var geometry = feature.getGeometry().toString(); | |
| if (geometry.contains(Geometry.TYPENAME_MULTIPOLYGON.toUpperCase())) { | |
| triangulateMultiPolygon((MultiPolygon) feature.getGeometry()); | |
| } else if (geometry.contains(Geometry.TYPENAME_POLYGON.toUpperCase())) { | |
| triangulatePolygon((Polygon) feature.getGeometry()); | |
| } | |
| if (feature.getGeometry() instanceof MultiPolygon multiPolygon) { | |
| triangulateMultiPolygon(multiPolygon); | |
| } else if (feature.getGeometry() instanceof Polygon polygon) { | |
| triangulateMultiPolygon(polygon); | |
| } |
| public class TriangulationUtils { | ||
| private TriangulationUtils() {} | ||
|
|
||
| public static TriangulatedPolygon triangulatePolygon(Polygon polygon) { |
There was a problem hiding this comment.
A Polygon can have holes, and a MultiPolygon just contains many Polygons (which may each have holes). Can we change this to handle those cases and use the poly.getExteriorRing() poly.getNumInteriorRing() poly.getInteriorRingN(int n) helpers? Let's add some tests for polygons with holes as well.
| // assertion: first polygon defines the outer linear ring and the other polygons define its | ||
| // holes! | ||
| if (i == 0) { | ||
| holeIndex = multiPolygon.getGeometryN(i).getCoordinates().length; |
There was a problem hiding this comment.
Each call to getCoordinates may be transforming a packed double[] CoordinateSequence to a Coordinate[] array. Is it possible to just extract the CoordinateSequence once and use double getX(int idx), double getY(int idx) and double getZ(int idx) (only when coordinateSequence.getDimension() > 2)
| when(innerPolygon.getCoordinates()).thenReturn(innerPolygonCoordinates); | ||
|
|
||
| when(multiPolygon.getNumGeometries()).thenReturn(2); | ||
| when(multiPolygon.getGeometryN(0)).thenReturn(outerPolygon); | ||
| when(multiPolygon.getGeometryN(1)).thenReturn(innerPolygon); |
There was a problem hiding this comment.
I'd suggest just creating the actual MultiPolygon/Polygon classes instead of using mocks. One easy way is to use new WKTReader().read("POLYGON ((30 10, 40 40, 20 40, 10 20, 30 10))") to decode the well-known-text representation of a geometry
Summary
Adjustments in: