Document distance_to_segment limits; fix epsilon doc and test helper#26
Open
gistrec wants to merge 2 commits into
Open
Document distance_to_segment limits; fix epsilon doc and test helper#26gistrec wants to merge 2 commits into
gistrec wants to merge 2 commits into
Conversation
- kDefaultEpsilon was documented as "0.1 nanometers" on Earth in latlng.hpp and api.md; 1e-12 degrees is actually ~111 nm (~0.1 um), a 1000x understatement. - distance_to_segment projects in raw (lat, lng) space: no cos(lat) scaling (overshoots a few percent at high latitudes) and no antimeridian handling (a point on a segment crossing +-180 can report kilometers). Document the approximation in poly.hpp and api.md and point tolerance checks at on_path. - EXPECT_NEAR_LatLng wrapped each longitude independently, so pairs straddling +-180 (179.9999999 vs -179.9999999) compared ~360 deg apart and failed despite being the same meridian within tolerance. Compare the wrapped difference instead, print raw longitudes on failure, and fix the comment: wrap() yields [-180, 180), not (-180, 180].
The only caller passes sin_dist13 * sin_bearing, where sin_bearing is the quotient (a*d - b*c)/sqrt((a^2+b^2)*(c^2+d^2)). Mathematically it is <= 1 by Cauchy-Schwarz, but rounding can push it 1 ulp above 1; the resulting sqrt(negative) NaN propagates through is_on_segment_gc where every NaN comparison is false, silently turning on_path/on_edge into a false positive for segments under ~2.1 rad of arc. Clamp x^2 to 1, matching the existing arc_hav clamp. Measured cost: +0.2 ns/call on the bare function, ~+3% on a worst-case geodesic on_path scan.
This was referenced Jun 10, 2026
MrHerrn
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four follow-ups from the June code review — no library behavior changes (item 4 only changes behavior on inputs that previously produced NaN).
1.
kDefaultEpsilondoc was off by 1000×.1e-12degrees is ~111 nm ≈ 0.1 µm on Earth, not "0.1 nanometers". Fixed inlatlng.hppanddocs/api.md.2.
distance_to_segmentapproximation documented. The closest point is found by planar projection in raw(lat, lng)coordinate space (same algorithm as android-maps-utilsPolyUtil.distanceToLine), then the great-circle distance to it is returned:cos(lat), so the projection skews at high latitudes — the result can overshoot by a few percent aroundlat 80°;±180°) is treated as spanning nearly the whole globe — a point lying exactly on such a segment reported ~11 km in the review probe.Both
poly.hppanddocs/api.mdnow state this and point tolerance checks aton_path(which handles geodesic segments correctly).3.
EXPECT_NEAR_LatLngantimeridian fragility. The test helper wrapped each longitude independently, so a pair like179.9999999vs-179.9999999— the same meridian within tolerance — compared as ~360° apart and failed. It now compares the wrapped differencewrap(expected.lng - actual.lng, -180, 180)and prints both raw longitudes on failure. Also fixes the stale comment:wrap()yields[-180, 180), not(-180, 180].4.
hav_from_sinNaN guard. Its only caller (is_on_segment_gc) passessin_dist13 * sin_bearing, wheresin_bearingis the quotient(a·d − b·c)/√((a²+b²)(c²+d²))— mathematically ≤ 1 by Cauchy–Schwarz, but rounding can push it 1 ulp above 1. Thensqrt(1 − x²)of a negative argument returns NaN, every subsequent NaN comparison isfalse, and both rejection checks inis_on_segment_gcare silently skipped —on_path/on_edge(geodesic, their default mode) return a false positive for segments under ~2.1 rad of arc. Clampx²to 1 (correct limithav(90°) = 0.5), matching the existingarc_havclamp; regular values are unchanged bit-for-bit.Measured cost (Apple M1,
-O2, medians of per-pair deltas across 10 interleaved before/after runs):hav_from_sinin a tight loopon_pathgeodesic, worst case: 50-segment full scan, point off pathhav_from_sinhas no other callersTest plan
Math.hav_from_sin_clamp:±(1 + 1 ulp)must yield0.5, not NaN (fails before the fix)EXPECT_NEAR_LatLngchange is strictly more permissive only at the ±180 boundary and identical elsewhere, so existing assertions keep their strength)