From 15d36c12b12e8dcbda791dc0558a7f3aeb6bb663 Mon Sep 17 00:00:00 2001 From: Wassim SAMAD Date: Sat, 27 Jun 2026 21:29:50 -0400 Subject: [PATCH] fix(editor): zone drafting respects the snapping mode + shows its HUD chip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two zone fixes. helper-manager: the contextual HUD only rendered for tools with `def.toolHints`, so zone (none) showed no HUD and no snapping chip even though it resolves a snap context. Render the generic RegisteredToolHelper whenever the tool has hints OR a snap/continuation context; hoist the legacy `roof` RoofHelper above it so the new fallback doesn't capture it. Any snappable hint-less draft tool now advertises Shift = cycle. zone-tool: a not-yet-migrated legacy tool — it used Shift as a snap bypass and applied `gridSnapStep` unconditionally, so Off mode still snapped to grid. Migrated to the mode-driven exclusive-modes convention (zone resolves to the 'wall' context): grid quantize gated on isGridSnapActive(), 15° ray gated on isAngleSnapActive(), Off/Lines leave the raw cursor. Dropped the Shift-bypass and its key listeners — Shift now cycles the mode globally. Recorded as migrated in the review skill's known-legacy list. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/review-architecture/SKILL.md | 2 +- .../src/components/tools/zone/zone-tool.tsx | 62 +++++++------------ .../components/ui/helpers/helper-manager.tsx | 23 ++++--- 3 files changed, 37 insertions(+), 50 deletions(-) diff --git a/.agents/skills/review-architecture/SKILL.md b/.agents/skills/review-architecture/SKILL.md index fbbf282b..923c36da 100644 --- a/.agents/skills/review-architecture/SKILL.md +++ b/.agents/skills/review-architecture/SKILL.md @@ -148,7 +148,7 @@ Apply when the diff touches a tool, a `move-tool` / `selection` / endpoint / res - **Snappable kinds declare `snapProfile`.** A kind whose tool snaps but whose `NodeDefinition` omits `snapProfile` (`'item' | 'structural'`) gets no contextual chip and the wrong default mode-set — flag it (suggestion, blocker if it ships a bespoke per-kind snapping switch instead). - **Bespoke movers must not open a `moving` scope.** `useMovingNode()` reads the scope, and `tool-manager` mounts the generic `MoveRegistryNodeTool` whenever it's non-null. A bespoke `move-tool.tsx` that calls `begin(movingScope(...))` or `setMovingNode(node)` re-creates the dual-path double-handling (FPS collapse / teleport on move). **Blocker.** Mode-driven snapping inside a bespoke mover must resolve the mode without a global `moving` / `reshaping` scope (see `interaction-scope.md` § "Snapping mode & modifiers"). - **`event.altKey` is not an alignment bypass.** A drafting/preview path that reads `event.altKey` to suppress Figma-alignment is a **blocker** in any **new** or **touched** tool — alignment follows the magnetic snap mode (`bypass: !isMagneticSnapActive()`). Alt is force/free for placement/move; it is **not** a snap/alignment modifier. The one sanctioned Alt use outside force is the **wall/fence chain-mode toggle** (clean Alt-tap → `cycleWallChainMode` / `cycleFenceChainMode`, via `hooks/use-keyboard.ts` `isChainModeContext()`), allowed only because wall/fence drafting has no force role. Grep tell: `event.altKey` near an `align` / `bypass` expression in a `tool.tsx` / floorplan preview path. -- **Known-legacy exceptions (migrate on touch).** Tracked debt in `plans/editor-placement-interaction-overhaul.md`; a PR that **touches** one must migrate it, not extend it; a **new** tool on either legacy pattern is a blocker regardless. (1) `shiftKey` snap-bypass in the MEP move/endpoint tools (`packages/nodes/src/{duct-segment,pipe-segment,liquid-line,lineset,duct-fitting}/{move-tool,selection}.tsx`). (2) `altKey` alignment-bypass in the roof / polygon / slab pointer-move previews (`components/editor/floorplan-panel.tsx`) and the `resolveSlabPlanPointSnap` / `resolveCeilingPlanPointSnap` paths. **Already migrated — do not regress:** wall + fence drafting (both modifier patterns). +- **Known-legacy exceptions (migrate on touch).** Tracked debt in `plans/editor-placement-interaction-overhaul.md`; a PR that **touches** one must migrate it, not extend it; a **new** tool on either legacy pattern is a blocker regardless. (1) `shiftKey` snap-bypass in the MEP move/endpoint tools (`packages/nodes/src/{duct-segment,pipe-segment,liquid-line,lineset,duct-fitting}/{move-tool,selection}.tsx`). (2) `altKey` alignment-bypass in the roof / polygon / slab pointer-move previews (`components/editor/floorplan-panel.tsx`) and the `resolveSlabPlanPointSnap` / `resolveCeilingPlanPointSnap` paths. **Already migrated — do not regress:** wall + fence drafting (both modifier patterns) and `zone` drafting (`components/tools/zone/zone-tool.tsx` — mode-driven grid/angle gates, no Shift bypass). ## 5. Output format diff --git a/packages/editor/src/components/tools/zone/zone-tool.tsx b/packages/editor/src/components/tools/zone/zone-tool.tsx index 5b74fb4e..74fe3899 100644 --- a/packages/editor/src/components/tools/zone/zone-tool.tsx +++ b/packages/editor/src/components/tools/zone/zone-tool.tsx @@ -13,7 +13,7 @@ import { BufferGeometry, DoubleSide, type Group, type Line, Shape, Vector3 } fro import { EDITOR_LAYER } from './../../../lib/constants' import { sfxEmitter } from './../../../lib/sfx-bus' import { snapWorldXZForActiveBuilding } from './../../../lib/world-grid-snap' -import useEditor from './../../../store/use-editor' +import useEditor, { isAngleSnapActive, isGridSnapActive } from './../../../store/use-editor' import { CursorSphere } from '../shared/cursor-sphere' const Y_OFFSET = 0.02 @@ -65,7 +65,6 @@ export const ZoneTool: React.FC = () => { const pointsRef = useRef>([]) const previousSnappedPointRef = useRef<[number, number] | null>(null) const levelYRef = useRef(0) // Track current level Y position - const shiftPressed = useRef(false) const currentLevelId = useViewer((state) => state.selection.levelId) const setTool = useEditor((state) => state.setTool) @@ -86,21 +85,19 @@ export const ZoneTool: React.FC = () => { mainLineRef.current.geometry = new BufferGeometry() closingLineRef.current.geometry = new BufferGeometry() - // 15° angle snap from the last vertex by default. Shift bypasses all snap. - // Distance snaps along the ray so the vertex lands on - // grid-multiple lengths without leaving the ray. + // Snapping follows the active mode (zone resolves to the 'wall' context): + // `angles` locks the ray to 15° from the last vertex, `grid` quantizes the + // distance along it, `lines` / `off` leave the raw cursor. No held-Shift + // bypass — Shift cycles the mode (see interaction-scope.md). const snapDraftPoint = ( lastPoint: [number, number], - gridPoint: [number, number], + _gridPoint: [number, number], rawPoint: [number, number], ): [number, number] => { - if (shiftPressed.current) return rawPoint - const [x, z] = snapPointAlongAngleRay( - lastPoint, - rawPoint, - DEFAULT_ANGLE_STEP, - useEditor.getState().gridSnapStep, - ) + const angleStep = isAngleSnapActive() ? DEFAULT_ANGLE_STEP : 0 + const gridStep = isGridSnapActive() ? useEditor.getState().gridSnapStep : 0 + if (angleStep === 0 && gridStep === 0) return rawPoint + const [x, z] = snapPointAlongAngleRay(lastPoint, rawPoint, angleStep, gridStep) return [x, z] } @@ -172,15 +169,16 @@ export const ZoneTool: React.FC = () => { if (!cursorRef.current) return // World-grid snap projected into building-local; rotated buildings - // used to pull the snap off the visible grid lines. - const bypassSnap = shiftPressed.current || event.nativeEvent?.shiftKey === true - const [gridX, gridZ] = bypassSnap - ? [event.localPosition[0], event.localPosition[2]] - : snapWorldXZForActiveBuilding( + // used to pull the snap off the visible grid lines. Grid quantize only + // in grid mode; off / lines / angles leave the raw cursor for the first + // vertex (later vertices snap along the ray in `snapDraftPoint`). + const [gridX, gridZ] = isGridSnapActive() + ? snapWorldXZForActiveBuilding( event.position[0], event.position[2], useEditor.getState().gridSnapStep, ).local + : [event.localPosition[0], event.localPosition[2]] cursorPosition = [gridX, gridZ] rawCursorPosition = [event.localPosition[0], event.localPosition[2]] levelYRef.current = event.localPosition[1] @@ -191,9 +189,10 @@ export const ZoneTool: React.FC = () => { ? snapDraftPoint(lastPoint, cursorPosition, rawCursorPosition) : cursorPosition - // Play snap sound when the snapped position changes during drawing + // Play snap sound when the snapped position changes during drawing — only + // when a quantizing mode is active (off / lines move continuously). if ( - !bypassSnap && + (isGridSnapActive() || isAngleSnapActive()) && pointsRef.current.length > 0 && previousSnappedPointRef.current && (displayPoint[0] !== previousSnappedPointRef.current[0] || @@ -211,14 +210,13 @@ export const ZoneTool: React.FC = () => { const onGridClick = (event: GridEvent) => { if (!currentLevelId) return - const bypassSnap = shiftPressed.current || event.nativeEvent?.shiftKey === true - const [gridX, gridZ] = bypassSnap - ? [event.localPosition[0], event.localPosition[2]] - : snapWorldXZForActiveBuilding( + const [gridX, gridZ] = isGridSnapActive() + ? snapWorldXZForActiveBuilding( event.position[0], event.position[2], useEditor.getState().gridSnapStep, ).local + : [event.localPosition[0], event.localPosition[2]] let clickPoint: [number, number] = [gridX, gridZ] // Snap to the 15° ray from the last point @@ -270,28 +268,12 @@ export const ZoneTool: React.FC = () => { } } - const onKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Shift') shiftPressed.current = true - } - const onKeyUp = (e: KeyboardEvent) => { - if (e.key === 'Shift') shiftPressed.current = false - } - const onWindowBlur = () => { - shiftPressed.current = false - } - document.addEventListener('keydown', onKeyDown) - document.addEventListener('keyup', onKeyUp) - window.addEventListener('blur', onWindowBlur) - // Subscribe to events emitter.on('grid:move', onGridMove) emitter.on('grid:click', onGridClick) emitter.on('grid:double-click', onGridDoubleClick) return () => { - document.removeEventListener('keydown', onKeyDown) - document.removeEventListener('keyup', onKeyUp) - window.removeEventListener('blur', onWindowBlur) emitter.off('grid:move', onGridMove) emitter.off('grid:click', onGridClick) emitter.off('grid:double-click', onGridDoubleClick) diff --git a/packages/editor/src/components/ui/helpers/helper-manager.tsx b/packages/editor/src/components/ui/helpers/helper-manager.tsx index ebc18ffd..a9a4f8ce 100644 --- a/packages/editor/src/components/ui/helpers/helper-manager.tsx +++ b/packages/editor/src/components/ui/helpers/helper-manager.tsx @@ -181,16 +181,25 @@ export function HelperManager() { return } - // Registry-first: kinds with `def.toolHints` render through the generic - // `RegisteredToolHelper`. Today that covers ceiling / door / fence / - // item / shelf / slab / spawn / wall / window. + // Legacy fallback — only `roof` remains because it hasn't migrated to + // `def.tool` / `def.toolHints` yet (no Stage D port). Checked before the + // generic tool branch so the snap-context fallback below doesn't capture it + // and drop its bespoke `RoofHelper` hints. When roof migrates, this deletes. + if (tool === 'roof') return + + // Registry-first: a kind renders the generic `RegisteredToolHelper` when it + // declares `def.toolHints`, OR whenever its draft resolves to a snap / + // continuation context — so a snappable tool with NO hand-written hints (e.g. + // `zone`) still advertises the snapping chip it already honors (Shift = cycle). + // `RegisteredToolHelper` self-hides when there's genuinely nothing to show. if (tool) { const def = nodeRegistry.get(tool) - if (def?.toolHints && def.toolHints.length > 0) { + const hints = def?.toolHints ?? [] + if (hints.length > 0 || snapContext || continuationContext) { return ( @@ -198,9 +207,5 @@ export function HelperManager() { } } - // Legacy fallback — only `roof` remains because it hasn't migrated to - // `def.tool` / `def.toolHints` yet (no Stage D port). When roof - // migrates, this switch deletes outright. - if (tool === 'roof') return return null }