From b2463b66162501e3c006e536de52dfd4496ec796 Mon Sep 17 00:00:00 2001 From: Nick Rabinowitz Date: Mon, 12 Feb 2024 14:22:27 -0800 Subject: [PATCH] Add fix for false positive on bad polygon point, plus more tests --- src/apps/testapps/testPolyfillInternal.c | 26 -- .../testapps/testPolygonToCellsExperimental.c | 252 +++++++++++++++--- src/h3lib/lib/polyfill.c | 75 +++--- 3 files changed, 259 insertions(+), 94 deletions(-) diff --git a/src/apps/testapps/testPolyfillInternal.c b/src/apps/testapps/testPolyfillInternal.c index 7efeebf79..915378f8c 100644 --- a/src/apps/testapps/testPolyfillInternal.c +++ b/src/apps/testapps/testPolyfillInternal.c @@ -36,14 +36,6 @@ static GeoPolygon sfGeoPolygon = { {0.6599990002976, -2.1376771158464}}}, .numHoles = 0}; -static GeoPolygon invalidGeoPolygon = { - .geoloop = {.numVerts = 4, - .verts = (LatLng[]){{NAN, -2.1364398519396}, - {0.6595011102219, NAN}, - {NAN, -2.1354884206045}, - {0.6581220034068, NAN}}}, - .numHoles = 0}; - SUITE(polyfillInternal) { TEST(iterInitPolygonCompact_errors) { IterCellsPolygonCompact iter; @@ -142,24 +134,6 @@ SUITE(polyfillInternal) { t_assert(iter.cell == H3_NULL, "Got null output for invalid cell"); } - TEST(iterStepPolygonCompact_invalidPolygonErrors) { - IterCellsPolygonCompact iter; - - // Start with a good polygon, otherwise we error out early - iter = - iterInitPolygonCompact(&sfGeoPolygon, 5, CONTAINMENT_OVERLAPPING); - t_assertSuccess(iter.error); - - // Give the iterator a bad polygon and a cell at target res - iter._polygon = &invalidGeoPolygon; - iter.cell = 0x85283473fffffff; - - iterStepPolygonCompact(&iter); - t_assert(iter.error == E_LATLNG_DOMAIN, - "Got expected error for invalid polygon"); - t_assert(iter.cell == H3_NULL, "Got null output for invalid cell"); - } - TEST(iterDestroyPolygonCompact) { IterCellsPolygonCompact iter = iterInitPolygonCompact(&sfGeoPolygon, 9, CONTAINMENT_CENTER); diff --git a/src/apps/testapps/testPolygonToCellsExperimental.c b/src/apps/testapps/testPolygonToCellsExperimental.c index 654de0c4f..d36938c34 100644 --- a/src/apps/testapps/testPolygonToCellsExperimental.c +++ b/src/apps/testapps/testPolygonToCellsExperimental.c @@ -50,6 +50,11 @@ static LatLng invalidVerts[] = {{INFINITY, INFINITY}, {-INFINITY, -INFINITY}}; static GeoLoop invalidGeoLoop = {.numVerts = 2, .verts = invalidVerts}; static GeoPolygon invalidGeoPolygon; +static LatLng outOfBoundsVert[] = {{-2000, -2000}}; +static GeoLoop outOfBoundsVertGeoLoop = {.numVerts = 1, + .verts = outOfBoundsVert}; +static GeoPolygon outOfBoundsVertGeoPolygon; + static LatLng invalid2Verts[] = {{NAN, NAN}, {-NAN, -NAN}}; static GeoLoop invalid2GeoLoop = {.numVerts = 2, .verts = invalid2Verts}; static GeoPolygon invalid2GeoPolygon; @@ -173,6 +178,9 @@ SUITE(polygonToCells) { invalid2GeoPolygon.geoloop = invalid2GeoLoop; invalid2GeoPolygon.numHoles = 0; + outOfBoundsVertGeoPolygon.geoloop = outOfBoundsVertGeoLoop; + outOfBoundsVertGeoPolygon.numHoles = 0; + nullGeoPolygon.geoloop = nullGeoLoop; nullGeoPolygon.numHoles = 0; @@ -226,6 +234,21 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCells_OverlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &sfGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &sfGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 1416, + "got expected polygonToCells size (overlapping bbox mode)"); + free(hexagons); + } + TEST(polygonToCellsHole) { int64_t numHexagons; t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( @@ -272,6 +295,22 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsHoleOverlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &holeGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &holeGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + t_assert( + actualNumIndexes == 1403, + "got expected polygonToCells size (hole, overlapping bbox mode)"); + free(hexagons); + } + TEST(polygonToCellsHoleParentIssue) { // This checks a specific issue where the bounding box of the parent // cell fully contains the hole. @@ -398,6 +437,22 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsContainsPolygon_OverlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &sfGeoPolygon, 4, CONTAINMENT_OVERLAPPING_BBOX, &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &sfGeoPolygon, 4, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 5, + "got expected polygonToCells size (overlapping bbox mode)"); + t_assert(hexagons[0] == 0x8428309ffffffff, "got expected hexagon"); + free(hexagons); + } + TEST(polygonToCellsExact) { LatLng somewhere = {1, 2}; H3Index origin; @@ -440,6 +495,15 @@ SUITE(polygonToCells) { // TODO: CONTAINMENT_OVERLAPPING yields 7 cells, presumably due to FPE // in the various cell boundaries + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &someHexagon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + // Overlapping BBox is very rough, so we get a couple of overlaps from + // non-neighboring cells + t_assert(actualNumIndexes == 9, + "got expected polygonToCells size for overlapping bbox " + "containment"); + free(hexagons); free(verts); } @@ -643,48 +707,105 @@ SUITE(polygonToCells) { } TEST(polygonToCellsPointPolygon) { - int64_t numHexagons; - t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_CENTER, &numHexagons)); - t_assert(numHexagons == 1, "got expected estimated size"); - H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); - - t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_CENTER, hexagons)); - int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); - - t_assert(actualNumIndexes == 0, "got expected polygonToCells size"); - free(hexagons); + for (int res = 0; res < MAX_H3_RES; res++) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &pointGeoPolygon, res, CONTAINMENT_CENTER, &numHexagons)); + t_assert(numHexagons >= 1 && numHexagons <= 5, + "got expected estimated size"); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &pointGeoPolygon, res, CONTAINMENT_CENTER, hexagons)); + int64_t actualNumIndexes = + countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 0, "got expected polygonToCells size"); + free(hexagons); + } } TEST(polygonToCellsPointPolygon_full) { - int64_t numHexagons; - t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_FULL, &numHexagons)); - t_assert(numHexagons == 1, "got expected estimated size"); - H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + for (int res = 0; res < MAX_H3_RES; res++) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &pointGeoPolygon, res, CONTAINMENT_FULL, &numHexagons)); + t_assert(numHexagons >= 1 && numHexagons <= 5, + "got expected estimated size"); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &pointGeoPolygon, res, CONTAINMENT_FULL, hexagons)); + int64_t actualNumIndexes = + countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 0, "got expected polygonToCells size"); + free(hexagons); + } + } - t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_FULL, hexagons)); - int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + TEST(polygonToCellsPointPolygon_overlapping) { + for (int res = 0; res < MAX_H3_RES; res++) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &pointGeoPolygon, res, CONTAINMENT_OVERLAPPING, &numHexagons)); + t_assert(numHexagons >= 1 && numHexagons <= 5, + "got expected estimated size"); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &pointGeoPolygon, res, CONTAINMENT_OVERLAPPING, hexagons)); + int64_t actualNumIndexes = + countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 1, "got expected polygonToCells size"); + free(hexagons); + } + } - t_assert(actualNumIndexes == 0, "got expected polygonToCells size"); - free(hexagons); + TEST(polygonToCellsPointPolygon_overlappingBBox) { + for (int res = 0; res < MAX_H3_RES; res++) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &pointGeoPolygon, res, CONTAINMENT_OVERLAPPING_BBOX, + &numHexagons)); + t_assert(numHexagons >= 1 && numHexagons <= 5, + "got expected estimated size"); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &pointGeoPolygon, res, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = + countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes >= 1 && actualNumIndexes <= 5, + "got expected polygonToCells size"); + free(hexagons); + } } - TEST(polygonToCellsPointPolygon_overlapping) { - int64_t numHexagons; - t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_OVERLAPPING, &numHexagons)); - t_assert(numHexagons == 1, "got expected estimated size"); - H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + TEST(polygonToCellsOutOfBoundsPolygon) { + for (int res = 0; res < MAX_H3_RES; res++) { + for (uint32_t flags = 0; flags < CONTAINMENT_INVALID; flags++) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &outOfBoundsVertGeoPolygon, res, flags, &numHexagons)); + t_assert(numHexagons == 0, "got expected estimated size"); + // Note: We're allocating more memory than the estimate to test + // for out-of-bounds writes here + H3Index *hexagons = calloc(10, sizeof(H3Index)); - t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( - &pointGeoPolygon, 9, CONTAINMENT_OVERLAPPING, hexagons)); - int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &outOfBoundsVertGeoPolygon, res, flags, hexagons)); + int64_t actualNumIndexes = + countNonNullIndexes(hexagons, numHexagons); - t_assert(actualNumIndexes == 1, "got expected polygonToCells size"); - free(hexagons); + h3Println(hexagons[0]); + t_assert(actualNumIndexes == 0, + "got expected polygonToCells size"); + free(hexagons); + } + } } TEST(polygonToCellsLinePolygon) { @@ -729,6 +850,20 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsLinePolygon_overlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &lineGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &lineGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + t_assert(actualNumIndexes == 21, "got expected polygonToCells size"); + free(hexagons); + } + TEST(polygonToCellsNullHole) { int64_t numHexagons; t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( @@ -777,6 +912,23 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsNullHole_overlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &nullHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, + &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &nullHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + // Same as without the hole + t_assert(actualNumIndexes == 1416, + "got expected polygonToCells size (null hole)"); + free(hexagons); + } + TEST(polygonToCellsPointHole) { int64_t numHexagons; t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( @@ -825,6 +977,23 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsPointHole_overlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &pointHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, + &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &pointHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + // Same as without the hole + t_assert(actualNumIndexes == 1416, + "got expected polygonToCells size (point hole)"); + free(hexagons); + } + TEST(polygonToCellsLineHole) { int64_t numHexagons; t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( @@ -873,6 +1042,23 @@ SUITE(polygonToCells) { free(hexagons); } + TEST(polygonToCellsLineHole_overlappingBBox) { + int64_t numHexagons; + t_assertSuccess(H3_EXPORT(maxPolygonToCellsSizeExperimental)( + &lineHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, + &numHexagons)); + H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); + + t_assertSuccess(H3_EXPORT(polygonToCellsExperimental)( + &lineHoleGeoPolygon, 9, CONTAINMENT_OVERLAPPING_BBOX, hexagons)); + int64_t actualNumIndexes = countNonNullIndexes(hexagons, numHexagons); + + // Same as without the hole + t_assert(actualNumIndexes == 1416, + "got expected polygonToCells size (line hole)"); + free(hexagons); + } + TEST(invalidFlags) { int64_t numHexagons; for (uint32_t flags = CONTAINMENT_INVALID; flags <= 32; flags++) { diff --git a/src/h3lib/lib/polyfill.c b/src/h3lib/lib/polyfill.c index 7b771f22d..a81339094 100644 --- a/src/h3lib/lib/polyfill.c +++ b/src/h3lib/lib/polyfill.c @@ -248,7 +248,7 @@ H3Error cellToBBox(H3Index cell, BBox *out, bool coverChildren) { out->north = M_PI_2; } - // Cell that contains the north pole + // Cell that contains the south pole if (cell == SOUTH_POLE_CELLS[res]) { out->south = -M_PI_2; } @@ -446,24 +446,6 @@ void iterStepPolygonCompact(IterCellsPolygonCompact *iter) { return; } } - if (mode == CONTAINMENT_OVERLAPPING) { - // For overlapping, we need to do a quick check to determine - // whether the polygon is wholly contained by the cell. We check - // the first polygon vertex, which if it is contained could also - // mean we simply intersect. - H3Index polygonCell; - H3Error polygonCellErr = H3_EXPORT(latLngToCell)( - &(iter->_polygon->geoloop.verts[0]), cellRes, &polygonCell); - if (polygonCellErr != E_SUCCESS) { - iterErrorPolygonCompact(iter, polygonCellErr); - return; - } - if (polygonCell == cell) { - // Set to next output - iter->cell = cell; - return; - } - } if (mode == CONTAINMENT_FULL || mode == CONTAINMENT_OVERLAPPING) { CellBoundary boundary; H3Error boundaryErr = @@ -487,16 +469,44 @@ void iterStepPolygonCompact(IterCellsPolygonCompact *iter) { // Set to next output iter->cell = cell; return; - } - // For overlap, we've already checked for center point inclusion - // above; if that failed, we only need to check for line - // intersection - else if (mode == CONTAINMENT_OVERLAPPING && - cellBoundaryCrossesPolygon( - iter->_polygon, iter->_bboxes, &boundary, &bbox)) { - // Set to next output - iter->cell = cell; - return; + } else if (mode == CONTAINMENT_OVERLAPPING) { + // For overlapping, we need to do a quick check to determine + // whether the polygon is wholly contained by the cell. We + // check the first polygon vertex, which if it is contained + // could also mean we simply intersect. + + // Deferencing verts[0] is safe because we check numVerts + // above + LatLng firstVertex = iter->_polygon->geoloop.verts[0]; + + // We have to check the bounding box first, because + // out-of-bounds values will yield false positives with + // latLngToCell + if (bboxContains(&bbox, &firstVertex)) { + H3Index polygonCell; + H3Error polygonCellErr = H3_EXPORT(latLngToCell)( + &firstVertex, cellRes, &polygonCell); + if (NEVER(polygonCellErr != E_SUCCESS)) { + // This should be unreachable after the bboxContains + // check + iterErrorPolygonCompact(iter, polygonCellErr); + return; + } + if (polygonCell == cell) { + // Set to next output + iter->cell = cell; + return; + } + } + // We've already checked for center point inclusion above; + // if that failed, we only need to check for line + // intersection + if (cellBoundaryCrossesPolygon( + iter->_polygon, iter->_bboxes, &boundary, &bbox)) { + // Set to next output + iter->cell = cell; + return; + } } } if (mode == CONTAINMENT_OVERLAPPING_BBOX) { @@ -711,11 +721,6 @@ H3Error H3_EXPORT(maxPolygonToCellsSizeExperimental)(const GeoPolygon *polygon, *out = 0; return E_SUCCESS; } - // Special case: 1-vertex polygon - if (polygon->geoloop.numVerts == 1) { - *out = 1; - return E_SUCCESS; - } // Initialize the iterator without stepping, so we can adjust the res and // flags (after they are validated by the initialization) before we start @@ -757,4 +762,4 @@ H3Error H3_EXPORT(maxPolygonToCellsSizeExperimental)(const GeoPolygon *polygon, } return iter.error; -} \ No newline at end of file +}