Conversation
Review on #260 found that polygons[0][0] drops more than holes. The inner index correctly ignores holes; the outer one silently discarded polygons[1..], which are disjoint PARTS of the same barrier. One part indexed and the other invisible is precisely the 'a barrier the index simply did not see' failure the multipolygon branch was added to remove, moved one level in. barrierLine becomes barrierLines and returns every outer ring, sharing one frame across the feature so all parts are expressed against the same anchor. A relation with two stitched outer rings covers it; reverting to first-part-only fails that test. The multilinestring branch is dropped as unreachable: toGeometry never produces one — only clip.ts does, and clipping is not in this path — so it was code no test could ever cover. The cell-coverage comment claimed to remove polygonToCells calls and did not. coverCells still runs once per ring, and batching cannot change that since coverCells runs addPolygon per ring internally. What went away is the existing.includes rescan, and the union making one-obstacle-per-cell structural. Narrowed to what it actually does — in a PR family whose subject is comments describing behaviour that does not exist.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @cs-util's task in 3m 6s —— View job Review
The core fix is correct.
Four things raised inline, none blocking:
I could not run No changes pushed — review only. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gps-plus-slam | 0474775 | Commit Preview URL Branch Preview URL |
Aug 06 2026, 02:53 AM |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const lines: readonly LatLng[][] = | ||
| geometry.kind === "linestring" | ||
| ? geometry.positions | ||
| ? [[...geometry.positions]] | ||
| : geometry.kind === "polygon" | ||
| ? geometry.rings[0] | ||
| ? [[...(geometry.rings[0] ?? [])]] | ||
| : geometry.kind === "multipolygon" | ||
| ? geometry.polygons[0]?.[0] | ||
| : geometry.kind === "multilinestring" | ||
| ? geometry.lines[0] | ||
| : undefined; | ||
| if (positions === undefined || positions.length < 2) return undefined; | ||
| ? geometry.polygons.map((polygon) => [...(polygon[0] ?? [])]) | ||
| : []; |
There was a problem hiding this comment.
Both ?? [] fallbacks are unreachable, which is the same category the comment three lines above deletes multilinestring for.
kind === "polygon"→wayToGeometrybuildsrings: [way.geometry](osm-geometry.ts:203) andrelationToGeometryreturnspolygons[0]!(osm-geometry.ts:253).rings[0]is always there.kind === "multipolygon"→groupRingsIntoPolygonsseeds every entry as[outer](multipolygon-builder.ts:379), sopolygon[0]is always there.
So ?? [] is a branch no test can cover and no mutant can be killed on — exactly the argument made for leaving multilinestring out. ! is what the rest of this file already uses for the same situation (line 144: lines[0]![0]!.y).
| const lines: readonly LatLng[][] = | |
| geometry.kind === "linestring" | |
| ? geometry.positions | |
| ? [[...geometry.positions]] | |
| : geometry.kind === "polygon" | |
| ? geometry.rings[0] | |
| ? [[...(geometry.rings[0] ?? [])]] | |
| : geometry.kind === "multipolygon" | |
| ? geometry.polygons[0]?.[0] | |
| : geometry.kind === "multilinestring" | |
| ? geometry.lines[0] | |
| : undefined; | |
| if (positions === undefined || positions.length < 2) return undefined; | |
| ? geometry.polygons.map((polygon) => [...(polygon[0] ?? [])]) | |
| : []; | |
| const lines: readonly (readonly LatLng[])[] = | |
| geometry.kind === "linestring" | |
| ? [geometry.positions] | |
| : geometry.kind === "polygon" | |
| ? [geometry.rings[0]!] | |
| : geometry.kind === "multipolygon" | |
| ? geometry.polygons.map((polygon) => polygon[0]!) | |
| : []; |
Widening the annotation to readonly (readonly LatLng[])[] also drops the three spreads — those copy every vertex of every ring purely to satisfy mutability variance, and nothing here mutates them.
| it("indexes EVERY part of a multipolygon barrier, not just the first", () => { | ||
| // RAISED IN REVIEW ON #260. The multipolygon branch took `polygons[0][0]`, | ||
| // where the inner index correctly ignores holes but the OUTER one silently |
There was a problem hiding this comment.
This pins the two-part case, but the one-part relation is still untested — and it is the more common shape of the two.
relationToGeometry returns kind: "polygon" when the outer rings stitch to a single ring, and only falls through to kind: "multipolygon" for ≥2 disjoint outers (osm-geometry.ts:252-255). So an ordinary type=multipolygon + barrier=wall relation with one outer ring lands on the polygon branch, which this PR also rewrote (geometry.rings[0] → [[...(geometry.rings[0] ?? [])]]) and which no fixture in this file reaches — closed barrier=wall ways don't get there either, since osmtogeojson blacklists barrier=wall in POLYGON_FEATURES, so isAreaWay sends them to linestring.
Given the mutation-testing bar this file already holds itself to (the "EVERY segment of a bent barrier" test above), a single-outer-ring relation fixture would be a cheap addition next to the one you just wrote — same relation shape, one outer member.
| // OUTER RINGS ONLY, and ALL of them: a barrier's holes are not something to | ||
| // walk through, but its parts are all wall. |
There was a problem hiding this comment.
The "all outer rings" half is right; the hole rationale doesn't describe what the code does.
"a barrier's holes are not something to walk through" only makes sense if the polygon's interior were being treated as solid. It isn't: barrierFootprints treats the ring as a centreline and emits one thicknessM-wide quad per segment (barrier-shape.ts), so all that ever becomes solid is a ~0.5 m band along the ring itself. The interior is fully walkable whether or not the inner rings are read, so dropping them isn't what keeps holes closed.
Where it actually matters: an area-mapped barrier=city_wall is normally outer = outer face, inner = inner face, and the wall material is the band between them. This code puts a default-thickness band on the outer face and ignores the inner one. That behaviour predates this PR and is defensible, but since you're rewriting this comment anyway, saying what it does — "each outer ring is treated as a centreline; an area-mapped barrier is indexed as a wall along its outline, not as a filled region" — would be more durable than the current claim. Same for the sidecar bullet in obstacles.ts.md.
| // WHAT THIS REMOVES IS THE RESCAN, not the h3 calls — an earlier comment | ||
| // here claimed the latter and was wrong (#260). `coverCells` still runs | ||
| // once per ring, and batching cannot change that: `coverCells` on a | ||
| // multipolygon runs `addPolygon` per ring internally, so the per-quad cost |
There was a problem hiding this comment.
Minor: coverCells runs addPolygon once per polygon (outer + its holes), not once per ring — cell-coverage.ts:75-77. The conclusion holds, since in the batched alternative you're arguing against each quad would be its own polygon, but this comment is replacing one that was wrong about this same function, so it's worth being literal: "runs addPolygon per polygon, and each quad would be its own polygon".
1 participant
Acts on the review findings from #260.