editor: keep room surfaces in sync with wall topology

[sudhir9297]

What does this PR do?

Keeps room walls, generated slabs, and generated ceilings aligned as wall topology changes.

Wall topology

• Splits straight and curved walls when a divider connects to or crosses them.
• Splits crossing dividers so all wall segments meet at the same junction.
• Preserves doors, windows, and wall-mounted items on the correct replacement wall.
• Applies each multi-wall operation atomically as one undo step.
• Avoids duplicate/covered walls, near-endpoint slivers, and invalid splits through openings.

Slabs and ceilings

• Creates one generated slab and ceiling per detected room after a divider is added.
• Updates or merges surfaces when dividers move or are removed.
• Preserves materials, slots, dimensions, visibility, heights, and openings when rooms split.
• Combines compatible surfaces and preserves conflicting customized surfaces as manual surfaces.
• Keeps deliberately deleted generated surfaces deleted after later edits or reloads.
• Keeps existing slabs and ceilings when an exterior wall is removed.

Scoped reconciliation

• Uses an in-memory topology index to reconcile only rooms near the changed walls.
• Does not persist deletion-memory or topology-index data in project files.
• Does not generate missing surfaces merely because an existing project was loaded.
• Rebuilds the in-memory index safely after load, undo, and redo.

Editor behavior

• Keeps 2D and 3D wall-chain continuation consistent.
• Keeps a 2D wall chain active when a duplicate or covered commit is rejected.
• Moves topology planning into @pascal-app/core; editor tools only apply the resulting atomic scene changes.

How to test

1. Run bun run check, bun run lint, and bun run check-types.
2. Create a four-wall room and confirm it has one slab and one ceiling.
3. Add a divider and confirm both boundary walls split and the tree shows two slabs and two ceilings.
4. Cross two dividers and confirm four wall segments meet at the center and four room surfaces are generated.
5. Connect a divider to a curved wall and confirm the curve and any hosted openings remain correct.
6. Customize surfaces, then split and merge rooms; confirm materials, dimensions, and openings are preserved.
7. Delete a generated slab or ceiling, edit another wall, and reload; confirm the deleted surface remains absent.
8. Remove an exterior wall and confirm its existing slab and ceiling remain unchanged.

Validation

• Full source suite: 2,227 passed, 1 skipped, 0 failed.
• bun run check, bun run lint, and bun run check-types pass.

Checklist

• Tested locally
• Follows existing code style
• Targets main

---

Note

High Risk
Touches core wall topology, scene history commits, and automatic slab/ceiling reconciliation—behavior that affects every floorplan edit, undo/redo, and load path.

Overview
Adds wall-topology planning in core so wall inserts/splits (straight or curved, including crossings) land as one atomic scene change, with doors/windows/items kept on the right segment and duplicate/covered segments rejected.

Space detection no longer keys off coarse level structure hashes; it listens to scene commits (with changedNodeIds) and reconciles only rooms near edited walls via an in-memory RoomTopologyIndex. Auto slabs/ceilings split and merge with rooms—carrying materials, heights, and openings (including clipped stair/elevator holes)—while incompatible merged surfaces are demoted to manual instead of forced union. User-deleted generated surfaces stay gone across later edits, reload, and undo/redo; load does not backfill missing auto surfaces.

The editor wires onSpacesChanged into space sync, aligns 2D wall-chain stop/continue with the mounted tool (including rejected commits), and exports topology helpers plus createDefaultSceneSnapshot from core.

^{Reviewed by Cursor Bugbot for commit f018494. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

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 7e6bd1d. Configure here.}

[cursor]

Index treats new walls as stale

Medium Severity

sameIndexedWall compares missing walls with left === right, but the cache lookup returns undefined while the before-state path normalizes absence to null. For any newly created wall ID, that mismatch makes ensureBeforeLevel discard the spatial index and rebuild the whole level, so divider inserts take the fallback path instead of the indexed reconcile the PR is built around.

Additional Locations (1)

• packages/core/src/lib/space-detection.ts#L1901-L1916

 

^{Reviewed by Cursor Bugbot for commit 7e6bd1d. Configure here.}

[Aymericr]

Thanks for the depth here — the topology-delta approach is the right idea, and TopologyIndex incrementalising findJunctions per level is genuinely valuable work. I've spent real time in this branch and I want to give you a straight answer rather than let it sit.

I can't merge it as it stands. There's one blocking correctness problem, and the shape of the PR makes it hard to land even once that's fixed. Both are addressable.

Blocker: reconciliation writes are invisible to collaboration

initSpaceDetectionSync now reacts to the commit stream instead of subscribing to the store:

const unsubscribeCommits = subscribeSceneCommits(processCommit)

and processCommit performs its writes with history paused:

isProcessing = true
pauseSceneHistory(sceneStore)
try {
  for (const [levelId, wallIds] of changedWalls) {
    const topologyDelta = topologyIndex.applyWallDelta(…)
    const spaces = reconcileWallTopologyDelta(levelId, topologyDelta, commit.current.nodes, sceneStore)

Commits originate from zundo's onSave, which snapshots both sides before any subscribeSceneCommits listener runs:

// packages/core/src/store/use-scene.ts:1605
onSave: (pastState, currentState) => {
  notifySceneCommit({
    origin: 'local',
    before: sceneHistorySnapshotFromState(pastState),
    current: sceneHistorySnapshotFromState(currentState),
  })
},

So reconciliation's writes land after commit.current is captured, and because they run under pauseSceneHistory they produce no commit of their own. The hosted editor derives the entire shared operation from exactly those two snapshots:

// apps/community — use-project-scene-collaboration.ts:552
const diff = deriveSceneOperationChanges(commit.before, commit.current)

Anything reconciliation mints is therefore never transmitted.

I measured it rather than inferring it. Same harness — four walls closing a room, capture every commit, compare against the live store:

On main:

origin=local before=5 current=8
LIVE ids: 8 ["building","level","wall","wall","wall","wall","slab","ceiling"]
NEVER IN ANY COMMIT current: []

On this branch:

origin=local before=5 current=6
LIVE ids: 8 ["building","level","wall","wall","wall","wall","slab","ceiling"]
NEVER IN ANY COMMIT current: ["slab_w7u9weytnh0q51zz","ceiling_2ul4zldaqsglvzsg"]
their types: ["slab","ceiling"]
live level children: ["wall_1","wall_2","wall_3","wall_4","slab_…","ceiling_…"]

main publishes all 8 nodes in the commit; this branch publishes 6 and the auto slab and ceiling never appear in any commit. In a shared project, closing a room gives the author a floor and a ceiling and gives every collaborator a bare wall — plus a level.children array that has diverged between clients, which is the kind of drift that doesn't heal on its own. The same applies to the frontSide/backSide surface flips.

Worth being explicit: this is a regression the PR introduces, not a pre-existing gap it happens to expose. The store-subscription approach on main works precisely because reconciliation writes land before the snapshot is taken.

The fix is structural, not a patch: reconciliation either has to write inside the same commit that triggered it, or emit its own commit that collaboration can pick up. Either is a real design decision, which is why I'd rather it not ride along inside a 4,700-line change.

Why I'd like this split

Setting the blocker aside, the branch is hard to review or land as one unit:

• Merge state is CONFLICTING — 25 hunks across 5 files.
• 44 commits, ~30 of which are merge commits from repeated main merges. The actual narrative is buried.
• One unrelated commit: 3731eb32 "Add roof surface placement support for items" has nothing to do with wall-topology sync.
• 6 of the 11 new packages/core/src/index.ts exports have no consumer outside core. Public surface is the hardest thing to walk back, so I'd rather only export what's actually used across the package boundary.

Concretely, what I'd merge quickly:

1. TopologyIndex + the per-level junction incrementalisation, on its own. Pure, testable, no commit-stream coupling. This is the part I actively want. Note that perf(core,viewer): stop rebuilding wall geometry every frame #556 has since landed a spatial-grid prefilter for findJunctions (level assembly 59.3 s → 0.01 s), so please rebase and re-measure — some of your wins may now be on main, and the remainder will be easier to see against the new baseline.
2. The surface-sync behaviour, as a follow-up on top, with the commit-visibility question settled first.

I've opened #586 for the collaboration-visibility contract so the design discussion has a home and isn't a review thread on a large PR — reconciliation-generated nodes needing to be transmittable is a constraint that applies to anything in that path, not just this branch. It also proposes the regression test that would have caught this automatically, since right now the invariant is enforced only by call ordering.

Closing for now to keep the queue honest about what's actionable, not as a rejection of the work. Open #1 as a fresh PR and I'll review it promptly — a focused TopologyIndex PR is something I expect to merge. Happy to talk through the commit-visibility design in #586 before you invest more in part 2.

[Aymericr]

Following up because I owe you a correction on the advice I closed this with.

I said part 1 — TopologyIndex plus the per-level junction incrementalisation — was "pure, testable, no commit-stream coupling" and something I expected to merge quickly. I then ran a deeper adversarial pass over the branch before you invest in resubmitting, and part 1 is not clean either. Three of the four defects below live in exactly the code I told you to send back first. I'd rather you hear that now than after you've split the PR and reopened it.

Framing first, because it affects how urgent any of this is: none of this code is on main, so nothing here is hurting users today. runWithSceneCommitNodeIds, RoomTopologyIndex, applyWallDelta and wall-topology.ts don't exist outside this branch. These are review findings for the resubmission, not production bugs.

Method, so you can weigh each claim: every defect below was reproduced by executing code at f018494f and re-run on the merge base ab76686b to establish it's a regression the branch introduces rather than something it inherits. 14 candidate defects went through that gate; 8 did not survive it and aren't listed. I verified #1 myself end to end.

---

1. Draft subdivision has no per-span minimum length — packages/core/src/systems/wall/wall-topology.ts:500

planWallInsertion applies isSegmentLongEnough once, to resolvedStart/resolvedEnd (line 500), then subdivides the draft at every crossing (539-548) with no per-span check. The only dedup between consecutive crossings is pointsEqual at WALL_INTERSECTION_EPSILON = 1e-6, so any two crossings more than a micron apart mint their own wall.

Two parallel hosts g apart, draft crossing both:

r=0.05 gap=0.003    inserted=[3, 0.003,   2.997]     BELOW_MIN=[0.003]
r=0.05 gap=0.006    inserted=[3, 0.006,   2.994]     BELOW_MIN=[0.006]
r=0.05 gap=0.00001  inserted=[3, 0.00001, 2.99999]   BELOW_MIN=[0.00001]
r=0.35 gap=0.003    inserted=[3, 0.003,   2.997]     BELOW_MIN=[0.003]

Same at both real radii — WALL_CONNECT_SNAP_RADIUS 0.05 and WALL_JOIN_SNAP_RADIUS 0.35 — so magnetic snapping doesn't absorb it. Sweeping the gap shows the cut is exactly WALL_MIN_LENGTH, with nothing below it rejected:

gap=0.02  min=0.02      gap=0.011 min=0.011
gap=0.01  min=0.01      gap=0.009 min=0.009

This is reachable with default grid snapping on — my first instinct was that it needed snap-off to construct, and that's wrong. Two walls drawn from the same 0.5 m grid corner to slightly different grid endpoints, then a horizontal draft across both:

wall_g1: [0, -0.5] → [0.5, 4]
wall_g2: [0, -0.5] → [0.5, 4.5]
draft:   [-1, 0]   → [3, 0]
→ inserted = [1.05, 0.005555555555555536, 2.9444444444444446]

Every endpoint in that scene is on the lattice, and the result is a 5.5 mm wall. Both crossings are host-interior (t ≈ 0.11), so joinCrossingAtNearbyWallEndpoint doesn't apply — it snaps a crossing to a host's endpoint within 0.02, and says nothing about spacing between successive crossings along the draft.

Nothing downstream catches it: createWallOnCurrentLevel length-checks only the raw draft before calling this, WallNode has no min-length refine, and applyNodeChangesAction has no length filter. And it's not recoverable by merging — buildWallMergePlans needs exactly 2 walls at a junction and 4 meet at each sliver end, while 5.5 mm is under both the connect (0.05) and endpoint (0.7) snap radii. So it's an unselectable, unmergeable sliver.

Worth noting this is a defence main already documents and this branch drops. wall-drafting.ts:47-51:

An endpoint projecting within this distance of an existing wall's corner resolves to the corner without splitting — splitting there would mint a sliver segment a hair longer than WALL_MIN_LENGTH that no snap radius can ever target again.

The branch ports that epsilon for the host-endpoint case only. main never hits the draft-side version because createWallOnCurrentLevel emits exactly one wall spanning resolvedStart→resolvedEnd and never subdivides — so its single gate genuinely covers everything it creates. Subdividing the draft is what makes the gate insufficient, and that's new here.

A vertex-collapse in the same place fixes it. I tried this and it holds — probe clean at every gap above, and the branch's own 28 wall tests still pass:

const kept: WallPlanPoint[] = []
for (const point of splitPoints) {
  const prev = kept[kept.length - 1] ?? resolvedStart
  if (!isSegmentLongEnough(prev, point)) continue
  if (!isSegmentLongEnough(point, resolvedEnd)) continue
  kept.push(point)
}

That's a sketch, not a prescription — dropping a crossing means the draft no longer splits that host, and you may prefer to nudge the vertex or skip the split explicitly. Your call on which semantics you want.

---

2. Scoped reconciliation misses a room newly enclosed between two existing rooms — space-detection.ts:1971

Rooms A (0,0)-(4,3) and B (6,0)-(10,3), both with slabs and ceilings, plus a pre-existing corridor bottom wall (4,0)-(6,0). Draw the corridor's top wall (4,3)-(6,3):

reconcile:  {strategy:"fallback", affectedBeforeRoomCount:0, affectedCurrentRoomCount:0}
detectSpacesForLevel: 3 rooms
published spaces:     2          slabs: [slab_a, slab_b]   ceilings: [ceiling_a, ceiling_b]

Merge base on the identical scene gives 3 spaces, a third slab with polygon [[4,3],[4,0],[6,0],[6,3]], and a matching ceiling.

The mechanism is one hop further out than it first looks. The BFS does collect A's and B's own walls — all 8 are in examinedWallIds. What it can't reach is the corridor's pre-existing bottom wall, because that wall only touches wall_a_right / wall_b_left, which is precisely where queue.push(neighbor) sitting in the else (1982-1988) stops traversal at a room-owning wall. Without that wall, extractRooms(currentCandidateWalls) can't close the corridor loop, affected.current is empty, and the surface block at :2293 is skipped.

Two things make this worse than a missing floor. wallClosesRoom returns true and detectSpacesForLevel returns 3, so the wall tool auto-stops the chain and treats it as a genuine closure — the two systems that tool.tsx:740 comments "never disagree" now disagree. And reload only partly rescues it: the load branch republishes 3 Spaces but never runs surface sync (the branch's deliberate no-backfill-on-load rule), so the Space comes back and the slab and ceiling don't.

3. Same gap, indexed branch — space-detection.ts:1960

Same scene, but instead extend A's own top wall to (0,3)-(6,3), changedNodeIds={wall_a_t}:

{strategy:"indexed", examinedWallIds:["wall_a_b","wall_a_l","wall_a_r","wall_a_t"],
 affectedBeforeRoomCount:1, affectedCurrentRoomCount:1}
detectSpacesForLevel: 3 rooms → published 2

Reachable through the wall panel's length field, which calls updateNode({end}) on a single id.

These need two different fixes, not one. In the else/BFS branch the new walls are reached and expansion stops at a room-owning wall. In the if/indexed branch they're never enumerated at all, because both enumeration rules — the room's own boundary walls, and walls whose interior lies inside the before polygon — are defined purely in terms of the before room's geometry, and the new enclosure is outside it. The if branch needs candidate enumeration seeded from the changed walls' current geometry regardless of whether a before-room was hit.

Neither is covered: the nearest tests (space-detection.test.ts:1120, :1167) both have a closing wall whose neighbours own no room, or a pre-existing room with no surfaces, so the room-owning-wall stop never fires.

4. changedNodeIds is frozen before cascades and merges grow the id set — use-scene.ts:1479

Two independent passes found this one, which is part of why I'm confident in it. runWithSceneCommitNodeIds snapshots new Set(nodeIds) at history-control.ts:70 before run(), while deleteNodesAction grows the real deletion set inside the set() callback — collect() recursing through children and onDeleteCascade, plus allIds.add(plan.secondaryWallId) for every merge plan. None of that feeds back. changedWallIdsByLevel only full-scans when candidateIds is undefined, and deleteNode/deleteNodes always pass a defined set.

Level cascade — one room on level_1, then deleteNode('level_1'):

spaces before:  1 ["level_1"]
nodes after:    ["building_1"]
spaces after:   1 [{levelId:"level_1"}]     ← merge base: 0

Collinear merge — room whose bottom edge is wall_b1+wall_b2 meeting at [2,0], plus a stub there; deleteNode('wall_stub') merges the two and deletes wall_b2:

published space wallIds: ["wall_b1","wall_b2","wall_r","wall_t","wall_l"]
                                     ↑ deleted
polygon: [[0,0],[4,0],[4,3],[2,3],[0,3]]    ← phantom vertex at (2,3)

Feeding identical before/after records with changedNodeIds=["wall_stub"] gives the stale 5-face space; ["wall_stub","wall_top_a","wall_top_b"] or undefined both give the correct 4-face one. The narrowed set is the sole cause.

Reachable via this PR's own flagship interaction — planWallInsertion splitting a wall for a stub, then deleting the stub — and via all three deleteLevelWithFallbackSelection call sites.

Downstream: resolvePaintScopeTargets reads useEditor.spaces at selection-manager.tsx:877, and paint-scope.ts:355-369 matches slab centroids against space.polygon with no levelId check — so a phantom level_1 space makes room-scope paint select unrelated slabs on level_2. In the merge case the merged wall is silently dropped from room-scope paint instead.

Two honest caveats on severity: it isn't permanent — origin:'load' and undo/redo both trigger a full rebuild — and it isn't per-level-isolated, since a fresh room on a sibling level leaves both spaces published. It persists across ordinary continued editing, which is exactly why the suite stays green.

---

One thing that looked bad and isn't

A candidate defect claimed every client independently mints its own auto slab from a remote peer's commit, accumulating duplicate stacked floors. I checked it and it doesn't hold, so please don't spend time on it: all three remote-apply sites in apps/community are wrapped in withSceneMutationLock, notifySceneCommit({origin:'host'}) fires synchronously while readOnly === true, and all of space-detection's write paths bail on readOnly at their first line (node-actions.ts:787, :984, :1073). And processCommit brackets everything in pauseSceneHistory, so no minted surface can become an origin:'local' commit to submit in the first place.

The residue is real but much narrower, and it's the same defect as #586 seen from the other end: because the originating peer's local commit no longer carries the generated surfaces, the receiving peer ends up with none — and a later local edit to that room doesn't backfill them.

---

Where this leaves the split

Unchanged in shape, but part 1 now has a review of its own to answer, and I shouldn't have implied it was a formality. wall-topology.ts is genuinely the pure, testable part, so #1 above is the good kind of finding — a unit test in wall-topology.test.ts alongside the existing joins at a nearby host endpoint without minting a sliver wall would pin it permanently, since that test currently covers only the single-host case.

#2 and #3 are the ones I'd think hardest about, because they say the scoped path is not yet equivalent to the full scan it replaces. That equivalence is the whole value proposition of the incrementalisation, and it's the kind of property worth asserting directly: for a set of representative edits, scoped reconciliation and detectSpacesFromWalls should produce the same spaces and the same surfaces. Getting that harness in place would have caught #2, #3 and #4 together, and it's a better use of your next commit than fixing them one at a time.

Still keen to see this land, and still happy to review promptly. If it's useful I can put the #1 vertex-collapse and its regression test up as a small PR of its own so it's off your plate — say the word and it's yours to review rather than the reverse.