viewer: Fix Dutch roof trim artifacts#452
Conversation
Items (e.g. solar panels) can now be placed on sloped roof surfaces. The placement system computes euler rotation from the roof surface normal so items sit flush on the slope instead of going inside. - Add roofStrategy to placement-strategies with enter/move/click/leave - Wire roof:enter/move/click/leave events in the placement coordinator - Add calculateRoofRotation in placement-math using surface normals - Support full 3D cursor rotation for sloped surfaces - Items on roofs are parented to the level with world-space rotation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove Dutch ridge axis abstraction and rework roof edit system, ridge vent clipping geometry, and roof surface placement. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Track ridge vent auto-generation via an `autoRidgeVent` metadata flag so geometry changes only regenerate default vents when enabled, treating legacy segments with generated vents as auto-enabled for back-compat. Expose a panel toggle to opt in/out per segment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Child meshes relied on a parent group's layer, which three.js does not propagate, so the trim section/rail/plane overlays rendered on the scene layer — getting inked/SSGI-darkened and leaking into thumbnail exports. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # packages/editor/src/components/editor/floorplan-panel.tsx # packages/editor/src/components/tools/roof/roof-tool.tsx # packages/editor/src/components/tools/tool-manager.tsx # packages/editor/src/components/ui/helpers/helper-manager.tsx # packages/editor/src/index.tsx # packages/editor/src/store/use-editor.tsx # packages/nodes/src/fence/definition.ts # packages/nodes/src/wall/tool.tsx
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Architecture review
The PR is overwhelmingly clean on the structural rules — layering, registry composition, schema migration, interaction-scope design, and layer tags all check out. The violations cluster entirely in the new fence-spline reshape feature and one roof-tool snap migration, and they all trace to a single rule: snapping is mode-driven, not a held-modifier bypass.
Reviewed against
origin/main...HEAD(163 files). Note: a localmaincheckout will be misleading here — it's 219 commits behindorigin/main, sogit diff main...HEADreports ~1041 files.
🚫 Blockers
B1 — Shift/Alt used as a snap bypass (regresses the mode-driven snapping convention)
Convention (wiki/architecture/interaction-scope.md §"Snapping mode & modifiers", tools.md §"Snapping is mode-driven"): Shift cycles the snap mode, Alt = force/free; snap state must come from isGridSnapActive() / isMagneticSnapActive() / isAngleSnapActive() — never event.shiftKey. Wall, straight-fence, zone, and slab tools are already migrated to this; these new paths regress it. Five sites:
-
packages/nodes/src/fence/tool.tsx:727— newSplineFenceDraft:if (shiftPressed.current) return local // Shift = snap bypass
The
StraightFenceToolin the same file documents "'off' is the bypass — there is no Shift hold-to-bypass." Fix: gate grid viagetSegmentGridStep()(already returns 0 when grid mode is off), route magnetic/angle through the mode helpers. -
packages/nodes/src/fence/move-control-point-tool.tsx:80& 3.packages/nodes/src/fence/move-tangent-tool.tsx:83:const bypass = shiftPressed || event.nativeEvent?.shiftKey === true
These run inside a
reshapingscope, so a snap context resolves — readisGridSnapActive()/isMagneticSnapActive()instead. The siblingfence/actions/move-endpoint.tsalready does this correctly ("No Shift bypass"). -
packages/nodes/src/fence/floorplan-affordances.ts:50-51,101-102(2D control-point + tangent):const x = modifiers.shiftKey ? planPoint[0] : snapScalarToGrid(planPoint[0], snapStep)
-
packages/editor/src/components/tools/roof/roof-tool.tsx:266-267— forwards raw flags into the shared snap pipeline:shiftKey: event.nativeEvent?.shiftKey === true, altKey: event.nativeEvent?.altKey === true,
surface-plan-snap.ts:175treatsshiftKeyas a full snap bypass and:216treatsaltKeyas an alignment bypass. This is a regression: the pre-PR roof-tool read these flags zero times (its own comment at:246still says "this tool never inspects the flags"), relying only onisGridSnapActive()/isMagneticSnapActive().
Nuance: the underlying pipeline's flag-bypass is tracked legacy debt (shared with the slab/ceiling callers); the clean fix is to stop forwarding the flags here (and ideally migrate the shared pipeline), not extend it onto a previously-clean tool.
B2 — Per-tick useScene.updateNode during drag in the new fence 3D reshape tools
packages/nodes/src/fence/move-control-point-tool.tsx:57 and move-tangent-tool.tsx:57,102 call useScene.getState().updateNode(fenceId, …) inside onGridMove (every pointer tick). Per tools.md §"Data-driven live drag", a data-driven kind (fence re-meshes from path/tangents) must preview via useLiveNodeOverrides and write the scene once on commit — per-tick updateNode swaps the nodes map ref and re-renders every useScene(s => s.nodes) subscriber each frame.
This is sharpened by a 2D↔3D parity gap (now an explicit rule in CLAUDE.md): the 2D sibling floorplan-affordances.ts does this correctly (useLiveNodeOverrides.set per tick → updateNodes once on commit), while the 3D tools don't. Nuance: the existing fence endpoint move also writes per-tick, so fence-as-a-kind isn't yet migrated to overrides — the clean fix is a fence-wide migration, and these new tools should follow the 2D sibling rather than the legacy endpoint path.
💡 Suggestions
S1 — Leftover diagnostic logging in a hot path
packages/nodes/src/wall/tool.tsx:608-609 — // TEMP DIAGNOSTIC console.log('[wall-snap]', …) fires on every onMove tick while drafting. Self-marked TEMP; please remove before merge. The wall-snap logic itself is correct (uses isMagneticSnapActive() + angle-lock, no modifier bypass).
S2 — Curved fence shouldn't be reached by cycling the continuation mode
SplineFenceDraft is mounted off continuationByContext.fence === 'curved', i.e. "curved" is reached by cycling the same fence continuation/chain state that straight fence uses. Curved-vs-straight is a different drafting interaction (control points + tangent handles + spline reshape scopes vs. straight segment chaining), and overloading the continuation cycle — whose job is "how the next segment chains" — makes the choice non-discoverable and order-dependent. This doesn't have to be a separate tool: keeping it in the same fence tool is fine, but the straight/curved choice should be its own explicit shortcut/toggle, distinct from the continuation/chain cycle.
✅ Verified compliant (no action)
packages/core— no Three.js/viewer/editor/nodes imports; all schema additions (roof-segment.trim, Dutch fields,fence.path/tangents) are.default()/.optional(), andridge-vent.materialPresetis widenedstring→optional(old'preset-white'scenes still parse) — load-safe, no migration gap. No host-kind-shaped capabilities.packages/viewer— viewer isolation clean; the large Dutch-roof / ridge-vent / trim rework delegates all shape math to core helpers and adds no new kind switches or editor vocabulary;roofAccessoryuse is the documented tech debt. No new untagged overlay primitives.packages/editor— every new overlay primitive (fence-tangent-lines-3d,wall-snap-beacon-layer,alignment-3d-guide-layer,cursor-sphere,roof-edit-system) setslayers={EDITOR_LAYER}; the newcontrol-point/tangentreshapes are added as sub-states of the existingreshapinginteraction scope (not newuseEditorflags) — exemplary; no whole-Map selector subscriptions added; no@pascal-app/nodesimport.
Summary
- Blockers: 2 classes / 7 sites — B1 Shift/Alt snap-bypass regression (fence spline ×4 + roof-tool ×1), B2 per-tick
updateNodein fence 3D reshape tools (×2). - Suggestions: 2 — leftover
console.log; curved fence selected via its own shortcut/toggle rather than by cycling the continuation mode (can stay in the same fence tool).
Verdict: needs changes. The structural architecture is sound; all blockers are localized to the new fence-spline reshape feature plus the roof-tool snap migration, and all trace to one rule — snapping is mode-driven, not a held-modifier bypass.
🤖 Architecture review via the review-architecture skill.
| const isOpeningMoveActive = movingOpeningType !== null | ||
| const isOpeningPlacementActive = isOpeningBuildActive || isOpeningMoveActive | ||
| const isFenceBuildActive = phase === 'structure' && mode === 'build' && tool === 'fence' | ||
| const fenceContinuation = useEditor((state) => state.continuationByContext.fence) |
There was a problem hiding this comment.
Unused fence continuation selector
Low Severity
fenceContinuation is subscribed from the editor store but never read in FloorplanPanel, so it triggers extra re-renders without affecting fence build or continuation behavior.
Reviewed by Cursor Bugbot for commit 0620d9d. Configure here.
| ], | ||
| } as AnyNode | ||
|
|
||
| return nextVents.map((vent) => vent.id as AnyNodeId) |
There was a problem hiding this comment.
Default vent refresh drops materials
Medium Severity
When a roof-segment geometry field triggers default ridge-vent refresh, existing generator vents are deleted and replaced with newly parsed nodes. Paint or material choices on those default vents are not carried over, so a width or pitch tweak can silently reset customized vent appearance.
Reviewed by Cursor Bugbot for commit 0620d9d. Configure here.
…aft start Remove the leftover TEMP DIAGNOSTIC console.log in the wall tool's onMove hot path. Curved fences commit on a closing gesture (double-click / Enter) rather than per-click, so surface a 'Finish curve' hint in the fence HUD — but only once a point has been placed and a curve is actually in flight. The draft point count is published from SplineFenceDraft into a small ephemeral editor store (useFenceCurveDraft) that the contextual helper reads, mirroring the existing useSegmentDraftChain pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1d20a74. Configure here.
| start, | ||
| end, | ||
| path: translatePath(snapshot.path, dx, dz), | ||
| } |
There was a problem hiding this comment.
Linked spline fence endpoints desync
High Severity
When a corner-linked fence is moved on the floorplan, projectLinked shifts the whole path by the drag delta but leaves start/end at the old coordinates whenever that endpoint is not tied to the mover’s corner. Committed nodes then disagree with their spline, breaking rendering, handles, and length math.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1d20a74. Configure here.
| const zProgressDenom = Math.max(0.0001, node.depth / 2 - metrics.waistHalfZ) | ||
| const xProgress = Math.max(0, Math.abs(localX) - waistHalfX) / xProgressDenom | ||
| const zProgress = Math.max(0, Math.abs(localZ) - metrics.waistHalfZ) / zProgressDenom | ||
| return node.wallHeight + lowerRise * (1 - Math.min(1, Math.max(xProgress, zProgress))) |
There was a problem hiding this comment.
Dutch rake height normal mismatch
Medium Severity
Dutch getRoofSegmentSurfaceY now treats the gablet rake band using upperHalfX / upperHalfZ, but getAnalyticalNormal still classifies the upper slope only out to the inner waist. Accessories that seat with the analytical normal (e.g. box vents) get the wrong tilt on rake even though height follows the updated surface.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1d20a74. Configure here.
wass08
left a comment
wass08
left a comment
There was a problem hiding this comment.
Blockers from the earlier review are resolved (mode-driven snapping across the fence spline tools + roof-tool, fence reshape now previews via useLiveNodeOverrides and commits once, curved-fence selected via the T shortcut). Pushed two follow-up cleanups: removed the leftover [wall-snap] diagnostic log and gated the curved-fence "Finish curve" hint on a draft being in progress. ci + quality green. 🟢
2 participants
What does this PR do?
Fixes the roof trim workflow around Dutch roof geometry: the trim cutaway now follows the actual roof shell instead of projecting into empty space, Dutch rake/trim materials preserve the roof material during trimming, ridge vent placement and clipping behavior is cleaned up, and roof accessories participate correctly in trim clipping. This branch also includes related roof shape/default updates, floorplan trim helpers, snapping improvements, spline fence work, and Biome cleanup after the main-branch merge.
How to test
bun install, thenbun dev, and openhttp://127.0.0.1:3002.bun run check-types,bun run check, andbun run build.Screenshots / screen recording
Visual change; recording will be added separately.
Checklist
bun devbun checkto verify)mainbranchNote
High Risk
Large changes to roof mesh generation, trim bounds, and scene node updates (ridge vent regeneration) affect rendering and saved scenes; spline fence schema adds new persisted fields.
Overview
This PR overhauls roof segments for Dutch and complex shapes: new
trimfields (including diagonal corners),getRoofSegmentVisibleTopBounds, and a dedicatedroof-segment-shapemesh builder with improved Dutch end slopes, gablet rake, and surface-height math. Ridge vents are generated from roof geometry (gable/hip/Dutch/mansard), can auto-refresh when segment dimensions change (but not on trim-only edits), and default vents inherit roof material instead of forced white preset.Spline fences gain optional
path/tangentson the schema, core sampling helpers, handle APIs for control points and tangents, and 3D tangent-line overlay plus floorplan interaction scopes.The editor shifts floorplan snapping to mode-driven magnetic/grid behavior (removing Shift/Alt bypass in many paths), aligns 2D/3D guide colors, treats
pipe-trapas an MEP sub-tool, and hides arc-curve on spline fences. Smaller fixes: integrated spiral stair slab openings no longer add a separate rectangular landing hole; Biome drops an unused nursery rule.Reviewed by Cursor Bugbot for commit 1d20a74. Bugbot is set up for automated code reviews on this repo. Configure here.