feat(viewer): per-level base elevation parameter

[mvanhorn]

What does this PR do?

Adds a per-level baseElevation parameter so each level can offset its computed stack position upward by a user-specified amount. The site panel surfaces a Base elevation MetricControl on each level (m, 2dp, 0.05 step) and the viewer's level system threads the offset through getLevelTargetY / getNextLevelCumulativeY helpers.

How to test

1. Open the site panel.
2. Expand any level.
3. Set the Base elevation field to a non-zero meter value.
4. Confirm the level (and all levels above) move up by that amount in 3D.
5. Switch to exploded mode; confirm EXPLODED_GAP still adds on top of the cumulative offset.
6. Run the test suite: bun test packages/core/test/level.test.ts packages/viewer/test/level-utils.test.ts.

Notes

check-types flags a pre-existing TypeScript error on the base branch unrelated to this change (packages/editor/src/store/use-editor.tsx:249 — childId implicit any). The error is present on origin/main before this branch is applied, so the failure isn't introduced by this PR. Happy to fix it in a sibling PR if useful.

Checklist

• Ran bun dev and exercised the change locally
• Ran bun check (biome clean)
• bun check-types fails on pre-existing error (see Notes)
• Tests added for the schema + utility paths
• Targets main

Closes #209

---

Note

Medium Risk
Changes core vertical stacking used by walls, ceilings, stairs, elevators, and 3D level placement; behavior is broad but covered by targeted tests and legacy normalization on load.

Overview
Adds level.baseElevation (meters, default 0): an additive offset applied when stacking floors so it shifts that level and every higher level in the same building; negative values are allowed.

Stacking & clamps: getLevelElevations now folds each level’s offset into cumulative baseY. Covering-slab, wall-plane, and ceiling clamp logic use a new getLevelFloorToFloorHeight (distance to the floor above) instead of only the stored storey height, so split-level gaps stay correct.

Downstream: Stair rise (when not explicit) follows offset-aware floor-to-floor height. Elevator shaft/cab math uses shared getLevelElevations; shaft top uses the max ceiling in the stack so a sunken top level cannot clip the cab. Scene load normalizes legacy levels via normalizeLevelBaseElevation. The site panel exposes Base elevation; editor elevator colliders reuse resolveElevatorLevels.

Tests cover schema, storey math, migration, elevators, stairs, and viewer level presentation modes.

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

[Aymericr]

This remains a distinct product capability, so I’m keeping it open. Before code review it needs a current-main rebase and one explicit semantic decision: is baseElevation an offset that shifts this level and every level above it, or an absolute Y for this level only? The current cumulative implementation chooses the former, while the field name can be read either way.

Please also cover stacked/exploded/solo modes, snapLevelsToTruePositions (bake/export), negative offsets, and selection/floorplan behavior with focused tests. Once that contract is stated, the implementation can be reviewed against it.

[mvanhorn]

Contract, stated explicitly: baseElevation is an offset. It shifts this level and every level above it in the same building (cumulative), it is not an absolute Y, and negative values are allowed. Rebased by porting onto the current level-stacking architecture in 92a550e so stacked, exploded, solo, floorplan, and snapLevelsToTruePositions all read it from the stacking computation, with focused tests for each of those plus negative offsets.

[mvanhorn]

All three findings are the same shape, baseElevation applied in one path and ignored in another. Fixed in ac82cf8.

The covering-slab math assumed the floor above was exactly one stored storey height up, so the clamps ignored the offset. Floor-to-floor now comes from the stacked elevations (above.baseY minus current.baseY) through a single helper, so it can't drift from getLevelElevations again. Stair rise had the same bug and uses the same helper.

Elevator level tables and the first-person colliders were building cumulative Y from storey heights; both now read baseY from getLevelElevations. first-person-controls turned out to have its own near-copy of that logic, so it's gone in favour of the shared resolveElevatorLevels.

For the NaN one I fixed it in migration rather than at the control, normalizing baseElevation to 0 next to level and children, with a defensive fallback at the input too.

928 core tests pass, biome clean.

[Aymericr]

Thanks for this, and for coming back to it in August to push fixes for the Bugbot findings — that is more follow-through than most contributions get. Issue #209 is a genuine gap: there is no way to model a staggered house today. The core tests you added to storey.test.ts, stair-rise.test.ts and the new elevator-service.test.ts are the right kind of coverage, and the resolveElevatorLevels dedup is a real improvement.

I reviewed this closely and there are several things to sort out. Taking the mechanical ones first, then the one that actually decides the PR.

The CI failure is not the pre-existing error the description points at

The description blames a pre-existing implicit-any at use-editor.tsx:249. That line is a type alias on main, and tsc -p packages/viewer exits 0 there. The real failure is in a file this PR adds:

src/systems/level/level-system.test.ts(124,3): error TS2349: This expression is not callable.
  Type 'never' has no call signatures

frameCallback is narrowed to never by the reset assignment on line 121. Biome is clean, so it is purely that.

That test does not exercise this change

packages/viewer/src/systems/level/level-system.test.ts is the only viewer file in the diff — level-system.tsx and level-utils.ts are byte-identical to main. And the test calls mock.module('@pascal-app/core', ...) with a hand-written getLevelElevations that reimplements the baseElevation folding itself:

const baseY = (cumulativeYByBuilding.get(level.parentId) ?? 0) + level.baseElevation

So it asserts against the mock, not against your change. Copied onto an unpatched main viewer, all three tests pass.

I would delete the file. The viewer picks the offset up for free because it calls the real getLevelElevations, and your core tests genuinely cover the arithmetic. Deleting it also removes the CI failure.

Relatedly, the description mentions threading through getLevelTargetY / getNextLevelCumulativeY and a test at packages/viewer/test/level-utils.test.ts — none of those exist in the repo or the diff. Worth correcting so the next reader is not hunting for them.

Missing reactivity wiring

getWallPlaneTop and getCeilingClampBound now resolve through getLevelFloorToFloorHeight, which means a level's baseElevation changes the storey below. But spatial-grid-sync.ts:188 still only dirties dependents when node.height !== prev.height, and the diff does not touch that file. Wall rebuild is dirty-gated, so after editing Base elevation the containers restack in 3D while the walls, ceilings and fences below keep stale tops until some unrelated edit rebuilds them. wiki/architecture/vertical-model.md calls this out directly: if a new consumer reads these bounds, wire its dirty rule there. Note this also needs a hop to the level below, since markLevelHeightDependents only walks the changed level's own children.

getWallPlaneTop becomes unbounded

With a level at baseElevation: -5 above a 2.5 m level, getLevelFloorToFloorHeight returns -2.5 and the wall plane top goes negative. Previously the plane was always the stored storey height, so MIN_WALL_HEIGHT and the slab clamp had a positive floor. The new MetricControl has no min, so a user can drag straight into it.

The part that actually decides this

The blocker is not the code, it is that a per-level baseElevation is a permanent addition to the persisted node schema, and it lands in the same getLevelElevations arithmetic we are about to rewrite. Our direction for vertical positioning is to anchor ordinal 0 and put grade in building.position[1] — a transform rather than a second elevation axis — specifically so basements dig down without a new schema field. Split-level wings are deferred behind that datum change on purpose. Merging a second vertical axis first means the datum work has to reconcile two of them. The name also already means something else on WallSlabSupport.baseElevation.

That is our sequencing problem rather than a flaw in your work, and it is not something I want to resolve by having you rewrite this five times.

Where I would like to land

I do not want to just close this after you put in the effort, so concretely:

1. Split out the elevator/first-person dedup — sharing resolveElevatorLevels instead of the near-copy in first-person-controls.tsx stands entirely on its own, is easy to review, and I will merge it quickly. One note when you do: resolveElevatorLevels now calls getLevelElevations(nodes), which iterates and sorts every node in the scene, and it runs per-elevator on a useFrame path. Hoist it out of the loop in stepElevatorRuntimes and pass the map in.
2. Keep this PR open as the reference implementation for split-levels. It is genuinely the clearest statement of what Floor height parameter #209 needs, and when the datum change lands I would rather revisit real code than a description.

I have linked this to #209 so the history stays together. Happy to talk through the field shape whenever you want.

[Aymericr]

Picked this up as part of a maintenance sweep. The gate was red, so I've pushed the fixes to your branch (97c10ca) rather than leaving you a chore list — plus a merge of main, which had moved 28 commits ahead.

What was failing, and why the log was misleading. CI reported a single error:

src/systems/level/level-system.test.ts(124,3): error TS2349: This expression is not callable.
  Type 'never' has no call signatures.

Line 124 is frameCallback?.({}, delta), not the LevelSystem() call above it. The useFrame mock assigns frameCallback while LevelSystem() runs; TypeScript can't see through that indirection, so after the frameCallback = null reset it narrows the binding to null and the later optional call becomes never. Reading it through a small function preserves the declared type. (An explicit annotation and an object wrapper both still narrow — worth knowing if this pattern comes up again.)

The second problem, which the type error was hiding. Because quality fails at the type step, the test step never ran. mock.module('@pascal-app/core', ...) replaces core's entire export surface for the whole test process, and Bun never restores it — so every viewer suite loaded after this file got the fake core:

Run	Before	After
bun test packages/viewer/src/systems	29 pass / 2 fail	47 pass / 0 fail
whole-repo run	32 tests lost	0 fail

The most confusing casualty was wall-support-extension.test.ts → "plane-bound wall under a flush thick deck tops out at the deck underside", which expected 2.2 and got 2.5. That reads exactly like a baseElevation regression in your floorToFloorHeight refactor. It isn't — the file passes alone on your branch, and getWallPlaneTop returns the correct 2.2 when I call it directly. It was the mock leaking.

Core didn't need mocking: sceneRegistry is a real in-memory store with clear(), and useScene is a zustand store with setState. The test now drives both directly and mocks only @react-three/fiber and use-viewer — the two modules that actually need a renderer or a React context. I dropped the lerp mock as well, since the real one is already pure. Same three assertions, same values, now checked against the real elevation code, which is a stronger test than it was.

One more: packages/editor/.../wall-drafting.test.ts needed baseElevation: 0 added to its level fixture. Giving baseElevation a zod default makes it required on LevelNode's output type, so that pre-existing as AnyNode cast stopped holding. Cheap to fix, but worth flagging as a general consequence — a defaulted field is optional on input and required on output, so it can break fixtures in packages your PR never touches.

On the change itself. The storeyHeight → floorToFloorHeight rename in services/storey.ts is the right call. Once a level can carry an offset, "how tall is this storey" and "how far up is the next floor" stop being the same number, and every covering-slab consumer wanted the second one. Deriving it from the next level's baseY rather than adding offsets at each call site means the ceiling clamps, slab undersides and wall plane tops can't drift apart. normalizeLevelBaseElevation handling undefined and NaN in one place is the right shape for legacy data, and the height-stays-optional-on-purpose comment is a good note to have left.

Gates on the merge ref, for the record: check clean, check-types 9/9, test 12/12 tasks / 0 fail, build 7/7.

Nothing outstanding from me. Once CI confirms green here this is ready to land. Thanks for threading the offset through the clamps, the elevator stacking and the migration rather than only the presentation layer — that's the part that would have been painful to retrofit.

[Aymericr]

Pushed 0f479236 — CI should go green now.

What broke

The failure that appeared after my last push wasn't caused by this branch:

(fail) resolveStairTotalRise > includes the next level base elevation in a following stair rise
Expected: 2.1
Received: 2.9

#556 ("stop rebuilding wall geometry every frame") landed between your last green run and mine, and it added an identity-keyed memo to getLevelElevations:

const elevationMemo = new WeakMap<object, Map<string, LevelElevation>>()

export function getLevelElevations(nodes: Record<AnyNodeId, AnyNode>): Map<string, LevelElevation> {
  const memoized = elevationMemo.get(nodes)
  if (memoized) return memoized
  …

CI builds refs/pull/312/merge, so it typechecks and tests your branch merged with current main — which is how a branch that's green locally goes red without a single commit changing on it.

Your new case did this:

expect(resolveStairTotalRise(stair, stackedNodes)).toBeCloseTo(2.9)
stackedNodes.level_2 = { ...upper, baseElevation: -0.4 }
expect(resolveStairTotalRise(stair, stackedNodes)).toBeCloseTo(2.1)   // got 2.9

Same object identity on both calls → memo hit → the second assertion re-read the first call's elevations. Note the failure mode: it returned the previous expected value, so had the numbers happened to coincide the test would have passed while asserting nothing.

Why the fix is in the test, not in storey.ts

I checked whether #556's memo contract actually holds before touching your test. It does: every store write publishes a new record rather than mutating —

// packages/core/src/store/actions/node-actions.ts:989
set((state) => {
  const nextNodes = { ...state.nodes }
  …
  return { nodes: nextNodes }
})

and grepping core, viewer and editor for in-place writes to the store's nodes record (state.nodes[…] = / getState().nodes[…] =) turns up nothing outside test fixtures. So the memo is sound in production and the test was relying on mutation semantics the store never exhibits. The fix builds a fresh record, matching what the sibling case in the same file already does:

const loweredNodes = {
  ...stackedNodes,
  level_2: { ...upper, baseElevation: -0.4 },
} as Record<string, AnyNode>
expect(resolveStairTotalRise(stair, loweredNodes)).toBeCloseTo(2.1)

I also scanned every test file this PR touches for the same pattern — this was the only one.

Gate status on the merge base

Run against main merged in (61cfb475), i.e. what CI actually evaluates:

gate	result
bun run check	1600 files, clean
bun run check-types	9/9 packages
bun run test	944 pass / 0 fail
bun run build	7/7

Plus the two suites turbo run test doesn't reach, because packages/viewer and packages/editor have no test script — worth knowing when you're checking work in those packages locally:

• bun test packages/viewer/src/systems → 63 pass / 0 fail
• bun test packages/editor/src/components/tools/wall → 34 pass / 0 fail

Nothing further needed from you on this one — this is a maintainer-side fix to a cross-PR interaction, not a review request.

[cursor]

Auto ceilings ignore base offsets

High Severity

Auto-room wall tops still come from getStoredLevelHeight, while rendered plane-bound walls now use offset-aware getWallPlaneTop / floor-to-floor spacing. When the level above has a non-zero baseElevation, heightForRoom writes auto ceiling heights that no longer match the actual wall plane, leaving rooms short or tall relative to the staggered stack.

 

^{Reviewed by Cursor Bugbot for commit 0f47923. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

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

[cursor]

Elevator shaft ignores offset safeguard

High Severity

shaftTopY only applies the highest-ceiling safeguard when the elevator serves the top of the stack. If a higher unserved level exists, it uses that level's baseY alone. A negative baseElevation there can place the shaft top below a served ceiling and clip the cab and first-person colliders.

 

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

[Aymericr]

One more push, b96c3b97 — a behavioral edge case I found on the final read-through, in the elevator consolidation.

Removing resolveElevatorColliderLevels from first-person-controls.tsx in favour of the core resolveElevatorLevels is a good call: it was a near-copy that had already drifted. But the two didn't compute the stack top the same way. The editor version took the max:

cumulativeY = Math.max(cumulativeY, baseY + (elevation?.height ?? 0))

whereas the core version reads the topmost level only:

const lastStackedLevel = allLevels[allLevels.length - 1]
const stackTopY = lastStackedLevel
  ? (levelElevations.get(lastStackedLevel.id)?.baseY ?? 0) +
    (levelElevations.get(lastStackedLevel.id)?.height ?? 0)
  : 0

Before this PR those were interchangeable — the top level always had the highest ceiling. baseElevation breaks that: a negative offset can sink the top level's ceiling below the level beneath it, and shaftTopY then lands under a served level and clips the cab.

level_0 height 3, level_1 with baseElevation: -2.5 and height 2 → level_1 sits at 0.5 and tops out at 2.5, so the shaft top comes back as 2.5 where it should be 3 — a half-metre clip at the top of travel. Restored the Math.max in core with a regression test; I verified it fails against the previous line (Received difference: 0.5) and passes with the fix.

Final gate run on the merge base

gate	result
bun run check	1600 files, clean
bun run check-types	9/9
bun run test	1881 pass / 0 fail
bun run build	7/7

Review summary

The design is right: baseElevation folds into getLevelElevations' cumulative sum, so an offset shifts its level and everything above it in the same stack, and getLevelFloorToFloorHeight derives spacing from the two floors rather than the stored height. That's what makes the stair rise and the slab/ceiling clamps follow the offset without any of them needing to know about it. normalizeLevelBaseElevation in migrateNodes keeps legacy scenes at a finite zero, and the deliberate contrast with height (no zod default, absence gates the migration) is documented in the schema. Coverage lands on the cases that matter — positive and negative offsets, the covering plane, wall and ceiling clamps, legacy normalization, and stacked level positions in the viewer.

Merging.