Vec3d indexing refactor by ajfriend · Pull Request #1145 · uber/h3

ajfriend · 2026-04-01T18:44:11Z

Summary

Vec3d refactor

Based on @holoskii 's work in #1052, we cherry pick out just the Vec3d indexing refactor.

The indexing path on master currently operates on "geo" lat/lng coordinates:

latLngToCell (old, on master)
├── _geoToFaceIjk → FaceIJK
│   ├── _geoToHex2d → face + Vec2d
│   │   ├── _geoToClosestFace (uses Vec3d internally, but discards it)
│   │   ├── _geoAzimuthRads
│   │   ├── gnomonic projection
│   │   └── polar → cartesian
│   └── _hex2dToCoordIJK → CoordIJK
└── _faceIjkToH3 → H3Index

The new path in this PR converts from lat/lng to Vec3d and does more operations in that representation:

vec3ToCell (new)
├── _vec3ToFaceIjk → FaceIJK
│   ├── _vec3ToHex2d → face + Vec2d
│   │   ├── _vec3ToClosestFace
│   │   ├── _vec3AzimuthRads
│   │   ├── gnomonic projection
│   │   └── polar → cartesian
│   └── _hex2dToCoordIJK → CoordIJK
└── _faceIjkToH3 → H3Index

latLngToCell (new, thin wrapper)
├── latLngToVec3
└── vec3ToCell

Additional changes

In addition to what was done in #1052, this PR makes everything go through the new Vec3d logic, for a complete migration:

latLngToCell
cellToLatLng
cellToBoundary
directedEdgeToBoundary
vertexToLatLng

Moving everything let us delete all the "to geo" and "geo to" code, e.g., _faceIjkToGeo.

Also:

Making a header-only vec3d.h provided a nice win: latLngToCell went from ~11% slower to ~3% faster than master
100% line and branch coverage on change

Cell assignment tests

Compares this PR to what's on master.

Code in src/apps/benchmarks/dumpCoreApi.c and stress.py (will be removed before landing). Run with uv run stress.py

Note: These are on top of the existing tests, bc*centers.txt and rand*centers.txt in tests/inputfiles/.

`latLngToCell`: random points

10,000 random points (uniform on sphere, fixed seed) tested at 8 resolutions (0, 1, 2, 3, 4, 7, 10, 15) = 80,000 tests.

Result: 0 mismatches

`latLngToCell`: boundary points

All cell boundary vertices and edge midpoints at resolutions 0–3, tested at 8 resolutions = 4,654,080 tests.

Result: ~12% mismatches. But this is expected, since we arbitrarily break ties between cells, and slightly different floating point math may break ties differently.

`cellToLatLng` and `cellToBoundary`

All cells at resolutions 0--4.

Result: all agree to within ~0.001 mm

Benchmarks

Code in src/apps/benchmarks/benchmarkCoreApi.c and bench.py. Run with uv run bench.py. (Will be removed before landing.)

Function	master	vec3-core	Change
`latLngToCell`	0.198 µs	0.192 µs	-3.4%
`cellToLatLng`	0.132 µs	0.109 µs	-17.6%
`cellToBoundary`	0.590 µs	0.443 µs	-25.0%
`directedEdgeToBoundary`	0.303 µs	0.258 µs	-14.7%
`vertexToLatLng`	0.172 µs	0.149 µs	-13.7%

Note that one of the bottlenecks of the new cellsToMultiPolygon code is directedEdgeToBoundary, so speedups here will help that function as well!

Follow-ups

Renaming

To help me understand the code, I had renamed a bunch of things, including using Vec2 and Vec3 consistently ("hex2d" and "vec2d" were used interchangeably, and function names didn't always match struct names). While that helped in working on the PR, it made this diff hard to read. I'll revisit in the future, since I think better naming would be nice if we continue to work on this code.

Header-only allows for helper function optimizations

Compiler optimizations across translation units makes a difference. By moving vec3d.h to be header-only and static inline, the latLngToCell benchmarks go from +11% (slower) to -3.4% (faster), compared to master. We can do the same elsewhere: coordijk.c has lots of small functions that get called in other .c files (translation units). I tested moving coordijk.c to be header-only, and got latLngToCell to be -9.7% (faster) compared to master, and cellToLatLng and cellToBoundary were both -33% (faster). We should do this in a follow-up PR. And there might be other files worth considering, like vec2d.h.

Trig-free rotation math

The rotation math in _vec3ToVec2 translates from 3d to angle and back to 3d. That rotation can be done without any trig functions. Instead of storing angles in faceAxesAzRadsCII (and note that we store all three, but only use the first!), we could instead store the Vec3 i-axis for each icosahedron face. This removes 5 trig function calls per indexing operation. Bigger change, separate PR.

Note, we can also remove some trig from this path in _vec3ToHex2d:

double r = acos(1 - sqd * 0.5);
r = tan(r);

by writing

double r = sqrt(sqd * (1.0 - sqd * 0.25)) / (1.0 - sqd * 0.5);

I tried this, but didn't see much movement in the benchmarks.

…oVec3 From uber#1052.

Extract _vec3dToClosestFace, _vec3dToHex2d, _vec3dToFaceIjk from LatLng wrappers. Add _vec3dAzimuthRads (3D tangent-plane azimuth) and _hex2dToVec3 (inverse via Rodrigues' rotation). Add vec3ToCell and cellToVec3 public API. Existing LatLng API is unchanged — wrappers delegate to the new Vec3d functions. From uber#1052.

From uber#1052.

…y code

…ads, and tests

coveralls · 2026-04-01T18:49:31Z

coverage: 99.161% (+0.1%) from 99.066% — ajfriend:vec3d-core into uber:master

ajfriend · 2026-04-01T19:15:39Z

src/apps/filters/h3.c

    if (err) {
        return err;
    }
-    printf("%.10lf\n", length);


Slightly different floating point math on Mac vs Linux. Reducing to 8 digits gets tests to pass on both platforms. (We do an exact string comparison in edgeLengthM.txt)

src/h3lib/lib/faceijk.c

holoskii

Left some minor comments, nothing blocking. LGTM

holoskii · 2026-04-07T09:40:51Z

src/h3lib/lib/h3Index.c

+ * @param out The encoded H3Index.
+ * @returns E_SUCCESS on success, another value otherwise
+ */
+H3Error vec3ToCell(const Vec3d *v, int res, H3Index *out) {


Should the function also check that the input vector is on the unit sphere?

holoskii · 2026-04-07T09:41:23Z

src/h3lib/include/vec3d.h

+
+static inline void vec3Normalize(Vec3d *v) {
+    double norm = vec3Norm(*v);
+    if (norm == 0.0) return;


Do we intentially keep == 0 instead < EPSILON? If so - could leave a comment explaining why

+1, except should be DBL_EPSILON from <float.h>

This is subtle, so I'm glad you're raising it, but I do think an exact zero check is correct here. And this also made me realize there's a slightly better version of this function!

A check like if (norm < DBL_EPSILON) return; would have what I'd consider undesirable behavior in that it would fail to normalize totally normalizable vectors. For example, if we were to try to normalize Vec3d v = {DBL_EPSILON / 2.0, 0, 0};, it would pass through the function unchanged (even though it should normalize to {1,0,0}), which I could imagine leading to confusing bugs.

In addition, there are vectors which are nonzero, but have norm zero, like v = {1e-163, 0, 0}. For those, we could reassign them to be exactly zero, since I could imagine someone assuming that to be the case after seeing vec3Norm(v) == 0. I don't think this case is common in real scenarios, but it's basically free and seems safer.

I'm updating the function with that slightly adjusted logic and some comments. I've also added tests for these examples. See 5cffa92

Happy to discuss more, since this is worth getting correct. Let me know what you think.

holoskii · 2026-04-07T09:46:43Z

src/apps/miscapps/generateFaceCenterPoint.c

    printf("};\n");
 }

 int main(int argc, char *argv[]) {


Do we still need this file?

I think we still want this to be able to generate the tables that we have hard coded. See #67

@isaacbrodsky?

holoskii · 2026-04-07T09:48:41Z

src/apps/testapps/testVec3.c

+        Vec3d infZ = {.x = 0.0, .y = 0.0, .z = -INFINITY};
+        t_assert(vec3ToCell(&infZ, 0, &out) == E_DOMAIN,
+                 "infinite z is rejected");
+    }


Probably should add a test for cellToVec3 here as well?

holoskii · 2026-04-07T09:49:20Z

src/h3lib/lib/faceijk.c

+ * @param v3 Output: the 3D coordinates of the cell center point
 */
-void _hex2dToGeo(const Vec2d *v, int face, int res, int substrate, LatLng *g) {
+void _hex2dToVec3(const Vec2d *v, int face, int res, int substrate, Vec3d *v3) {


Could be made static

True. I was just generally trying to avoid any avoidable code changes to make the diff easier to review. But making this static should be fine.

holoskii · 2026-04-07T09:50:07Z

src/h3lib/lib/faceijk.c

+ * @param v3 Output: The 3D coordinates of the cell center point.
 */
-void _faceIjkToGeo(const FaceIJK *h, int res, LatLng *g) {
+void _faceIjkToVec3(const FaceIJK *h, int res, Vec3d *g) {


Argument name mismatch:
void _faceIjkToVec3(const FaceIJK *h, int res, Vec3d *v3);

+1, should be fixed before landing

Fixed: 04275a2

dmitryzv

Approved with a comment

dmitryzv · 2026-04-08T15:40:38Z

src/h3lib/include/vec3d.h

+
+static inline void vec3Normalize(Vec3d *v) {
+    double norm = vec3Norm(*v);
+    if (norm == 0.0) return;


+1, except should be DBL_EPSILON from <float.h>

holoskii and others added 30 commits March 27, 2026 11:17

Add Vec3d math functions: dot, cross, normalize, mag, distSq, latLngT…

432cf47

…oVec3 From uber#1052.

Add Vec3d unit tests

fe7b36a

From uber#1052.

Route latLngToCell/cellToLatLng through Vec3d; remove dead LatLng-onl…

4eefbfa

…y code

cell boundary code to Vec3d path; remove _hex2dToGeo, _geoAzDistanceR…

4aa3b63

…ads, and tests

vec3ToCell validation tests

0c5dcba

align function names and struct: hex2d -> vec2d

6c3eedf

use Vec2 and Vec3 everywhere

2f159ed

normalize

4021953

h3 -> cell; years

2a55747

bench

35b3302

update bench

f7e9e44

correctness

2f5a9f4

test vertices and along edges

b310c51

_vec3TangentBasis helper

f1f1938

vec3LinComb helper

e2d586e

return by value

3f21332

pass by value

8709618

clean

1a50e33

test for inlining

2e36bd2

header-only; suffering on LTO

12c8f5d

header only

9916417

clean up benchmark script

ad41cde

_vec3ToFaceIjk

b770a1c

output params

7977187

header version of _ijkToVec2

3f7dd6e

revert

e7278d9

revert

7e9d540

redo

6670909

inline ijk ops

0040fd6

ajfriend added 9 commits March 31, 2026 22:49

nits

2d48ca0

boop

b2232b6

i swear...

fd2a985

boop

7d568f0

9 digits

a2ce573

move

218c973

8 digits

e63e313

comments align

4dc91f7

better names for vec3LinComb

bd5c9a5

ajfriend added 2 commits April 1, 2026 12:01

dead code: remove faceCenterGeo definition

9fcb5aa

remove benchmarks and justfile

a06d168

ajfriend requested review from dfellis, dmitryzv, isaacbrodsky and nrabinowitz April 1, 2026 19:09

ajfriend commented Apr 1, 2026

View reviewed changes

This was referenced Apr 1, 2026

Add geodesic polygon coverage mode for polygonToCellsExperimental #1052

Open

[wip] Vec3 core ajfriend/h3#4

Closed

ajfriend changed the title ~~vec3d core~~ Vec3d indexing refactor Apr 2, 2026

ajfriend mentioned this pull request Apr 2, 2026

Header-only coordijk.h #1148

Open

justinhwang reviewed Apr 6, 2026

View reviewed changes

src/h3lib/lib/faceijk.c Outdated Show resolved Hide resolved

justinhwang approved these changes Apr 6, 2026

View reviewed changes

ajfriend added 2 commits April 6, 2026 11:37

camel_case

1aa5d4a

fix more snakeCase

c739284

holoskii approved these changes Apr 7, 2026

View reviewed changes

dmitryzv approved these changes Apr 8, 2026

View reviewed changes

ajfriend added 3 commits April 8, 2026 23:56

small norm handling

5cffa92

static void _hex2dToVec3

81ae468

_faceIjkToVec3 g

04275a2

Conversation

ajfriend commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Vec3d refactor

Additional changes

Cell assignment tests

latLngToCell: random points

latLngToCell: boundary points

cellToLatLng and cellToBoundary

Benchmarks

Follow-ups

Renaming

Header-only allows for helper function optimizations

Trig-free rotation math

Uh oh!

coveralls commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

holoskii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajfriend Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajfriend Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitryzv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ajfriend commented Apr 1, 2026 •

edited

Loading

`latLngToCell`: random points

`latLngToCell`: boundary points

`cellToLatLng` and `cellToBoundary`

coveralls commented Apr 1, 2026 •

edited

Loading

ajfriend Apr 9, 2026 •

edited

Loading

ajfriend Apr 9, 2026 •

edited

Loading