feat(wall): add endHeightOffset for sloped top edge - #640
Conversation
cf8f12c to
7200ea6
Compare
7200ea6 to
8910206
Compare
8910206 to
6a53abd
Compare
6a53abd to
f250e10
Compare
f250e10 to
d2b2045
Compare
d2b2045 to
a51d552
Compare
8cedc3a to
bda880e
Compare
bda880e to
4eb816b
Compare
b95898c to
0819b93
Compare
0819b93 to
4a67a86
Compare
4a67a86 to
81790a5
Compare
81790a5 to
e48a086
Compare
e48a086 to
e7b4a53
Compare
e7b4a53 to
16d9557
Compare
Aymericr
left a comment
There was a problem hiding this comment.
First, an apology. You've been iterating against Bugbot for six days with no human in the loop, and the quality of those replies deserved better. Your rebuttal on the miter clamp is correct: evaluating the slope continuously past [0, L] is what keeps the top face coplanar, and clamping would fold the corner. I also checked that your last commit fixed four of the five findings Bugbot raised after your last reply (window sill, door clamp past wall end, door fit search range, width max below min) — Bugbot never re-ran, so the PR looks more open than it is.
I want this capability. I need to sort out one product question and a handful of code items before it lands.
The product question — how this relates to #260. When I scoped that issue I split "sloped walls" into a gable-end wall whose top follows the roof plane above it, and a battered wall that leans off vertical. This PR is a third thing: a manually-dialled linear tilt. That's the right primitive for a shed-roof knee wall, and I think the geometry work here is reusable for the roof-driven version. My hesitation is shipping the manual slider as the user-facing answer, because endHeightOffset doesn't know about the roof — #651 (lean-to extensions) and #690 (roof intersections) just landed, and a hand-set offset silently desyncs the moment the roof pitch changes. The roof already exposes getRoofOuterSurfaceFrameAtPoint, so deriving the wall top from the roof above it is reachable.
So: could we land the primitive without the slider? Schema field, resolveWallTop support, geometry, opening clearance — all of it — and then let a follow-up drive endHeightOffset from the roof plane. If you'd rather keep the manual control, make the case and I'll listen; I mostly don't want to ship a control users can put into permanent disagreement with the roof over their head. Either way this doesn't close #260 (that also covers sloped slabs, untouched here), so let's reference it rather than close it.
Please rebase first. You branched from #673 and main is 45 commits ahead. It merges cleanly, but three of those commits change your assumptions:
- #608 sewed each level's wall set into one merged mesh, and
assignWallMaterialGroupsbecamesetGroupsSortedByMaterialbecause the batch needs material-major layout. Your slope mutates positions before grouping, so I think it carries into the merged mesh — butgetWallBandSplitPlanesnow emits planes up tomaxWallHeight, and a plane above the actual top at the start end can produce degenerate or extra material runs in a batch. Please verify on a level with several sloped walls, and check #687's hover-out-of-batch outline on one. - #702 tightened how a slab-sourced wall draft elects its base. You rewrote the
supportSlabId === 'ground'branch ofresolveWallTopwithout those tests in your branch — re-runwall-drafting.test.tsafter the rebase. - #721 lifted arbitrary maxes from inspector dimension fields and wrote up the convention in
wiki/architecture/inspector-field-limits.md. Your new slider'smax: 3is the pattern it removed; the doc says offsets get no static bound. Drop the max (keep the min — a host-derived validity floor is explicitly fine).
Code items:
tshould not be optional.resolveWallTop(wall, storeyHeight, base)withtomitted silently returns the start height, so every un-updated caller reports a sloped wall as flat. Six onmainweren't updated:wall-cutout.tsx:237(opening cutouts and face bands),zone-quantities.ts:485(wall-area takeoffs — wrong quantities, no warning),mcp/scene-query.ts:142(the height an AI agent reads, while your schema doc now advertises the slope to that agent), plus thestorey.tsdoc comments that no longer describe the function. Either maketrequired, or addresolveWallTopAt(wall, …, t)and haveresolveWallTopdelegate att = 0. Both turn those six into compile errors, which is what I want here.wall-top.test.tsneeds to grow. It has 17 references to the function you changed and the PR doesn't touch it. The door/window math tests are good; the core primitive needs the same treatment — the negative-offset clamp,t = 0/t = 1/ midpoint, and the plane-bound (height == null) case with a slope.- Solve the fit analytically. Both
clampToWalls scan in 0.1m steps up towallLength / 2, tworeadHostWallCeilingcalls per step, each resolving the elected slab support — that's ~100 support resolutions per pointer move on a 10m wall, inside a drag handler. The ceiling is linear int, so the interval where an opening fits is closed-form. Same forreadHostWallCeilingMaxWidth's 15-iteration binary search. - One clamp, one place.
Math.max(rawOffset, -(h - 0.01))appears inwall-top.ts,wall-system.tsxandtreatments.tsxwith three differenthvalues. Export one helper fromwall-top.tsand call it from all three — that's the actual fix for the divergence Bugbot flagged, rather than a fourth clamp. - Deduplicate the scene-reader adapter. The
typeof (sceneOrNodes as WallCeilingSceneReader).nodes === 'function' ? …block is copy-pasted intodoor-math.tsandwindow-math.ts. OnetoSceneReaderinwall-opening-ceiling.ts. - Drop the
readWallLengthchange, or guard it.door.wallId || door.parentIdthrows onwall.end[0]when the parent isn't a wall andreadRoofFaceWidthMaxreturned null. If it's a real bug, it's worth its own small PR.
The chord-projected t for curved walls and sampling opening clearance at span edges rather than centers are both the harder-and-correct choices, and I'd have accepted worse. Answer the roof question and I'll review the next push quickly — you won't be waiting six days again.
16d9557 to
974a289
Compare
- **Schema & Core Systems**:
- Add endHeightOffset property to WallNode schema with default 0.
- Update resolveWallTop and resolveWallOpeningCeiling to support
parametric height sampling along sloped top edges.
- Update getWallFaceBandConfig and getWallFaceBandForHeight to
scale with maximum sloped wall height.
- Update spatial grid synchronization, space detection, and level
height calculations for sloped walls.
- **3D Geometry & Viewer Systems**:
- Add applyWallEndHeightSlope in wall-system to evaluate planar slope
continuously across miter extensions without creases.
- Update getWallBandSplitPlanes to split horizontal face bands across
the full sloped wall envelope.
- Update treatments to clamp slope offsets against minimum height,
omit trims on low walls, and make cutouts slope-aware.
- **Openings & Floorplan Tools**:
- Update door and window ceiling clearance math, floorplan move
handlers, and 3D move tools to respect sloped wall height.
- Fix resolveWallRole in paint to resolve face bands by elevation Y,
ensuring 1:1 parity with 3D mesh horizontal band splits.
- Update 3D opening head clearance guides to sample minimum ceiling
across the opening width.
- Update elevation guides, measurement features with chord-projected
t for curved walls, side move handles, and snap beacons.
974a289 to
d19ca3c
Compare
d19ca3c to
73902b5
Compare
73902b5 to
1195d3f
Compare
1195d3f to
31ab927
Compare
31ab927 to
763d38a
Compare
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.
Reviewed by Cursor Bugbot for commit 763d38a. Configure here.
763d38a to
4b7e5b0
Compare
|
Hi @Aymericr,
Thanks for the review and feedback!
1. The Product Question (Manual Slider vs. Automatic): To be
completely honest, my initial goal was to make walls automatically
slope under roofs, but I failed to do so because I had no idea how to.
That's why I added the UI slider as a practical solution.
In addition, there is a very common architectural use-case where
automatic roof detection cannot help: walls under staircases / stair
enclosures (as well as custom knee walls, stepped parapets, and
landscaping walls) where there is no roof above the wall. Keeping the
manual slider gives users immediate control for these cases, while
leaving the door wide open for follow-up roof-driven automation.
2. Code Review Items: All requested items have been addressed in the branch:
. Ready for your review!
Le ven. 28 août 2026 à 05:45, Aymeric Rabot ***@***.***> a écrit :
…
@Aymericr requested changes on this pull request.
First, an apology. You've been iterating against Bugbot for six days with no human in the loop, and the quality of those replies deserved better. Your rebuttal on the miter clamp is correct: evaluating the slope continuously past [0, L] is what keeps the top face coplanar, and clamping would fold the corner. I also checked that your last commit fixed four of the five findings Bugbot raised after your last reply (window sill, door clamp past wall end, door fit search range, width max below min) — Bugbot never re-ran, so the PR looks more open than it is.
I want this capability. I need to sort out one product question and a handful of code items before it lands.
The product question — how this relates to #260. When I scoped that issue I split "sloped walls" into a gable-end wall whose top follows the roof plane above it, and a battered wall that leans off vertical. This PR is a third thing: a manually-dialled linear tilt. That's the right primitive for a shed-roof knee wall, and I think the geometry work here is reusable for the roof-driven version. My hesitation is shipping the manual slider as the user-facing answer, because endHeightOffset doesn't know about the roof — #651 (lean-to extensions) and #690 (roof intersections) just landed, and a hand-set offset silently desyncs the moment the roof pitch changes. The roof already exposes getRoofOuterSurfaceFrameAtPoint, so deriving the wall top from the roof above it is reachable.
So: could we land the primitive without the slider? Schema field, resolveWallTop support, geometry, opening clearance — all of it — and then let a follow-up drive endHeightOffset from the roof plane. If you'd rather keep the manual control, make the case and I'll listen; I mostly don't want to ship a control users can put into permanent disagreement with the roof over their head. Either way this doesn't close #260 (that also covers sloped slabs, untouched here), so let's reference it rather than close it.
Please rebase first. You branched from #673 and main is 45 commits ahead. It merges cleanly, but three of those commits change your assumptions:
#608 sewed each level's wall set into one merged mesh, and assignWallMaterialGroups became setGroupsSortedByMaterial because the batch needs material-major layout. Your slope mutates positions before grouping, so I think it carries into the merged mesh — but getWallBandSplitPlanes now emits planes up to maxWallHeight, and a plane above the actual top at the start end can produce degenerate or extra material runs in a batch. Please verify on a level with several sloped walls, and check #687's hover-out-of-batch outline on one.
#702 tightened how a slab-sourced wall draft elects its base. You rewrote the supportSlabId === 'ground' branch of resolveWallTop without those tests in your branch — re-run wall-drafting.test.ts after the rebase.
#721 lifted arbitrary maxes from inspector dimension fields and wrote up the convention in wiki/architecture/inspector-field-limits.md. Your new slider's max: 3 is the pattern it removed; the doc says offsets get no static bound. Drop the max (keep the min — a host-derived validity floor is explicitly fine).
Code items:
t should not be optional. resolveWallTop(wall, storeyHeight, base) with t omitted silently returns the start height, so every un-updated caller reports a sloped wall as flat. Six on main weren't updated: wall-cutout.tsx:237 (opening cutouts and face bands), zone-quantities.ts:485 (wall-area takeoffs — wrong quantities, no warning), mcp/scene-query.ts:142 (the height an AI agent reads, while your schema doc now advertises the slope to that agent), plus the storey.ts doc comments that no longer describe the function. Either make t required, or add resolveWallTopAt(wall, …, t) and have resolveWallTop delegate at t = 0. Both turn those six into compile errors, which is what I want here.
wall-top.test.ts needs to grow. It has 17 references to the function you changed and the PR doesn't touch it. The door/window math tests are good; the core primitive needs the same treatment — the negative-offset clamp, t = 0 / t = 1 / midpoint, and the plane-bound (height == null) case with a slope.
Solve the fit analytically. Both clampToWalls scan in 0.1m steps up to wallLength / 2, two readHostWallCeiling calls per step, each resolving the elected slab support — that's ~100 support resolutions per pointer move on a 10m wall, inside a drag handler. The ceiling is linear in t, so the interval where an opening fits is closed-form. Same for readHostWallCeilingMaxWidth's 15-iteration binary search.
One clamp, one place. Math.max(rawOffset, -(h - 0.01)) appears in wall-top.ts, wall-system.tsx and treatments.tsx with three different h values. Export one helper from wall-top.ts and call it from all three — that's the actual fix for the divergence Bugbot flagged, rather than a fourth clamp.
Deduplicate the scene-reader adapter. The typeof (sceneOrNodes as WallCeilingSceneReader).nodes === 'function' ? … block is copy-pasted into door-math.ts and window-math.ts. One toSceneReader in wall-opening-ceiling.ts.
Drop the readWallLength change, or guard it. door.wallId || door.parentId throws on wall.end[0] when the parent isn't a wall and readRoofFaceWidthMax returned null. If it's a real bug, it's worth its own small PR.
The chord-projected t for curved walls and sampling opening clearance at span edges rather than centers are both the harder-and-correct choices, and I'd have accepted worse. Answer the roof question and I'll review the next push quickly — you won't be waiting six days again.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|

What does this PR do?
Adds an
endHeightOffsetproperty to theWallNodeschema to allow wall extremities to have different heights (e.g., for creating a knee wall following a single-pitch roof slope). This adds an "End height offset" slider to the wall properties panel and updates the geometry generation to apply the slope along the top edge of the wall.Relates to #260 (sloped elements).
How to test
End height offsetslider.Screenshots / screen recording
Checklist
bun devbun checkto verify)mainbranchNote
Medium Risk
Touches core wall height resolution, viewer extrusion/CSG, and door/window placement across many packages; behavior changes for sloped walls even when offset is zero due to new
tparameters and placement rules.Overview
Adds
endHeightOffseton walls so the top edge can slope linearly from start to end (knee walls / single-pitch roofs), with an End height offset slider on the wall panel andclampWallEndHeightOffsetso the low end never collapses below 0.01 m.resolveWallTopandresolveWallEffectiveHeightnow take parametrictalong the wall; downstream systems (level height, room ceiling detection, spatial-grid wall placement, face bands, crown trim slope/cuts, viewer extrusion viaapplyWallEndHeightSlope, opening CSG caps) sample height along the span instead of assuming a flat cap.Doors and windows get sloped-ceiling-aware
clampToWall(fit flag), resize limits (readHostWallCeiling/readHostWallCeilingMaxWidth), and placement guides; wall-hosted move tools drop grab-offset anchoring in favor of cursor-centered sliding. Editor overlays (measurements, move handles, snap highlights, elevation guides) and snap context while moving doors/windows are updated to match.Reviewed by Cursor Bugbot for commit 4b7e5b0. Bugbot is set up for automated code reviews on this repo. Configure here.