Skip to content

Conversation

holoskii
Copy link

@holoskii holoskii commented Oct 5, 2025

Summary

Adds the geodesic traversal mode to polygonToCellsExperimental, letting us follow true great-circle edges when filling massive polygons (think continental footprints and polar caps). The PR builds the geodesic stack from the ground up—new vector math helpers, bounding caps, iterator wiring, and plenty of validation coverage—while keeping the default planar behavior unchanged (and safely rejecting geodesic flags on the legacy API).

Highlights

  • New geodesic mode

    • Introduces FLAG_GEODESIC_MASK (with helpers) so callers can opt into geodesic coverage; validates acceptable containment modes (FULL, OVERLAPPING) at both maxPolygonToCellsSizeExperimental and polygonToCellsExperimental.
    • Dispatches a dedicated geodesic iterator path that reuses the existing traversal logic but evaluates cells using great-circle geometry.
  • Core geometry plumbing

    • Adds GeodesicPolygon acceleration structures + helpers (geodesicPolygonCreate/Destroy, containment, boundary/cap intersection tests).
    • Extends the vector toolkit (latLngToVec3, cross/dot/normalize, distance helpers) and new conversions: vec3dToCell, cellToVec3, cellToGeodesicBoundary, cellToSphereCap.
    • Pulls bounding-cap tables into sphereCapTables.h for both prod code and tests; exposes SphereCap/AABB helpers.
  • Iterator & polyfill updates

    • Refactors polygon iterators into polyfill_iterator.h (with _extra context) and wires in geodesic_iterator.c.
    • Ensures geodesic flags short-circuit the legacy planar polygonToCells path.
  • Benchmarks, fuzzers, and tests

    • Benchmarks: geodesic variants for existing benchmark.
    • Fuzzers now exercise geodesic flag permutations.
    • Added new test suites: testGeodesicPolygonToCellsExperimental, testSphereCap, testVec3d, plus extended testBBoxInternal, testPolygonToCells(Experimental) flag validation, and memory-failure coverage.
    • Hooks new tests into CMakeLists.txt/CMakeTests.cmake.
  • Documentation

    • Updates the Regions API docs to explain the geodesic flag, trade-offs, and usage tips.

Attribution

This geodesic polyfill work originated at Yellowbrick Data (where I’m currently employed). We’re upstreaming it to the H3 project so the broader community can benefit and use it for coverage.

@CLAassistant
Copy link

CLAassistant commented Oct 5, 2025

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

coverage: 97.061% (-1.9%) from 98.934%
when pulling c9be803 on holoskii:geodesic_coverage
into 4c56ad1 on uber:master.

@ajfriend
Copy link
Collaborator

ajfriend commented Oct 9, 2025

Thanks @holoskii for the contribution! Geodesic polyfill is an exciting addition that I think a lot of folks would use, myself included!

Please bear with us as we try to find the time to properly review this PR. Some initial questions from my end:

  • Is it fair to characterize this PR as providing two major improvements:
    1. geodesic mode for "polyfill", for polygons that would otherwise already be well-handled in the existing planar mode in polygonToCellsExperimental, and
    2. the ability to do polyfill on polygons larger or tricker (e.g. around poles or the antimeridian) than those that could be handled in the previous algorithm (and, if so, would these improvements also generalize to the existing planar mode?)
  • This may become obvious after I'm able to review more closely, but how much code can we share between the planar and geodesic algorithms?
  • Is it necessary to expose Vec3d and GeodesicCellBoundary in the public api (h3api.h.in), or can we keep those structs internal?
  • You mentioned that this mode is slower than the existing algorithm. How much slower? Do you have any example benchmarks you could share?
  • Any thoughts on what to do about the failing CI checks? Or would you like some input from the team?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants