craigtaverner commented on code in PR #13980:
URL: https://github.com/apache/lucene/pull/13980#discussion_r1832435160
##########
lucene/core/src/java/org/apache/lucene/geo/Tessellator.java:
##########
@@ -390,12 +377,108 @@ private static final void eliminateHole(
}
}
+ /** Choose a common vertex between the polygon and the hole if it exists,
otherwise return null */
Review Comment:
The comment and method name do not match the behaviour. This method does not
find and return a common point. Instead it finds a common point and if it
exists, merges the hole(s) into the outer polygon and returns true so that the
alternative approach of Eberly's, does not need to run. Perhaps a name like
`mergeHolesWithSharedVertices` would work better?
##########
lucene/core/src/test/org/apache/lucene/geo/TestTessellator.java:
##########
@@ -928,12 +848,42 @@ public void testComplexPolygon55() throws Exception {
public void testComplexPolygon56() throws Exception {
String geoJson = GeoTestUtil.readShape("github-12352-2.geojson.gz");
Polygon[] polygons = Polygon.fromGeoJSON(geoJson);
- for (Polygon polygon : polygons) {
- List<Tessellator.Triangle> tessellation =
- Tessellator.tessellate(polygon, random().nextBoolean());
- assertEquals(area(polygon), area(tessellation), 0.0);
- // don't check edges as it takes several minutes
- }
+ checkMultiPolygon(polygons, 3e-11);
+ }
+
+ public void testComplexPolygon57() throws Exception {
+ String geoJson = GeoTestUtil.readShape("github-13841-1.geojson.gz");
+ Polygon[] polygons = Polygon.fromGeoJSON(geoJson);
+ checkMultiPolygon(polygons, 3e-11);
+ }
+
+ @Nightly
+ public void testComplexPolygon58() throws Exception {
+ String wkt = GeoTestUtil.readShape("github-13841-2.geojson.gz");
+ checkMultiPolygon(wkt);
+ }
+
+ @Nightly
+ public void testComplexPolygon59() throws Exception {
+ String wkt = GeoTestUtil.readShape("github-13841-3.geojson.gz");
+ Polygon[] polygons = (Polygon[]) SimpleWKTShapeParser.parse(wkt);
+ checkMultiPolygon(polygons, 1e-11);
+ }
+
+ public void testComplexPolygon60() throws Exception {
+ String wkt =
+ "POLYGON((0 0, 5 1, 10 0, 11 5, 10 10,5 11, 0 10, 1 5, 0 0),"
Review Comment:
Nice test, this polygon is quite a work of art! Lots of holes with common
edges and a wide range of angles.
##########
lucene/core/src/test/org/apache/lucene/geo/TestTessellator.java:
##########
@@ -928,12 +848,42 @@ public void testComplexPolygon55() throws Exception {
public void testComplexPolygon56() throws Exception {
String geoJson = GeoTestUtil.readShape("github-12352-2.geojson.gz");
Polygon[] polygons = Polygon.fromGeoJSON(geoJson);
- for (Polygon polygon : polygons) {
- List<Tessellator.Triangle> tessellation =
- Tessellator.tessellate(polygon, random().nextBoolean());
- assertEquals(area(polygon), area(tessellation), 0.0);
- // don't check edges as it takes several minutes
Review Comment:
I see you added back the edges check here, so perhaps this comment was not
valid and this test is not taking several minutes?
##########
lucene/core/src/java/org/apache/lucene/geo/Tessellator.java:
##########
@@ -350,30 +349,18 @@ private static final Node eliminateHoles(
}
/** Finds a bridge between vertices that connects a hole with an outer ring,
and links it */
- private static final void eliminateHole(
+ private static void eliminateHole(
final Node holeNode,
Node outerNode,
double holeMinX,
double holeMaxX,
double holeMinY,
double holeMaxY) {
- // Attempt to find a common point between the HoleNode and OuterNode.
- Node next = outerNode;
- do {
- if (Rectangle.containsPoint(
- next.getY(), next.getX(), holeMinY, holeMaxY, holeMinX, holeMaxX)) {
- Node sharedVertex = getSharedVertex(holeNode, next);
- if (sharedVertex != null) {
- // Split the resulting polygon.
- Node node = splitPolygon(next, sharedVertex, true);
- // Filter the split nodes.
- filterPoints(node, node.next);
- return;
- }
- }
- next = next.next;
- } while (next != outerNode);
+ // Attempt to find a common point between the HoleNode and OuterNode.
Review Comment:
This comment and method name should probably change, as discussed in the
other comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
