From 65a69b9757a6cd0a8e8be0ed08797643054634b1 Mon Sep 17 00:00:00 2001 From: Karl Wright Date: Sat, 9 Apr 2016 16:12:39 -0400 Subject: [PATCH] LUCENE-7197: Fix two test failures and add more forensics that helped resolve the issue. --- .../spatial3d/geom/GeoConcavePolygon.java | 10 ++++---- .../spatial3d/geom/GeoConvexPolygon.java | 1 + .../lucene/spatial3d/geom/GeoPoint.java | 2 +- .../spatial3d/geom/GeoPolygonFactory.java | 12 +++++++-- .../lucene/spatial3d/geom/PlanetModel.java | 25 +++++++++++++++++++ .../spatial3d/geom/StandardXYZSolid.java | 18 ++++++++++++- .../lucene/spatial3d/TestGeo3DPoint.java | 25 ++++++++++++++----- 7 files changed, 78 insertions(+), 15 deletions(-) diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConcavePolygon.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConcavePolygon.java index 05e58ee5734..8c6f7571380 100644 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConcavePolygon.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConcavePolygon.java @@ -202,11 +202,9 @@ class GeoConcavePolygon extends GeoBasePolygon { throw new IllegalArgumentException("Polygon points are all coplanar"); } final GeoPoint check = points.get(endPointIndex); - // Here note the flip of the sense of the sided plane!! - final SidedPlane sp = new SidedPlane(check, false, start, end); //System.out.println("Created edge "+sp+" using start="+start+" end="+end+" check="+check); - edges[i] = sp; - invertedEdges[i] = new SidedPlane(sp); + edges[i] = new SidedPlane(check, false, start, end); + invertedEdges[i] = new SidedPlane(edges[i]); notableEdgePoints[i] = new GeoPoint[]{start, end}; } // In order to naively confirm that the polygon is concave, I would need to @@ -277,9 +275,11 @@ class GeoConcavePolygon extends GeoBasePolygon { // cannot use them as bounds. They are independent hemispheres. for (int edgeIndex = 0; edgeIndex < edges.length; edgeIndex++) { final SidedPlane edge = edges[edgeIndex]; + final SidedPlane invertedEdge = invertedEdges[edgeIndex]; final GeoPoint[] points = this.notableEdgePoints[edgeIndex]; if (!isInternalEdges.get(edgeIndex)) { - if (edge.intersects(planetModel, p, notablePoints, points, bounds, eitherBounds.get(edge))) { + //System.err.println("Checking concave edge "+edge+" for intersection against plane "+p); + if (invertedEdge.intersects(planetModel, p, notablePoints, points, bounds, eitherBounds.get(edge))) { //System.err.println(" intersects!"); return true; } diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConvexPolygon.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConvexPolygon.java index d1e009178f3..b631b55a059 100755 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConvexPolygon.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoConvexPolygon.java @@ -265,6 +265,7 @@ class GeoConvexPolygon extends GeoBasePolygon { final SidedPlane edge = edges[edgeIndex]; final GeoPoint[] points = this.notableEdgePoints[edgeIndex]; if (!isInternalEdges.get(edgeIndex)) { + //System.err.println("Checking convex edge "+edge+" for intersection against plane "+p); if (edge.intersects(planetModel, p, notablePoints, points, bounds, eitherBounds.get(edge))) { //System.err.println(" intersects!"); return true; diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPoint.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPoint.java index 31ab0aa2f0a..662a0565ce8 100755 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPoint.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPoint.java @@ -188,6 +188,6 @@ public class GeoPoint extends Vector { if (this.longitude == Double.NEGATIVE_INFINITY) { return super.toString(); } - return "[lat="+getLatitude()+", lon="+getLongitude()+"]"; + return "[lat="+getLatitude()+", lon="+getLongitude()+"("+super.toString()+")]"; } } diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.java index a68f908dc22..7fc22dd558b 100755 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.java @@ -395,6 +395,7 @@ public class GeoPolygonFactory { if (confirmEdge == checkEdge) { continue; } + // Look for a point that is on the wrong side of the check edge. This means that we can't build the polygon. final GeoPoint thePoint; if (checkEdge.startPoint != confirmEdge.startPoint && checkEdge.endPoint != confirmEdge.startPoint && !flippedPlane.isWithin(confirmEdge.startPoint)) { thePoint = confirmEdge.startPoint; @@ -404,7 +405,11 @@ public class GeoPolygonFactory { thePoint = null; } if (thePoint != null) { - // Found a split!! + // thePoint is on the wrong side of the complementary plane. That means we cannot build a concave polygon, because the complement would not + // be a legal convex polygon. + // But we can take advantage of the fact that the distance between the edge and thePoint is less than 180 degrees, and so we can split the + // would-be concave polygon into three segments. The first segment includes the edge and thePoint, and uses the sense of the edge to determine the sense + // of the polygon. // This should be the only problematic part of the polygon. // We know that thePoint is on the "wrong" side of the edge -- that is, it's on the side that the @@ -431,6 +436,8 @@ public class GeoPolygonFactory { } //System.out.println("...done convex part."); + // ??? check if we get the sense right + // The part preceding the bad edge, back to thePoint, needs to be recursively // processed. So, assemble what we need, which is basically a list of edges. Edge loopEdge = edgeBuffer.getPrevious(checkEdge); @@ -459,6 +466,7 @@ public class GeoPolygonFactory { return false; } //System.out.println("...done first part."); + final List secondPartPoints = new ArrayList<>(); final BitSet secondPartInternal = new BitSet(); loopEdge = edgeBuffer.getNext(checkEdge); @@ -479,7 +487,7 @@ public class GeoPolygonFactory { secondPartInternal, secondPartPoints.size()-1, 0, - new SidedPlane(checkEdge.endPoint, true, checkEdge.startPoint, thePoint), + new SidedPlane(checkEdge.startPoint, false, checkEdge.endPoint, thePoint), holes, testPoint) == false) { return false; diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/PlanetModel.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/PlanetModel.java index c7d45a826c4..f5ab8d86daa 100644 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/PlanetModel.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/PlanetModel.java @@ -189,6 +189,31 @@ public class PlanetModel { return (x * x + y * y) * inverseAb * inverseAb + z * z * inverseC * inverseC - 1.0 > Vector.MINIMUM_RESOLUTION; } + /** Compute a GeoPoint that's scaled to actually be on the planet surface. + * @param vector is the vector. + * @return the scaled point. + */ + public GeoPoint createSurfacePoint(final Vector vector) { + return createSurfacePoint(vector.x, vector.y, vector.z); + } + + /** Compute a GeoPoint that's based on (x,y,z) values, but is scaled to actually be on the planet surface. + * @param x is the x value. + * @param y is the y value. + * @param z is the z value. + * @return the scaled point. + */ + public GeoPoint createSurfacePoint(final double x, final double y, final double z) { + // The equation of the surface is: + // (x^2 / a^2 + y^2 / b^2 + z^2 / c^2) = 1 + // We will need to scale the passed-in x, y, z values: + // ((tx)^2 / a^2 + (ty)^2 / b^2 + (tz)^2 / c^2) = 1 + // t^2 * (x^2 / a^2 + y^2 / b^2 + z^2 / c^2) = 1 + // t = sqrt ( 1 / (x^2 / a^2 + y^2 / b^2 + z^2 / c^2)) + final double t = Math.sqrt(1.0 / (x*x*inverseAbSquared + y*y*inverseAbSquared + z*z*inverseCSquared)); + return new GeoPoint(t*x, t*y, t*z); + } + /** Compute a GeoPoint that's a bisection between two other GeoPoints. * @param pt1 is the first point. * @param pt2 is the second point. diff --git a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/StandardXYZSolid.java b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/StandardXYZSolid.java index ec2e26ce870..9d94c519b90 100644 --- a/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/StandardXYZSolid.java +++ b/lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/StandardXYZSolid.java @@ -16,6 +16,8 @@ */ package org.apache.lucene.spatial3d.geom; +import java.util.Arrays; + /** * 3D rectangle, bounded on six sides by X,Y,Z limits * @@ -152,6 +154,11 @@ class StandardXYZSolid extends BaseXYZSolid { notableMinZPoints = glueTogether(minXminZ, maxXminZ, minYminZ, maxYminZ); notableMaxZPoints = glueTogether(minXmaxZ, maxXmaxZ, minYmaxZ, maxYmaxZ); + //System.err.println( + // " notableMinXPoints="+Arrays.asList(notableMinXPoints)+" notableMaxXPoints="+Arrays.asList(notableMaxXPoints)+ + // " notableMinYPoints="+Arrays.asList(notableMinYPoints)+" notableMaxYPoints="+Arrays.asList(notableMaxYPoints)+ + // " notableMinZPoints="+Arrays.asList(notableMinZPoints)+" notableMaxZPoints="+Arrays.asList(notableMaxZPoints)); + // Now, compute the edge points. // This is the trickiest part of setting up an XYZSolid. We've computed intersections already, so // we'll start there. @@ -173,7 +180,11 @@ class StandardXYZSolid extends BaseXYZSolid { final boolean maxXminYmaxZ = planetModel.pointOutside(maxX, minY, maxZ); final boolean maxXmaxYminZ = planetModel.pointOutside(maxX, maxY, minZ); final boolean maxXmaxYmaxZ = planetModel.pointOutside(maxX, maxY, maxZ); - + + //System.err.println("Outside world: minXminYminZ="+minXminYminZ+" minXminYmaxZ="+minXminYmaxZ+" minXmaxYminZ="+minXmaxYminZ+ + // " minXmaxYmaxZ="+minXmaxYmaxZ+" maxXminYminZ="+maxXminYminZ+" maxXminYmaxZ="+maxXminYmaxZ+" maxXmaxYminZ="+maxXmaxYminZ+ + // " maxXmaxYmaxZ="+maxXmaxYmaxZ); + // Look at single-plane/world intersections. // We detect these by looking at the world model and noting its x, y, and z bounds. @@ -286,6 +297,11 @@ class StandardXYZSolid extends BaseXYZSolid { maxZEdges = EMPTY_POINTS; } + //System.err.println( + // " minXEdges="+Arrays.asList(minXEdges)+" maxXEdges="+Arrays.asList(maxXEdges)+ + // " minYEdges="+Arrays.asList(minYEdges)+" maxYEdges="+Arrays.asList(maxYEdges)+ + // " minZEdges="+Arrays.asList(minZEdges)+" maxZEdges="+Arrays.asList(maxZEdges)); + // Glue everything together. This is not a minimal set of edgepoints, as of now, but it does completely describe all shapes on the // planet. this.edgePoints = glueTogether(minXminY, minXmaxY, minXminZ, minXmaxZ, diff --git a/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java b/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java index 4edfd2d9120..5c876ceebbb 100644 --- a/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java +++ b/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java @@ -782,6 +782,7 @@ public class TestGeo3DPoint extends LuceneTestCase { if (point != null) { boolean expected = ((deleted.contains(id) == false) && ((PointInGeo3DShapeQuery)query).getShape().isWithin(point)); if (hits.get(docID) != expected) { + GeoPoint scaledPoint = PlanetModel.WGS84.createSurfacePoint(point); StringBuilder b = new StringBuilder(); if (expected) { b.append("FAIL: id=" + id + " should have matched but did not\n"); @@ -789,10 +790,14 @@ public class TestGeo3DPoint extends LuceneTestCase { b.append("FAIL: id=" + id + " should not have matched but did\n"); } b.append(" shape=" + ((PointInGeo3DShapeQuery)query).getShape() + "\n"); + b.append(" world bounds=(" + + " minX=" + PlanetModel.WGS84.getMinimumXValue() + " maxX=" + PlanetModel.WGS84.getMaximumXValue() + + " minY=" + PlanetModel.WGS84.getMinimumYValue() + " maxY=" + PlanetModel.WGS84.getMaximumYValue() + + " minZ=" + PlanetModel.WGS84.getMinimumZValue() + " maxZ=" + PlanetModel.WGS84.getMaximumZValue() + "\n"); b.append(" point=" + point + "\n"); b.append(" docID=" + docID + " deleted?=" + deleted.contains(id) + "\n"); b.append(" query=" + query + "\n"); - b.append(" explanation:\n " + explain("point", ((PointInGeo3DShapeQuery)query).getShape(), point, r, docID).replace("\n", "\n ")); + b.append(" explanation:\n " + explain("point", ((PointInGeo3DShapeQuery)query).getShape(), point, scaledPoint, r, docID).replace("\n", "\n ")); fail(b.toString()); } } else { @@ -810,7 +815,7 @@ public class TestGeo3DPoint extends LuceneTestCase { } public void testShapeQueryToString() { - assertEquals("PointInGeo3DShapeQuery: field=point: Shape: GeoStandardCircle: {planetmodel=PlanetModel.WGS84, center=[lat=0.7722082215479366, lon=0.13560747521073413], radius=0.1(5.729577951308232)}", + assertEquals("PointInGeo3DShapeQuery: field=point: Shape: GeoStandardCircle: {planetmodel=PlanetModel.WGS84, center=[lat=0.7722082215479366, lon=0.13560747521073413([X=0.7094263130137863, Y=0.09679758930862137, Z=0.6973564619248455])], radius=0.1(5.729577951308232)}", Geo3DPoint.newShapeQuery("point", GeoCircleFactory.makeGeoCircle(PlanetModel.WGS84, toRadians(44.244272), toRadians(7.769736), 0.1)).toString()); } @@ -1171,6 +1176,7 @@ public class TestGeo3DPoint extends LuceneTestCase { final GeoShape shape; final GeoPoint targetDocPoint; + final GeoPoint scaledDocPoint; final IntersectVisitor in; final List stack = new ArrayList<>(); private List stackToTargetDoc; @@ -1183,9 +1189,10 @@ public class TestGeo3DPoint extends LuceneTestCase { // In the first phase, we always return CROSSES to do a full scan of the BKD tree to see which leaf block the document lives in boolean firstPhase = true; - public ExplainingVisitor(GeoShape shape, GeoPoint targetDocPoint, IntersectVisitor in, int targetDocID, int numDims, int bytesPerDim, StringBuilder b) { + public ExplainingVisitor(GeoShape shape, GeoPoint targetDocPoint, GeoPoint scaledDocPoint, IntersectVisitor in, int targetDocID, int numDims, int bytesPerDim, StringBuilder b) { this.shape = shape; this.targetDocPoint = targetDocPoint; + this.scaledDocPoint = scaledDocPoint; this.in = in; this.targetDocID = targetDocID; this.numDims = numDims; @@ -1302,6 +1309,9 @@ public class TestGeo3DPoint extends LuceneTestCase { final int relationship = xyzSolid.getRelationship(shape); final boolean pointWithinShape = shape.isWithin(targetDocPoint); final boolean pointWithinCell = xyzSolid.isWithin(targetDocPoint); + final boolean scaledWithinShape = shape.isWithin(scaledDocPoint); + final boolean scaledWithinCell = xyzSolid.isWithin(scaledDocPoint); + final String relationshipString; switch (relationship) { case GeoArea.CONTAINS: @@ -1320,7 +1330,10 @@ public class TestGeo3DPoint extends LuceneTestCase { relationshipString = "UNKNOWN"; break; } - return "Cell(x=" + xMin + " TO " + xMax + " y=" + yMin + " TO " + yMax + " z=" + zMin + " TO " + zMax + "); Shape relationship = "+relationshipString+"; Point within cell = "+pointWithinCell+"; Point within shape = "+pointWithinShape; + return "Cell(x=" + xMin + " TO " + xMax + " y=" + yMin + " TO " + yMax + " z=" + zMin + " TO " + zMax + + "); Shape relationship = "+relationshipString+ + "; Point within cell = "+pointWithinCell+"; Point within shape = "+pointWithinShape+ + "; Scaled point within cell = "+scaledWithinCell+"; Scaled point within shape = "+scaledWithinShape; } @Override @@ -1340,7 +1353,7 @@ public class TestGeo3DPoint extends LuceneTestCase { } } - public static String explain(String fieldName, GeoShape shape, GeoPoint targetDocPoint, IndexReader reader, int docID) throws Exception { + public static String explain(String fieldName, GeoShape shape, GeoPoint targetDocPoint, GeoPoint scaledDocPoint, IndexReader reader, int docID) throws Exception { // First find the leaf reader that owns this doc: int subIndex = ReaderUtil.subIndex(docID, reader.leaves()); @@ -1350,7 +1363,7 @@ public class TestGeo3DPoint extends LuceneTestCase { b.append("target is in leaf " + leafReader + " of full reader " + reader + "\n"); DocIdSetBuilder hits = new DocIdSetBuilder(leafReader.maxDoc()); - ExplainingVisitor visitor = new ExplainingVisitor(shape, targetDocPoint, new PointInShapeIntersectVisitor(hits, shape), docID - reader.leaves().get(subIndex).docBase, 3, Integer.BYTES, b); + ExplainingVisitor visitor = new ExplainingVisitor(shape, targetDocPoint, scaledDocPoint, new PointInShapeIntersectVisitor(hits, shape), docID - reader.leaves().get(subIndex).docBase, 3, Integer.BYTES, b); // Do first phase, where we just figure out the "path" that leads to the target docID: leafReader.getPointValues().intersect(fieldName, visitor);