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
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 92a550e. Configure here.
| precision={2} | ||
| step={0.05} | ||
| unit="m" | ||
| value={Math.round(level.baseElevation * 100) / 100} |
There was a problem hiding this comment.
Legacy levels show NaN elevation
Medium Severity
The new Base elevation control uses Math.round(level.baseElevation * 100) / 100 without a fallback. Levels loaded from older project JSON can omit baseElevation because scene migration only normalizes level and children, not this field, so the control receives NaN even though stacking treats missing values as zero.
Reviewed by Cursor Bugbot for commit 92a550e. Configure here.
| // cumulatively, every level above it in the same building. Negative offsets are valid. | ||
| const levelY = baseY + (entry.baseElevation ?? 0) | ||
| positions.set(entry.levelId, levelY) | ||
| cumulativeYByBuilding.set(entry.buildingId, levelY + entry.height) |
There was a problem hiding this comment.
Elevator stack ignores base elevation
Medium Severity
Per-level baseElevation now shifts stacked level meshes via getLevelStackPositions, but elevator level tables and first-person elevator colliders still build cumulative Y from story heights only. Non-zero offsets leave cab stops and shaft geometry out of sync with the floors users see in the editor.
Reviewed by Cursor Bugbot for commit 92a550e. Configure here.
|
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. |
2 participants
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 3D level positioning used by floorplan, selection, and solo/exploded modes; behavior is well-tested but mis-stacked levels could affect downstream spatial presentation.
Overview
Adds a per-level
baseElevation(meters, default0) so users can shift a level’s stacked Y position; the offset is additive and cumulative—it moves that level and every higher level in the same building, with negative values allowed.The site panel exposes Base elevation on each expanded level via
MetricControl(0.05 m steps, 2 dp).getLevelStackPositionsnow appliesbaseElevationwhen computing stack Y;LevelSystemandsnapLevelsToTruePositionspass the field through so stacked, exploded, solo, and floorplan snap behavior stay consistent.Schema tests cover defaults and parsing; stacking and level-system tests cover multi-building offsets, negatives, and presentation modes.
Reviewed by Cursor Bugbot for commit 92a550e. Bugbot is set up for automated code reviews on this repo. Configure here.