fix(renderer,clash): bound the point-cloud cell map, drop a no-op quadratic dedup - #3041
Conversation
Two independent defects found while diagnosing a production
`RangeError: Map maximum size exceeded`. Neither is claimed to be that
crash's cause; both are provable on their own. V8 throws that RangeError
at exactly 2^24 = 16,777,216 entries, and `Set` shares the limit.
1. `PointCloudSpatialIndex` stores its voxel grid as a
`Map<number, number[]>` with one entry per occupied cell, and
`insertRange` can create a new cell for every point it indexes. The
only limb of its documented "memory safety valve" was
`DEFAULT_MAX_INDEXED_POINTS = 30_000_000` — above the engine ceiling.
On a sparse cloud (airborne LiDAR, a coarse site scan) roughly one
point falls in each 0.5 m cell, so occupied cells track indexed points
almost 1:1 and the Map would throw around 16.8M points, before the
valve could bind. Dense terrestrial scans put many points in one cell
and never approach it, which is why the gap survived.
The two limbs bound different resources and neither implies the other,
so both are enforced: the point cap still bounds retained position
memory, and a new `DEFAULT_MAX_INDEXED_CELLS` (2^24 - 2^20) bounds the
Map. It is set high enough to bind only where V8 would otherwise have
thrown, so no cloud that indexes fully today loses coverage, and a
caller-supplied budget is clamped below the ceiling regardless. Points
landing in an already-occupied cell cost no budget.
Whichever limb binds first closes the index; a chunk crossing it is
truncated to the prefix actually indexed, so the cap bounds memory
rather than just stopping bookkeeping. Truncation is no longer silent:
it is reported once via `console.warn` naming the limb and the counts,
and `capReason` / `cellCount` / `cellCapacity` expose it to callers.
2. `queryMeshCross` funnelled its candidate triangle pairs through a
`Set` keyed `${iA}|${iB}` — one key string and one Set entry per
emitted pair, O(triA * triB) worst case for a single element pair,
uncapped. Its own comment explained why it was unnecessary, and the
reasoning holds: `buildNode` splits a node's indices into disjoint,
covering halves and a leaf keeps exactly its own slice, so every
triangle lives in exactly one leaf; and `crossNode` reaches any node
pair by a single route, descending both sides together while both are
internal and only the internal side once the other is a leaf. Each
leaf pair is visited once, so each pair is emitted at most once.
Removed. 4096 * 4096 = 2^24, so two ~4k-triangle elements whose AABB
filter passes nearly everything sat exactly on the Set ceiling.
Verification: output of `queryMeshCross` is byte-identical with and
without the Set — same sha256 over the full ordered pair lists of 432
fixtures (182,610 pairs) sweeping leaf size, triangle count, epsilon and
topology. New tests pin the emitted list against brute force and check
the leaf partition directly; breaking either the partition or the
traversal's single-visit property makes them fail. The index tests drive
the cell cap via a parameterised limit rather than allocating 16.7M
entries, and observe the reported outcome.
renderer 1011 -> 1022 tests, clash 414 -> 424, all passing; tsc --noEmit
and the test typecheck clean on both packages.
|
Warning Review limit reached
Next review available in: 25 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
|
Flagging a claim I should have checked before opening this, now that #3040 has landed. You are assigned to #3028 and you have #3042 open on it. This PR references the same issue and I did not check either before starting. Under the rule that just merged, that makes it a competing PR on someone else's issue, and the fact that I got here first is explicitly not the tiebreak. Your call what happens to it — I am not going to push it forward on my own. The one thing that might make it worth keeping: the two PRs touch no file in common. #3028 names a Enumerating what this holds that #3042 does not, so nothing is lost if you close it:
Item 2 is independent of #3028 entirely and could be split out if you want the cap discussion to stay on your PR. Happy to close this, or to rebase whatever survives onto yours as a patch, or to leave it for you to cherry-pick. Say which and I will do it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Self-review pass. Sound — here is what it survived, and one unenforced claim to hand back. Shape 6 attacked directly. Probed V8 rather than reasoning about it: a The dedup removal is pinned on ordering, not just membership — which was the thing I most doubted. Mutants: partition break ( One claim nothing enforces
Both the changeset ("truncated to the prefix actually indexed, so the cap bounds retained memory rather than just stopping bookkeeping") and the test comment ("Only the accepted prefix may be retained") assert it, and nothing observes it — It is observable: mutate the source An observation, not a defect: |
… it indexes `insertRange` makes two memory claims, and neither was enforced. Replacing `truncated ? positions.slice(0, accepted * 3) : positions` with a bare `positions` left the suite at 42/42 green — `chunks` is private, and `pointCount`, `cellCount` and `getBounds` are all folded in at insert time and read identically whichever array the chunk ended up holding. So the changeset's "truncated to the prefix actually indexed, so the cap bounds retained memory rather than just stopping bookkeeping" and the test's "Only the accepted prefix may be retained" both rested on nothing. The one handle on which array a chunk kept is what `queryRay` reads back, so three tests write to the caller's buffers after the insert: - a cap-crossing chunk's positions must be a copy (source overwritten wholesale, the query still reports the original prefix); - its classifications must be a copy on the same branch, or a later write would change which points the LAS visibility mask hides; - a chunk that FITS must still be aliased, since copying every streamed chunk would double this index's retained bytes per point. Writing to the caller's array is what the class doc tells callers not to do. That is the instrument, not an endorsement — what is asserted is which storage the index kept — and the mechanism is specified rather than incidental: `slice` allocates a new buffer, the array passed in stays a live view over the caller's. Each test's own comment says so. Mutants, all run: bare `positions` -> test 1 red; bare `classifications` -> test 2 red; slice-always -> test 3 red. Production code unchanged. point-cloud-spatial-index.test.ts 42 -> 45 pass; package suite 1026 pass / 0 fail.
|
The unenforced claim is now enforced, pushed Verified the gap first, by running: baseline 42/42, and with Three tests added, each mutation-checked:
Both directions, checked before asserting rather than assumed: the current behaviour for a non-truncating insert genuinely is by-reference, so that is pinned too — meaning a copy-always "fix" would pass the first two and fail the third. On the judgement callThe test mutates the source array after The mechanism is specified rather than incidental: If you would rather not have a test that touches that path at all, the alternative is to weaken the changeset's claim to what is actually observable — but a claim nothing checks seemed the worse of the two. I also rewrote a misleading comment on the existing truncation test: it now says only the indexing is observed there, and points at the new suite for retention. Targeted file 42 → 45 pass / 0 fail; package suite 1026 pass / 0 fail after building |
2 participants
Two unbounded-collection defects found while diagnosing #3028. Neither is claimed to be that crash's cause — both stand on their own.
Premise verified rather than assumed: a plain int-keyed
Mapinsert loop throws at exactly 2²⁴ = 16,777,216.1. A memory safety valve set above the ceiling it protects
point-cloud-spatial-index.tskeys oneMapentry per occupied 0.5 m voxel, guarded byDEFAULT_MAX_INDEXED_POINTS = 30_000_000— above the 16.78MMapceiling.Measured against the unmodified source with a realistic sparse fixture (200 m × 250 m ground tile at 1 m spacing, the airborne-swath shape):
Worst-case points-per-cell is 1, so
cells === pointsand the valve can never bind on a sparse cloud. Dense terrestrial scans put many points per cell and never approach it — which is exactly why this survived.(A first probe reported 0.655 cells/point; that was the
CELL_KEY_BIASboundary clamp folding a 50 km span — a fixture artefact, not density. Re-laid out, it gives the honest 1.0.)Both limbs are now bounded, rather than lowering the point cap.
DEFAULT_MAX_INDEXED_POINTSstays 30M — it bounds retained position memory and binds on dense clouds, a job that was never broken. NewDEFAULT_MAX_INDEXED_CELLS = 2²⁴ − 2²⁰ = 15,728,640bounds the Map. Neither implies the other: a sparse cloud exhausts cells at a fraction of the point cap, a dense one exhausts points with a tiny grid.Sat 1M cells below the ceiling deliberately — anything materially lower strips picking from clouds that index fine today. The bound should bind only where V8 would otherwise have thrown.
And the truncation was silent.
isCappedexisted with no consumer anywhere in the repo — grepped; only tests referenced it. It now warns once, naming the limb and counts, and exposescapReason/cellCount/cellCapacity. A chunk crossing either cap is truncated to the prefix actually indexed, andgetBoundsfolds in only indexed points so it never advertises a regionqueryRaycannot reach.Two mutants survived and were fixed, not papered over. The clamp test asserted
pointCount === 1— observing nothing; it now asserts the effective budget plus the0/-5/NaN/Infinityfallbacks. The warn-once mutant survived because thecapWarnedflag was dead code (insertRangealready returns early once capped) — the flag is deleted rather than kept as untestable defensive state.2. A quadratic Set proven to buy nothing
mesh-bvh.tsdeduped emitted triangle pairs with a Set keyed`${triA}|${triB}`— O(triA × triB) for a single element pair, uncapped. Two ~4k-triangle elements whose AABB filter passes nearly everything sit exactly on the ceiling.The comment three lines above already argued it was unnecessary. That argument was checked, not trusted:
buildNodesplits intoslice(0, mid)/slice(mid)— disjoint, covering — so every triangle lives in exactly one leaf; andcrossNodereaches a node pair by exactly one route, advancing both sides only when both are internal.Then proven by running. A probe hashed the full ordered pair list across 432 fixtures — leafSize 1/2/3/4/8/16, eps 0/0.05/0.25/10, 1–64 triangles including lopsided 1-vs-64, overlapping/staircase/scattered — 182,610 pairs:
Byte-identical, content and order. Removed.
The committed tests prove rather than assert: 8 parameterised fixtures compare against an independent brute-force ground truth on exact count and exact set — so a double-visiting traversal shows as a count mismatch, which is precisely what a "fixing" Set would have hidden. One test walks the tree and checks the partition directly; one guards vacuity with multi-leaf trees on both sides (1600 real pairs, not one pair from two single-leaf trees where nothing could duplicate).
Mutants: breaking the partition (overlapping leaves) fails 10 of 11; making the traversal visit a pair twice fails 6 of 11.
On the bare
catch {}atuseClash.ts:918— not touchedHalf defensible. The fallback is right — it draws the AABB overlap box instead of the precise contact interface, so the user is not left with nothing. What is not defensible is that it emits nothing: a
RangeErrorfrom a real bug and a legitimately degenerate mesh are observably identical, and neither reaches telemetry. A singleconsole.warnwould fix diagnosability without widening the catch or changing the fallback. Worth a follow-up.packages/renderer1011 → 1022;packages/clash414 → 424.tsc --noEmitclean on both, typecheck-tests clean, lint 0 errors. Changesets for both packages.🤖 Generated with Claude Code