perf(core,viewer): stop rebuilding wall geometry every frame - #556
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6ac6ee3. Configure here.
Three separate hot paths were doing full-scene work per frame on a 1081-wall floor, adding up to 5.6 s of main-thread stalls per six scroll ticks: - findJunctions was O(J x N) over every wall end; a spatial-grid prefilter narrows it to real neighbours (level assembly 59.3 s -> 0.01 s, calculateLevelMiters 436 ms -> 10.8 ms, identical output) - miter data is now cached per level and keyed on the exact wall inputs, so draining the dirty queue no longer recomputes it - getLevelElevations was called inside the per-wall loop; memoised - slab support and the wall appearance key were both unstable, which retriggered opening cutouts on every frame for no reason Measured on scene e5f5822f8837, six wheel ticks in split view: long tasks 5652 ms -> 4322 ms -> 0 ms. Co-Authored-By: Claude <noreply@anthropic.com>
The cache lives at module scope, so it outlived the mount it was created for: every level ever visited kept its wall array reachable, and a remount or a second project in the same tab simply added more. Editor teardown already resets the other shared singletons; the cache now does the same from the system's unmount effect. Also records the rule in the systems wiki, since the same trap is waiting for the next module-level memo. Co-Authored-By: Claude <noreply@anthropic.com>
The grid prefilter visits the per-cell bucket before the oversized-wall fallback, so a wall spanning more than JUNCTION_GRID_MAX_CELLS_PER_WALL cells was appended after shorter walls it precedes in the input. Collinear walls overlapping a junction tie on angle, and the sort in calculateJunctionIntersections is stable, so that reordering picked the other wall's thickness for the miter: a 20 m facade with a collinear infill of a different thickness moved the spur's boundary by 0.32 m. Restore the input order before appending. Extract the level miter cache so it is reachable from a test — replacing sameMiterInputs with `return true` previously left the whole suite green, which meant the cache the PR is built around had no coverage at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1b54ec2 to
7acdcd4
Compare
The single-slot memo held a strong reference to the whole node record, so closing a project left its entire graph reachable until the next call. The adjacent wrapper in terrain-support.ts already uses a WeakMap for the same value; match it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Aymericr
left a comment
There was a problem hiding this comment.
This is the best-diagnosed performance PR this repo has received, and the win is real — I measured it rather than taking the numbers on trust. calculateLevelMiters on synthetic levels, main vs this branch:
| walls | main | branch | speedup |
|---|---|---|---|
| 100 | 1.37 ms | 0.26 ms | 5.3× |
| 500 | 22.10 ms | 1.61 ms | 13.7× |
| 1089 | 88.01 ms | 1.12 ms | 78× |
Repeated every frame while the progressive queue drains, that is the multi-second freeze in #492. The AABB argument is sound: a T-junction can only exist where the point lies on the segment, so it must lie in the wall's AABB, and the cell containing the point is always one the wall was indexed into. Bucketing loses nothing. Writing the dirty-tracking rule into wiki/architecture/systems.md is exactly where it belongs.
maintainerCanModify was on, so rather than send you round again I pushed four commits. Please review them — they are changes to your work, and I will drop any you disagree with.
1. Rebased onto main. One conflict, one hunk, in getSlabSupportForWall: main added a terrain-aware levelBase, a !slabMap early return and a supportOffset post-adjustment; you replaced the two inline .map()s with getSupportInputs. Both are wanted, so the resolution keeps main's guard and elevation handling and takes your cached inputs. CONFLICTING → MERGEABLE.
2. Fixed a geometry difference in the grid prefilter. The comment claims the result is "bit-identical to the naive pass". It is not:
for (const bucket of [cellCandidates, oversized]) { // ← cell bucket always first
A wall covering more than JUNCTION_GRID_MAX_CELLS_PER_WALL (64) cells goes to oversized, scanned after the per-cell bucket — so an oversized wall is appended after shorter walls that precede it in walls. That matters because collinear walls overlapping a junction tie on angle, and the sort in calculateJunctionIntersections is stable, so append order decides which wall's thickness the miter uses.
A 20 m diagonal facade at 0.6 thickness, a collinear 8 m infill at 0.15, a spur at (8,8):
main: connectedWalls = [spur, long, infill] spur.startLeft.y = 8.2743
branch: connectedWalls = [spur, infill, long] spur.startLeft.y = 7.9561
0.32 m of drift. The threshold is easy to hit: a 45° wall goes oversized past ~20 m of length (14 m extent = 81 cells), any horizontal wall past ~100 m. Over 4003 generated cases, 1503 diverged.
The fix buffers the matches and appends them in input order. After it, main and branch agree on all 4003 cases — full connectedWalls ordering, junctionData, and every wall's boundary quad. Two regression tests in wall-mitering.test.ts; deleting the sort makes the first one fail, so it is not a vacuous test. I also corrected the comment, since "identical" is now a claim the tests back.
In fairness, this is not you breaking correct geometry. I checked whether main is order-independent here, and it is not: the same three walls as [a,b,c] vs [b,a,c] give main 8.2743 vs 7.9561 — the same two values. So main already returns an answer that depends on scene-graph array order, and your change picks a different arbitrary one. That downgrades this from "silent corruption" to "changes which coin-flip wins". Still worth exact parity, because otherwise the same file reloads with different joints after this lands. The tie-break itself is a pre-existing latent bug; I will file it separately rather than grow this PR.
3. Added the missing miter-cache tests. The description says the wall-mitering, slab-support and spatial-grid suites cover the invalidation paths. I mutation-tested each new cache — broke its invalidation, ran the full suite:
| cache | mutation | result |
|---|---|---|
polygonMemo (slab-support.ts) |
never reset | 3 fail ✅ |
supportInputs (spatial-grid-manager.ts) |
ignore revision/identity guards | 1 fail ✅ |
levelMiterCache (wall-system.tsx) |
sameMiterInputs → return true |
0 fail ❌ |
Two of three are genuinely covered. The headline cache had none: it could serve a permanently stale miter solution with all 818 tests green — and a stale hit silently rendering wrong joints is the exact risk your own comment names.
It was not reachable from a test where it sat, so I extracted it to packages/viewer/src/systems/wall/level-miter-cache.ts (the closure-factory pattern this repo already uses, since there is no DOM/React test infra) and added level-miter-cache.test.ts: reuse across equal-but-not-identical arrays, recompute on move, per-level keying, clear() on teardown, one case per compared field. That same mutation now fails 7 tests.
4. Made the level-elevation memo weakly keyed. The new single-slot memo in storey.ts holds a strong reference to the whole nodes record, so closing a project keeps its entire graph reachable until the next call. terrain-support.ts:55 already wraps this same function in a WeakMap for that reason — I matched it. Worth noting your memo is not redundant with that wrapper: space-detection.ts:979 and stair-opening-sync.ts:610,630 call getLevelElevations directly and bypass it, so memoizing at the function is the better placement. Only the strong retention needed changing.
Verification on the final state:
bun run check— cleanbun run check-types— 9/9 tasksbun run test— 12/12 tasks, 2882 pass / 0 fail
Two notes, neither a change request. Bugbot's only finding ("miter cache never cleared") was already fixed by your own e54ff9d6. And "How to test" step 6 says bun test — that is a Bun builtin which shadows the package script and never invokes Turborepo, so it does not run what it looks like; bun run test is the gate.
Ready to merge once you have looked over the four commits and confirmed you are happy with them — particularly the ordering fix, since it changes output the comment asserted was unchanged.
|
CI is green now (it was sitting at Follow-up filed as promised in my review: #581 covers the pre-existing junction-ordering bug. To restate the scope so it isn't misread as a regression from this PR — Merging. The per-frame rebuild this removes was a real cost on any scene with a meaningful wall count, and the cache test coverage is what makes it safe to take. |
…base elevation pascalorg#556 landed an identity-keyed WeakMap memo on getLevelElevations after this branch last ran green. The new base-elevation case mutated `stackedNodes` in place, so the second resolveStairTotalRise call handed the memo the same object and got the cached 2.9 back instead of 2.1. The memo's contract holds in production — every store write publishes a new record (updateNodesAction spreads into `nextNodes`) — so the fix belongs in the test. The sibling storey-height case in this same file already builds a fresh record; match it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(viewer): per-level base elevation parameter baseElevation is an offset: it shifts the level and every level above it within the same building (cumulative). Integrated into the level stacking computation so stacked, exploded, solo, floorplan, and snap-to-true-positions all respect it. Includes focused tests. Closes #209 * fix: thread baseElevation through slab clamps, elevator stacking and migration Three Bugbot findings, all the same shape: baseElevation was applied in one path and ignored in another. - Covering-slab math assumed the floor above sat exactly one stored storey height away, so wall and ceiling clamps ignored the offset. A positive offset over-shortened walls under thick slabs and a negative one let them penetrate the slab above. Floor-to-floor distance now comes from the stacked elevations (above.baseY - current.baseY) via one helper, so the clamp math and getLevelElevations cannot drift apart. - Stair rise used the stored storey height for the same reason; it now uses the same helper. - Elevator level tables and the first-person elevator colliders built cumulative Y from storey heights, so cab stops desynced from the visible floors. Both now read baseY from getLevelElevations. first-person-controls had its own near-copy of that logic, which is deleted in favour of the shared resolveElevatorLevels. - Levels loaded from older project JSON could omit baseElevation, which made the editor control render NaN. Migration now normalizes it to 0 alongside level and children, with a defensive fallback at the control. * viewer: stop the level-system test mocking core, and fix the type gate The quality gate was red for two reasons. 1. `frameCallback?.({}, delta)` typed `never`. The useFrame mock assigns that binding while LevelSystem() runs, which TypeScript cannot see, so after the `frameCallback = null` reset it narrowed the binding to `null`. Reading it through a function keeps the declared type. 2. `mock.module('@pascal-app/core', ...)` replaced core's entire export surface. mock.module is process-wide and Bun does not restore it, so every viewer suite that ran after this file got the fake core: running the systems directory went from 47 pass / 0 fail to 29 / 2, and a whole-repo run lost 32 tests, including wall-support-extension's covering-slab case. The failure looked like a baseElevation regression and was not one. Core does not need mocking here — `sceneRegistry` is a real in-memory store with a `clear()`, and `useScene` is a zustand store with `setState`. The test now drives both directly and only mocks `@react-three/fiber` and `use-viewer`, the two modules that genuinely need a renderer or a React context. Dropping the `lerp` mock too: the real one is already pure. Also adds the now-required `baseElevation` to the level fixture in wall-drafting.test.ts — the schema default makes it required on LevelNode's output type, so the existing `as AnyNode` cast no longer held. Gates on the merge ref (main merged in): check clean, check-types 9/9, test 12/12 tasks with 0 fail, build 7/7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(core): build a fresh nodes record when re-asserting the lowered base elevation #556 landed an identity-keyed WeakMap memo on getLevelElevations after this branch last ran green. The new base-elevation case mutated `stackedNodes` in place, so the second resolveStairTotalRise call handed the memo the same object and got the cached 2.9 back instead of 2.1. The memo's contract holds in production — every store write publishes a new record (updateNodesAction spreads into `nextNodes`) — so the fix belongs in the test. The sibling storey-height case in this same file already builds a fresh record; match it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): take the highest ceiling in the stack as the elevator shaft top The editor-side helper this PR consolidates into resolveElevatorLevels guarded the stack top with Math.max over every level; the core version reads only the topmost level. With the new baseElevation that is no longer the same thing — a negative offset can sink the top level's ceiling below the level beneath it, and the shaft then tops out under a served level and clips the cab (3 m -> 2.5 m for a -2.5 m offset on a 2 m top storey). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Aymeric Rabot <aymeric.rabot@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for taking this through review, and for #581 -> #596 — good to see the ordering trap closed properly rather than papered over. One small thing, flagged so it does not simply get lost: #598 added |

What does this PR do?
Wall geometry, mitre joints and slab support were recomputed inside the render
loop, so every frame paid for work whose inputs had not changed. On an imported
floor with 1089 walls the first level build took 59 s and the editor was
unusable until it finished. This PR caches the derived results and invalidates
them only when the walls, the level or the openings actually change: mitre
solving keeps a per-level result keyed by the exact wall fields the miters
depend on, slab support holds its effective slab/wall records by identity
instead of re-deriving them once per wall, and the rendered-polygon lookup
memoises against those same arrays. The same level now builds in 0.01 s.
No behaviour changes — the geometry produced is identical, it is simply produced
once instead of once per frame.
Related to #492 — the per-frame recompute is one of the paths that makes wall
interaction stutter on large scenes. Measurements and method are in
a comment on that issue.
The second commit clears the new level-miter cache when
WallSystemunmounts —it lives at module scope, so without that a remount or a second project in the
same tab keeps the previous level's wall arrays reachable. Editor teardown
already resets the other shared singletons; this makes the cache follow the same
rule, and the rule is now written down in
wiki/architecture/systems.md.How to test
bun devand open a scene with a few hundred walls (any floor plan import will do).be immediate rather than a multi-second freeze.
look exactly as they did before.
the cache must invalidate, not go stale.
tab — the miter cache must not carry entries from the previous level.
bun test—wall-mitering,slab-supportandspatial-gridsuites cover theinvalidation paths.
Screenshots / screen recording
N/A — no visual change. The output geometry is identical; only the time to produce
it changes. Happy to add a before/after screen recording of the load freeze if that
would help review.
Checklist
bun devbun checkto verify)mainbranchNote
Medium Risk
Caching and junction prefiltering must invalidate on the right inputs; incorrect hits would show wrong joints or wall bases, though tests cover miter cache equality and grid ordering.
Overview
Stops recomputing unchanged wall geometry work every frame on large imports (~1000+ walls), cutting first-level build from tens of seconds to near-instant while keeping output identical.
Miter solving now goes through a per-level cache (
getCachedLevelMiters) that compares the wall fields miters depend on (not array identity), so progressive rebuilds that pass fresh arrays each frame still hit. T-junction discovery infindJunctionsuses a spatial grid over wall AABBs instead of scanning every wall per junction, with passthrough walls still appended in input order so collinear thickness ties stay correct. Slab support reuses one derived slab/wall list per level per frame (getSupportInputsinspatialGridManager) and memoizes rendered slab polygons insidecomputeWallSlabSupportwhen those arrays are stable.getLevelElevationsis weak-memoized on thenodesslice.WallCutoutonly rebuilds the full-scene wall appearance key whennodes, materials, shading, or wall count change.The level miter cache is cleared on
WallSystemunmount so remounts/projects in the same tab do not retain stale entries; docs add a rule to clear module-level caches like other singletons.Reviewed by Cursor Bugbot for commit d87e4fa. Bugbot is set up for automated code reviews on this repo. Configure here.