feat(viewer): per-level base elevation parameter - #312
Conversation
|
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. |
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 pascalorg#209
9d14a64 to
92a550e
Compare
|
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. |
…r-height-parameter # Conflicts: # packages/core/src/schema/nodes/level.ts # packages/viewer/src/systems/level/level-stacking.test.ts # packages/viewer/src/systems/level/level-stacking.ts # packages/viewer/src/systems/level/level-system.tsx # packages/viewer/src/systems/level/level-utils.ts
…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.
|
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. |
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 ac82cf8. Configure here.
| floorToFloorHeight: | ||
| resolveLevelFloorToFloorHeight(levelId, elevations) ?? | ||
| getStoredLevelHeight(level as LevelNode), | ||
| slabs, |
There was a problem hiding this comment.
Base elevation skips geometry rebuild
High Severity
Walls and ceilings now derive their plane from offset-aware floor-to-floor spacing via getLevelFloorToFloorHeight, so a level’s baseElevation changes the storey below. spatial-grid-sync still only dirties dependents when height changes, not baseElevation, so editing Base elevation moves the stack in 3D while walls, ceilings, and fences on the level below keep stale geometry until something else rebuilds them. Stairs happen to update because StairOpeningSystem already reacts to any level node change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac82cf8. Configure here.


What does this PR do?
Adds a per-level
baseElevationparameter so each level can offset its computed stack position upward by a user-specified amount. The site panel surfaces aBase elevationMetricControl on each level (m, 2dp, 0.05 step) and the viewer's level system threads the offset throughgetLevelTargetY/getNextLevelCumulativeYhelpers.How to test
Base elevationfield to a non-zero meter value.EXPLODED_GAPstill adds on top of the cumulative offset.bun test packages/core/test/level.test.ts packages/viewer/test/level-utils.test.ts.Notes
check-typesflags a pre-existing TypeScript error on the base branch unrelated to this change (packages/editor/src/store/use-editor.tsx:249—childIdimplicit any). The error is present onorigin/mainbefore this branch is applied, so the failure isn't introduced by this PR. Happy to fix it in a sibling PR if useful.Checklist
bun devand exercised the change locallybun check(biome clean)bun check-typesfails on pre-existing error (see Notes)mainCloses #209
Note
Medium Risk
Changes core vertical stacking used by walls, ceilings, stairs, and elevators across the app; behavior is well covered by tests but mistakes would skew building geometry globally.
Overview
Adds
baseElevationon level nodes: an additive meter offset applied on top of the per-building stack. A level’s offset shifts that floor and every higher floor in the same building (negative values allowed). Legacy scenes getnormalizeLevelBaseElevationon load so missing/invalid values become 0.getLevelElevationsnow folds offsets intobaseY. Vertical helpers that used only stored storey height switch togetLevelFloorToFloorHeight(distance to the floor above), so wall plane tops, ceiling clamps, and covering-slab undersides stay correct when offsets change floor spacing. Stairs without an explicittotalRiseuse that floor-to-floor height; elevators and editor colliders use the shared elevation map viaresolveElevatorLevels.The site panel exposes a Base elevation control per level. Tests cover schema, storey math, migration, stairs, elevators, and viewer level presentation.
Reviewed by Cursor Bugbot for commit ac82cf8. Bugbot is set up for automated code reviews on this repo. Configure here.