Merge pull request #279 from kwmsmith/fix-polyfill

trivial fix to bboxHexRadius to guarantee containment

CHANGELOG.md

@@ -8,6 +8,7 @@ The public API of this library consists of the functions declared in file
 ## [Unreleased]
 ### Fixed
 - `compact` handles zero length input correctly. (#278)
+- `bboxHexRadius` scaling factor adjusted to guarantee containment for `polyfill`. (#279)
 
 ## [3.6.0] - 2019-08-12
 ### Added

CMakeLists.txt

@@ -143,6 +143,7 @@ set(OTHER_SOURCE_FILES
     src/apps/testapps/testVertexGraph.c
     src/apps/testapps/testCompact.c
     src/apps/testapps/testPolyfill.c
+    src/apps/testapps/testPolyfill_GH136.c
     src/apps/testapps/testPentagonIndexes.c
     src/apps/testapps/testKRing.c
     src/apps/testapps/testH3ToGeoBoundary.c
@@ -506,6 +507,7 @@ if(BUILD_TESTING)
     add_h3_test(testH3SetToVertexGraph src/apps/testapps/testH3SetToVertexGraph.c)
     add_h3_test(testLinkedGeo src/apps/testapps/testLinkedGeo.c)
     add_h3_test(testPolyfill src/apps/testapps/testPolyfill.c)
+    add_h3_test(testPolyfill_GH136 src/apps/testapps/testPolyfill_GH136.c)
     add_h3_test(testVertexGraph src/apps/testapps/testVertexGraph.c)
     add_h3_test(testH3UniEdge src/apps/testapps/testH3UniEdge.c)
     add_h3_test(testGeoCoord src/apps/testapps/testGeoCoord.c)

src/apps/testapps/testPolyfill.c

@@ -64,10 +64,10 @@ SUITE(polyfill) {
 
     TEST(maxPolyfillSize) {
         int numHexagons = H3_EXPORT(maxPolyfillSize)(&sfGeoPolygon, 9);
-        t_assert(numHexagons == 3169, "got expected max polyfill size");
+        t_assert(numHexagons == 7057, "got expected max polyfill size");
 
         numHexagons = H3_EXPORT(maxPolyfillSize)(&holeGeoPolygon, 9);
-        t_assert(numHexagons == 3169, "got expected max polyfill size (hole)");
+        t_assert(numHexagons == 7057, "got expected max polyfill size (hole)");
 
         numHexagons = H3_EXPORT(maxPolyfillSize)(&emptyGeoPolygon, 9);
         t_assert(numHexagons == 1, "got expected max polyfill size (empty)");

src/apps/testapps/testPolyfill_GH136.c

@@ -0,0 +1,58 @@
+/*
+ * Copyright 2017-2019 Uber Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <stdlib.h>
+#include "algos.h"
+#include "constants.h"
+#include "geoCoord.h"
+#include "h3Index.h"
+#include "test.h"
+
+// https://github.com/uber/h3/issues/136
+
+static GeoCoord testVerts[] = {{0.10068990369902957, 0.8920772174196191},
+                               {0.10032914690616246, 0.8915914753447348},
+                               {0.10033349237998787, 0.8915860128746426},
+                               {0.10069496685903621, 0.8920742194546231}};
+static Geofence testGeofence = {.numVerts = 4, .verts = testVerts};
+static GeoPolygon testPolygon;
+
+static int countActualHexagons(H3Index* hexagons, int numHexagons) {
+    int actualNumHexagons = 0;
+    for (int i = 0; i < numHexagons; i++) {
+        if (hexagons[i] != 0) {
+            actualNumHexagons++;
+        }
+    }
+    return actualNumHexagons;
+}
+
+SUITE(polyfill_gh136) {
+    testPolygon.geofence = testGeofence;
+    testPolygon.numHoles = 0;
+
+    TEST(gh136) {
+        int res = 13;
+        int numHexagons = H3_EXPORT(maxPolyfillSize)(&testPolygon, res);
+        H3Index* hexagons = calloc(numHexagons, sizeof(H3Index));
+
+        H3_EXPORT(polyfill)(&testPolygon, res, hexagons);
+        int actualNumHexagons = countActualHexagons(hexagons, numHexagons);
+
+        t_assert(actualNumHexagons == 4353, "got expected polyfill size");
+        free(hexagons);
+    }
+}

src/h3lib/lib/bbox.c

@@ -112,10 +112,14 @@ int bboxHexRadius(const BBox* bbox, int res) {
     // Determine the radius of the center hexagon
     double centerHexRadiusKm = _hexRadiusKm(H3_EXPORT(geoToH3)(&center, res));
 
-    // The closest point along a hexagon drawn through the center points
-    // of a k-ring aggregation is exactly 1.5 radii of the hexagon. For
-    // any orientation of the GeoJSON encased in a circle defined by the
-    // bounding box radius and center, it is guaranteed to fit in this k-ring
-    // Rounded *up* to guarantee containment
-    return (int)ceil(bboxRadiusKm / (1.5 * centerHexRadiusKm));
+    // We use centerHexRadiusKm un-scaled and rounded *up* to guarantee
+    // containment ot the bbox.  Ideal, undistorted hexagons could scale
+    // centerHexRadiusKm by a factor of up to 1.5, reducing bboxHexRadius.
+    // This is because the closest point along an undistorted hexagon drawn
+    // through the center points of a k-ring aggregation is exactly 1.5 radii
+    // of the hexagon.  But there is distortion near pentagons, and for those
+    // cases, the scaling needs to be less than 1.5.  Using the un-scaled value
+    // conservatively guarantees containment for all cases, at the expense of a
+    // larger bboxHexRadius.
+    return (int)ceil(bboxRadiusKm / centerHexRadiusKm);
 }