perf(viewer): sew a level's walls into one mesh - #608
Conversation
…yers Both features hide an object by clearing its scene layer, and each stashed the previous `layers.mask` under its own private Symbol, restoring it wholesale on the way out. That only holds while they nest. Interleave them and the second to finish writes back a mask the first has since changed: solo a floor, isolate a wall, leave solo hands every level its scene layer straight back, so leaving solo un-hides exactly what the isolation filter was hiding. Clearing the filter afterwards then restores the mask isolation captured *during* solo, and the level is stuck shadow-caster-only with nothing soloed — invisible until reload. `lib/scene-visibility.ts` takes the mask over. Callers name a reason rather than a mask, the mask is recomputed from the one snapshot taken when the first reason arrived, and the original is handed back only when the last reason leaves. Order stops mattering, and two duplicated stash implementations collapse into one. The new `isolation.test.ts` drives the real pair in both interleavings; both cases fail on `main` and pass here. Co-Authored-By: Claude <noreply@anthropic.com>
Material groups are contiguous slices of the index buffer, so triangles that alternate between materials cost a draw call per run rather than per material. ExtrudeGeometry interleaves the cap and side faces, so every wall was emitting four groups for two materials. Bucketing the triangles by material before grouping cuts the floor from 8812 draw calls to 6544 and lifts the idle frame rate from 41.6 FPS to the 50 FPS frame-limiter ceiling. The image is unchanged: same triangles, same materials, only the order they are handed to the GPU differs. Co-Authored-By: Claude <noreply@anthropic.com>
A floor of a thousand walls issued a thousand draw calls, because every wall carried its own mesh and its own material groups. Sewing them into one geometry per level takes the same floor from 6520 draw calls to 25. The merge follows the scene's dirty tracking rather than watching the walls itself. A wall matters to the merged mesh for exactly one reason -- the wall system rebuilt its geometry -- and that system already runs off `dirtyNodes`, so this reads the same signal from both ends: the marks still standing when the frame reaches it, and the rebuild notices the wall system leaves behind for the walls whose marks it has already cleared. The per-frame cost is the size of the dirty set, not the size of the floor. That is also what keeps a dragged wall out of the batch. It is marked on every pointermove tick, so it is released in the same frame and stays released until the drag stops. Re-sewing waits for the wall system's deferred neighbour rebuilds to drain as well, so a floor is never merged from geometry that is about to change under it. The batch keeps a range-to-node map, so nothing that relied on a wall being its own object breaks: each run records the slice every source wall contributed, and hiding a wall from the batch rewrites the group list without touching a buffer. Pointer picking never went through the merged mesh anyway, it rides the wall's own invisible collision child. A wall the batch draws moves to a layer no camera enables, rather than emptying its draw range: three.js submits a draw call even for a zero-count group, so an emptied range saves nothing. Moving the mesh alone leaves its children -- opening cutters, treatments, the collision child -- rendering as before, which `visible = false` would not. That move goes through `lib/scene-visibility.ts` as a third reason alongside isolation and solo, so a wall that is sewn in and also hidden by one of those unwinds correctly whichever ends first; batching outranks the shadow-caster pass, since the merged mesh already casts the wall's shadow. Raycasters that must hit real surfaces opt into that layer through `setSurfaceRaycastLayers`, otherwise a sewn wall would stop answering measurement rays. Co-Authored-By: Claude <noreply@anthropic.com>
The merge is only worth having while the draw ranges stay flat as the floor grows, and nothing was watching that: the existing tests all run on three walls, where one range per material and one per wall look the same. Builds floors of 1, 10, 100 and 1000 walls and asserts the run and group counts stay at one per material, that every wall keeps its own addressable slice, and that hiding walls costs ranges proportional to the holes rather than to the floor. Reverting the merge turns 6 groups into 2997 and takes these down with it. Co-Authored-By: Claude <noreply@anthropic.com>
The isolation filter hides everything outside the focused subtree, and a level's merged wall mesh hangs off the level root, so it goes dark with everything else. Isolate a wall the batch had sewn in and nobody draws it: its own mesh is silent because the batch owns it, and its stand-in is hidden because the filter never heard of merged geometry. Teaching the filter about the batch would put the knowledge in the wrong place — isolation is a viewer-wide concern and the batch is an implementation detail of one system. So the batch steps aside instead: it releases every wall while a filter is up and re-sews the affected levels once it lifts. That costs one boolean check per frame and leaves isolation exactly as it was. Co-Authored-By: Claude <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Reviewed by Cursor Bugbot for commit 87a4d25. Configure here.
| const node = nodes[nodeId as AnyNodeId] | ||
| if (node?.type === 'wall' && node.parentId) staleLevels.add(node.parentId) | ||
| releaseWall(nodeId) | ||
| } |
There was a problem hiding this comment.
Hidden walls still batch-drawn
High Severity
Turning a wall off with node.visible (or its mesh visible) hides the per-wall mesh but does not remove it from the merged batch. toCandidate skips invisible walls only when re-sewing; runBatchFrame never releases batched walls on visibility changes, so the batch mesh keeps drawing geometry that should be hidden.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87a4d25. Configure here.
|
Bugbot is right, and thanks — this is a real hole and I had missed it. Fixed in e91b2a9. What was wrong. Why my own testing missed it. The default wall mode is One correction to the report. Selection itself is not affected: it is drawn by the merged The fix stands the batch down in every mode but The honest cost: in How I would recover it, as a follow-up rather than here. The machinery is already in That gets |
87a4d25 to
e91b2a9
Compare
The merged mesh captures one material set when it is sewn and nothing re-reads it. That holds in "up", where a wall's materials never move. It does not hold anywhere else: "cutaway" re-assigns them from the camera's facing test every time the view turns far enough, and "down" and "translucent" make every wall see-through. In those modes the merged copy kept drawing walls the cutaway pass had already turned to glass, so rotating the camera left the near walls solid and the mode did nothing. Isolation already had a stand-down for a related reason; this puts both behind one predicate rather than growing a second mechanism. Selection and delete-hover tints are applied the same way — by swapping the materials on the wall's own mesh — so a tinted wall now drops out of its batch and draws itself, the way an edited one already did. Selection itself was never affected: it is drawn by the outline pass, not the tint. The batch therefore buys nothing in cutaway mode. Recovering it there needs the merged buffer ordered by wall normal so the camera-dependent hidden set stays contiguous, which is a change of its own.
A wall that changes carries a dirty mark, and the batch already lets it go on that signal. Four inputs re-make every wall's material set without touching a node: the shading, texture and colour-preset toggles, the scene theme, and the scene's material library. The cutaway pass rebuilds all the wall materials from them and assigns them to the per-wall meshes; the merged mesh held whatever set it captured when it was sewn, so flipping any of them left a whole batched floor looking the way it did before. Watched by identity, so the check is four comparisons and a reference test per frame. They only move when someone deliberately flips a switch, which makes re-sewing the scene the cheap answer rather than the expensive one.
|
Second pass — one of these was a real hole I had left open, the other I do not think reproduces. "Batched walls ignore cutout modes" — the remaining part was real. Fixed in 6052bf5. The mode half of this went away with the stand-down in the previous commit, and a wall whose "Hidden walls still batch-drawn" — I do not think this one holds, and here is my working. Visibility is not set on the mesh directly. The tree's visibility toggle calls The one thing I will grant is that If the finding was aimed at a path I have not looked at — something that writes |
1 participant
What does this PR do?
Closes the draw-call half of #588 (and the "stuttering" half of #492): a floor of ~1000
walls issued 6520 draw calls at 42.7 FPS; sewing each level's walls into one mesh per
material set takes the same floor to 25 draw calls at 49.7 FPS, against this
renderer's ceiling of 50. Profiling fixture is the anonymised scene attached to #492
(gist,
1089 walls).
Scope, stated up front: the merge only runs in the
upwall mode.cutawayre-assignswall materials from the camera's facing test,
downandtranslucentmake wallssee-through, and the merged mesh captures one material set when it is sewn — so it stands
down in all three, the same way it already does under isolation. In
cutawaythis PRtherefore buys nothing; recovering it needs the merged buffer ordered by wall normal so the
camera-dependent hidden set stays contiguous, which is a change of its own. See the
discussion below.
Two commits carry the work:
draw each wall material once instead of once per face run— wall geometry emittedone material group per run of same-material triangles, so a wall with alternating
faces paid several draw calls for the same material. Sorting the index buffer by
material collapses that to one group per material. 7982 → 6544 calls on its own.
sew a level's walls into one mesh— the batch itself.Built on the dirty tracking from #556, as you asked in #588. The batch does not watch
the walls; it reads the same
dirtyNodessignal the wall system runs off, from both ends:deferred — a progressive import, or a mesh that has not mounted yet), and
cleared in that same frame. Neighbours re-mitred by the wall system's trailing-edge
flush only show up here — that path calls
updateWallGeometrydirectly and never marksanything dirty.
Per-frame cost is therefore the size of the dirty set, not the size of the floor.
The dragged wall falls out of the batch for free. It is marked on every pointermove
tick, so it is released in the same frame and stays released until the drag stops.
Re-sewing additionally waits for the wall system's deferred neighbour rebuilds to drain,
not just for a quiet window on the clock — otherwise a floor could be merged from geometry
that is about to change under it.
Deleting a wall marks the neighbours it re-mitres, not the node that went away, so
removals are caught by comparing the registry's wall count against the previous frame —
O(1) once the scene is quiet.
Nothing that relied on a wall being its own object breaks. Each run records the slice
every source wall contributed, so pulling a wall out rewrites the group list without
touching a buffer. A sewn wall moves to a layer no camera enables rather than having its
draw range emptied (three.js submits a draw call even for a zero-count group), which
leaves its children — opening cutters, treatments, the invisible collision child that
carries pointer events — rendering and picking exactly as before. Raycasters that must hit
real surfaces opt into that layer through
setSurfaceRaycastLayers.A level is only merged once it has at least 8 mergeable walls, and translucent or
cut-away walls keep the per-wall path, since merging would change their blend ordering.
Stacked on the isolation/solo layer fix
The first commit here belongs to a separate, smaller PR — #607, which gives
Object3D.layersa single owner so isolation and solo stop overwriting each other. Thebatch is the third caller of that module. Happy to rebase this once that one lands, or to
squash them if you would rather review a single change.
The last commit stands the batch down entirely while an isolation filter is up: the merged
mesh hangs off the level root and the filter hides it along with everything else, so a
focused wall the batch had sewn in would be drawn by nobody. Teaching isolation about
merged geometry felt like the wrong place for that knowledge, so the batch steps aside
instead — one boolean check per frame.
How to test
Automated —
bun run test --filter @pascal-app/viewer:src/lib/wall-batch.test.tsincludes a draw-call budget block: floors of 1, 10, 100and 1000 walls must all end at 3 runs and 3 groups. Verified by mutation — reverting the
merge turns 6 groups into 2997 and fails six tests.
src/lib/geometry-groups.test.tscovers the material-group sorting.Manually:
merged floor never lags a frame behind the wall you moved.
on
main.Screenshots
Same scene (the 1089-wall fixture from #492), same camera pose, same window, same overlay.
Before —
main: 6 767 draw calls, 0 FPS.After — this branch: 39 draw calls, 50 FPS.
Two things about that pair are worth stating plainly rather than leaving you to spot them.
The overlay is not in
main. It is part of another unlanded change of mine, so I appliedit locally to both sides purely as a measuring rig, together with a frame-the-scene-on-load
change — without that,
mainopens this scene with the camera inside the geometry and thereis no reliable way to get the same pose twice. The rig is three cherry-picked commits, touches
nothing in this PR, and was identical on both sides. The camera pose was set from the console
(
setLookAt(0, 230, 78, 0, 0, 0)), so it is the same to the pixel rather than eyeballed. Both sides are pushed if you want to reproduce the shots yourself:demo/main-rigisupstream
mainplus the rig,demo/batched-rigis this branch plus the same four commits. They are measuring scaffolding, not
proposals — the overlay probe in them would not pass this repo's lint as it stands.
The counters in the shots differ slightly from the headline numbers above (6 767 vs 6 520,
39 vs 25) because these were taken at a different camera pose and a smaller viewport than the
measurement run — draw calls scale with what is in frustum, and the batch splits per material
set in view. Same scene, same machine, same renderer.
The two pictures do not show the same amount of wall, and I want to be precise about why.
Wall geometry is built incrementally across frames, and the
maincapture had not finishedbuilding when I took the shot: the canvas renders on demand and each frame there costs ~6.7k
draw calls, so under a scripted camera it crawled — 1 130 to 3 608 triangles over 100 seconds
of continuous motion. That is a property of how I drove the capture, not a claim that
mainnever gets there. In the idle measurement quoted above,
mainhad the same floor fully built at60 536 triangles and 42.7 FPS. Read these two images for the draw-call counters; the amount of
wall drawn is an artefact of my capture method, and I did not want to quietly crop it out.
Checklist
bun devbun checkcleanbun check-typescleanmain