perf(viewer): sew a level's walls into one mesh - #608
Conversation
|
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
|
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 |
|
Ten days in, so this is a gentle nudge rather than a bump — mostly to say that nothing here is blocked on my side.
One leftover from #556 while I am here, flagged so it does not just get lost: the No urgency on any of this — I appreciate that external PRs queue up behind the day-to-day work. |
|
Maintainer follow-up pushed in 5bada49 and 7a255f3. The wall-specific batch implementation and tests now live under packages/nodes/src/wall and mount through the wall definition; viewer retains only generic layer/visibility primitives plus the existing legacy wall rebuild signal. Visibility updates now mark dirty synchronously, closing the hidden-wall timing gap before render rather than relying on the next RAF. Validation passed on the branch and on a local merge with current main: bun check, full check-types after a frozen merged-lock install, 117 Viewer tests, 957 Nodes tests (1 skipped), and the 33-test focused batching/visibility suite. I am leaving merge approval pending the 1089-wall browser smoke described in the PR. |
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>
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.
7479a03 to
41b926b
Compare
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 41b926b. Configure here.
| const invalidate = useThree((state) => state.invalidate) | ||
| const wakeRef = useRef<ReturnType<typeof setTimeout> | null>(null) | ||
|
|
||
| useFrame(() => runBatchFrame(invalidate, wakeRef), 5) |
There was a problem hiding this comment.
Batch frame runs after scene render
High Severity
WallBatchSystem subscribes with useFrame priority 5, while WallSystem and the post-processing render path use the default priority 0. In R3F, higher priorities run later, so release/merge work lands after the frame has already been drawn. Dirty walls (drags, visibility) therefore stay in the merged mesh for that frame, which breaks the claimed same-frame handoff and can leave a stale batch under render-on-demand until another invalidate.
Reviewed by Cursor Bugbot for commit 41b926b. Configure here.
|
Thank you, Andrei. This is a substantial and carefully decomposed performance contribution: first reducing each wall material to one draw run, then batching level walls while preserving picking, isolation, shadows, visibility, editing, and material changes. The 1,000-wall fixture and draw-call budget tests made the improvement concrete: 6,520 draw calls fell to 25 in the submitted benchmark. I completed the maintainer pass by keeping the batching system with the wall node definition, dirtying visibility changes before render, excluding selected/preview/delete-hover tinted walls, documenting the batched layer contract, and adding regression coverage for the threshold case. The combined branch passes formatting, types, the editor build, 1,089 focused tests, and a browser smoke of the 1,089-wall fixture that settles to six meshes and switches 3D/2D/3D without runtime errors. Credit for the design, implementation, fixture, and core performance win belongs to you. I appreciate how promptly and thoughtfully you worked through every review finding. |
Aymericr
left a comment
There was a problem hiding this comment.
Approved after the integrated architecture, regression, and browser validation pass. Thank you, Andrei.
…pickable (#686) * fix(editor): stop node tops lifting floor placements off the ground #638 made the pointer support election consider node tops by default — `options?.includeNodeTopSurfaces === false` opted out, so every caller that passed nothing (item placement, registry move/presets, slab drafting) started electing them. It also gave `item` a `surfaces.top`, widening the candidate set from wall/item/column to wall, slab, ceiling, cabinet, column, item, shelf, block. A ray aimed at a floor crosses every upward-facing face above that floor first. In a finished room that is the ceiling: placing an item at the room centre elects the ceiling's top face (nearest hit, normal.y ≈ 1) and freezes it into the draft's authored Y via `resolveFrozenFloorPlacementPatch`, so the item sits at ceiling height instead of on the floor. Walls the ray passes over do the same in a narrower band. Restore the opt-in. Keep #638's registry-driven discovery — the kind list is still derived from `capabilities.surfaces.top` rather than hardcoded — but only the tools that build ON a surface ask for it: wall (already did), column, fence, stair, block. Item placement, registry move and slab drafting go back to placing against the floor the pointer indicates. Also exclude the node the active interaction is placing or moving. Its mesh rides the cursor, so electing its own top would raise it by its own height every pointer move. The tools neuter the dragged mesh's `raycast` for their own pointer routing, which happens to cover this today — but that is each tool's private convention, and async-mounted item children are only neutered on the next frame. The election owns the invariant now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(viewer): keep batched walls answering the pointer #608 sews a level's walls into one mesh once they settle (8+ walls, 180ms quiet). Each sewn wall is moved off SCENE_LAYER onto BATCHED_LAYER by `hideBatchedWall` so it costs no draw call while staying in the graph — with its R3F pointer handlers still attached. R3F picks with one shared raycaster whose default mask is SCENE_LAYER alone, so a batched wall stops being hit: no `wall:enter` (no hover outline, no paint preview), no `wall:move`, no `wall:click`. Selection is the circular case — a selected wall leaves the batch, but the click that would select it never lands. #608 saw this for measurement and added `setSurfaceRaycastLayers` for the raycasters that module builds; the shared event raycaster was never opted in. Enable BATCHED_LAYER on it. Additive rather than `setSurfaceRaycastLayers`, which resets the mask — right for the private per-query raycasters it was written for, wrong for the one every pointer event goes through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#687) * fix(nodes/wall): let a hovered wall out of its batch so it can outline Hover feedback for a wall is an outline, nothing else: `SelectionMaterialSync` skips walls outright and the cutaway pass tints only a selection or a delete hover, so `default`, `paint-ready` and `paint-disabled` all reduce to `outliner.hoveredObjects` and a colour. `MergedOutlineNode` draws that mask with `renderer.render(scene, camera)` on the main camera, which enables no `BATCHED_LAYER`. Once #608 sews a level's walls (wall mode `up`, 8+ walls, 180ms quiet) every wall on the floor is on that layer alone, so it reaches neither the depth pass nor a mask pass and hovering it lights up nothing at all. Selection looks fine only because `collectTintedWalls` already pulls a selected wall back out of the batch — the click lands, the wall starts drawing itself, and the outline appears. Hover had no such release, so on any finished floor the wall you point at goes dark: no hover outline in select mode, and in paint mode no preview of the surface the next click will paint. Release on any hover, not just a delete one. Same mechanism, one more reason: selection and delete tint through materials the merged mesh never reads, a plain hover outlines through a camera that never sees it, and both are fixed by the wall drawing its own geometry while it is lit. One wall is lit at a time, so the cost is the one extra draw call the delete path already accepted. Verified on a 12-wall level in the community editor: before, hovering a wall in `Full height` produced no outline in select or paint mode while `Cutaway` (where batching stands down) outlined correctly; after, both modes outline in `Full height`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: apply biome formatting to the hover test Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>


What does this PR do?
Fixes #588. Contributes to the remaining draw-call half of #492.\n\nMaintainer follow-up (August 17, 2026): wall-specific batching now lives with the wall definition under
packages/nodes/src/wall, while@pascal-app/viewerexposes only generic visibility/layer primitives and the existing wall rebuild signal. Visibility mutations also mark nodes dirty synchronously, so a hidden wall leaves the merged batch before the next render, including under render-on-demand.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 —
bunx turbo run test --filter=@pascal-app/viewer --filter=@pascal-app/nodes:packages/nodes/src/wall/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.
packages/viewer/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 / screen recording
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 checkto verify)mainbranchNote
High Risk
Large rendering-path change with intricate batch invalidation (modes, isolation, materials, drags); mistakes could cause wrong visuals, stale geometry, or broken picking/measurement, though coverage is substantial.
Overview
Cuts wall draw calls by sewing each level’s opaque walls into one merged mesh per material set after edits settle (~180ms quiet window and pending wall rebuilds drained). Per-wall meshes stay in the scene for picking and children but move to
BATCHED_LAYERvia a newbatchedhide reason; the merged mesh skips raycasts.Wall geometry now sorts material groups (
setGroupsSortedByMaterial) so each wall draws once per material instead of once per interleaved face run—prerequisite for merging.WallBatchSystem(in nodes) releases walls on dirty/rebuild signals, selection/hover tints, theme/material changes, wall count changes, and suspends when wall mode isn’tupor isolation is active.drainRebuiltWalls/getPendingWallRebuildCountbridge the viewer wall system to batch timing.Visibility updates call
markDirtyimmediately (not only on the batched RAF) so hidden walls drop out of batches the same frame, including render-on-demand.Measurement raycasters use
setSurfaceRaycastLayers(SCENE_LAYER+BATCHED_LAYER) so surfaces stay queryable when walls are batch-drawn.Reviewed by Cursor Bugbot for commit 41b926b. Bugbot is set up for automated code reviews on this repo. Configure here.