Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build-tools-internal/version.properties
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ bundled_jdk_vendor = openjdk
bundled_jdk = 25.0.1+8@2fbf10d8c78e40bd87641c434705079d
# optional dependencies
spatial4j = 0.7
jts = 1.15.0
jts = 1.20.0
jackson = 2.15.0
snakeyaml = 2.0
icu4j = 77.1
Expand Down
5 changes: 5 additions & 0 deletions docs/changelog/138351.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 138351
summary: Bumps jts version to 1.20.0
area: Geo
type: upgrade
issues: []
6 changes: 3 additions & 3 deletions gradle/verification-metadata.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4478,9 +4478,9 @@
<sha256 value="9b93fb605b9fa0b750a69212a0141eb648c6c90f3b7f2c48dd8cdd2b72513109" origin="Generated by Gradle"/>
</artifact>
</component>
<component group="org.locationtech.jts" name="jts-core" version="1.15.0">
<artifact name="jts-core-1.15.0.jar">
<sha256 value="00102cde26c457b81fbb0248e4f8845884243caba0dc9b7fb42e0ea877383bc1" origin="Generated by Gradle"/>
<component group="org.locationtech.jts" name="jts-core" version="1.20.0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the verification data for the old JTS (line 4264 above)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this is a good idea, to remove the old line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please remove the unused line.

<artifact name="jts-core-1.20.0.jar">
<sha256 value="6a783d8f9dba3d3cf7265435f134402f63c05838aa6cbcc4297ad3a5b2842baf" origin="Generated by Gradle"/>
</artifact>
</component>
<component group="org.locationtech.spatial4j" name="spatial4j" version="0.7">
Expand Down
2 changes: 1 addition & 1 deletion modules/legacy-geo/src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
requires org.apache.lucene.core;
requires org.apache.lucene.spatial_extras;
requires org.apache.logging.log4j;
requires jts.core;
requires org.locationtech.jts;
requires spatial4j;

exports org.elasticsearch.legacygeo.mapper;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public void testPolygonWithUndefinedOrientationDueToCollinearPoints() {
new CoordinatesBuilder().coordinate(0.0, 0.0).coordinate(1.0, 1.0).coordinate(-1.0, -1.0).close()
);
InvalidShapeException e = expectThrows(InvalidShapeException.class, pb::buildS4J);
assertEquals("Self-intersection at or near point (-1.0, -1.0, NaN)", e.getMessage());
assertEquals("Self-intersection at or near point (0.0, 0.0, NaN)", e.getMessage());
}

public void testCrossingDateline() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public byte[] points(List<GeoPoint> multiPoint) {
* Returns a List {@code byte[]} containing the mvt representation of the provided geometry
*/
public List<byte[]> getFeatures(Geometry geometry) {
// Get geometry in pixel coordinates
// Get clipped and simplified geometry in pixel coordinates
final org.locationtech.jts.geom.Geometry mvtGeometry = geometry.visit(mvtGeometryBuilder);
if (mvtGeometry == null) {
return List.of();
Expand All @@ -127,6 +127,15 @@ public List<byte[]> getFeatures(Geometry geometry) {
return byteFeatures;
}

/**
* Testing purposes only, allowing us to test the impact of the geometry clipping and simplification
* without needing to parse the protobuffer results.
*/
org.locationtech.jts.geom.Geometry getMvtGeometry(Geometry geometry) {
// Get clipped and simplified geometry in pixel coordinates
return geometry.visit(mvtGeometryBuilder);
}

private record MVTGeometryBuilder(
GeometryFactory geomFactory,
org.locationtech.jts.geom.Geometry clipTile,
Expand Down Expand Up @@ -332,8 +341,13 @@ private MvtCoordinateSequenceFilter(Envelope tileEnvelope, int extent) {

@Override
public void filter(CoordinateSequence seq, int i) {
seq.setOrdinate(i, 0, lon(seq.getOrdinate(i, 0)));
seq.setOrdinate(i, 1, lat(seq.getOrdinate(i, 1)));
// JTS can sometimes close polygons using references to the starting coordinate, which can lead to that coordinate
// being converted twice. We need to detect if the end is the same actual Coordinate, and not do the conversion again.
if (i != 0 && i == seq.size() - 1 && seq.getCoordinate(0) == seq.getCoordinate(i)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment here might be useful. It is especially not clear why we need to do reference equality vs contents equality. We could add a comment like:

// JTS can sometimes close polygons using references to the starting coordinate, which can lead to that coordinate being converted twice.
// We need to detect if the end is the same actual Coordinate, and not do the conversion again.

return;
}
seq.setOrdinate(i, CoordinateSequence.X, lon(seq.getOrdinate(i, CoordinateSequence.X)));
seq.setOrdinate(i, CoordinateSequence.Y, lat(seq.getOrdinate(i, CoordinateSequence.Y)));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.nio.charset.StandardCharsets;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.function.Consumer;
Expand Down Expand Up @@ -292,6 +293,44 @@ private void assertParsing(InputStream is) throws IOException, ParseException {
assertThat(builder.getFeatures(geometry), iterableWithSize(1));
}

public void testVectorTilesDoNotContainOriginWithSimplifiableGeometries() {
Polygon polygon = new Polygon(
new LinearRing(
new double[] { -94.491034, -94.493892, -94.399587, -94.483604, -94.491034 },
new double[] { -72.522177, -72.491761, -72.607412, -72.545519, -72.522177 }
)
);

final FeatureFactory builder = new FeatureFactory(0, 0, 0, 4096, 5);

var mvtGeometry = builder.getMvtGeometry(polygon);
var coordinates = mvtGeometry.getCoordinates();
var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
assertFalse("Coordinate (2048,2048) only occurs if we are converting the first / last coordinate twice", containsCenter);
}

public void testVectorTilesDoNotContainOriginWithComplexGeometries() throws IOException, ParseException {
// make sure we can parse big polygons
var is = new GZIPInputStream(getClass().getResourceAsStream("Antarctica.wkt.gz"));
final BufferedReader reader = new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8));
final Geometry geometry = WellKnownText.fromWKT(StandardValidator.instance(true), true, reader.readLine());
final FeatureFactory builder = new FeatureFactory(0, 0, 0, 4096, 5);

var mvtGeometry = builder.getMvtGeometry(geometry);
var coordinates = mvtGeometry.getCoordinates();
var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
assertFalse("Coordinate (2048,2048) only occurs if we are converting the first / last coordinate twice", containsCenter);
}

public void testVectorTilesDoNotContainOriginWithNonSimplifiableGeometries() {
Polygon polygon = new Polygon(new LinearRing(new double[] { -10, 10, 10, -10, -10 }, new double[] { -10, -10, 10, 10, -10 }));
final FeatureFactory builder = new FeatureFactory(0, 0, 0, 4096, 5);
var mvtGeometry = builder.getMvtGeometry(polygon);
var coordinates = mvtGeometry.getCoordinates();
var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
assertFalse("Coordinate (2048,2048) only occurs if we are converting the first / last coordinate twice", containsCenter);
}

public void testPolygonOrientation() throws IOException {
Polygon polygon = new Polygon(
new LinearRing(new double[] { -10, 10, 10, -10, -10 }, new double[] { -10, -10, 10, 10, -10 }),
Expand Down