Phase 5 Stage D: fix bend history + move-tool loop + grid-snap sfx
Three user-reported regressions, all in the Stage D move/curve ports: 1. **Fence/wall bend cancelled the fence creation on Ctrl-Z.** The action.commit's `return false` shortcut on "no offset change" bypassed the dance entirely — pastStates wasn't touched, so the next Ctrl-Z fell through to whatever preceded activation (typically the create step). Fix: always run the dance, even on no-op commits. First Ctrl-Z then absorbs a silent no-op entry, subsequent presses roll back real prior actions. Same fix applied to fence move-endpoint, fence move, slab move, ceiling move. 2. **Slab/ceiling move 'maximum update depth exceeded' loop.** The `useScene` selector in `SlabMoveTool` returned a freshly-allocated `[sx, sz]` tuple on every call. Zustand's `Object.is` equality failed each comparison → re-subscribe → re-render → loop. Fix: subscribe to the stable live-node reference and derive the center via `useMemo`. Same recipe for fence/ceiling move-tools, including memoizing the `originalCenter` fallback that was getting a new array per render. 3. **No grid-snap sfx during move drag.** Action `preview` now tracks the last snapped pointer on a mutable `lastSnapped` ctx field and emits `sfx:grid-snap` when it changes between ticks. Matches the legacy MoveFenceTool's per-tick sound. Locking test for foot-gun 1 lives in `packages/core/src/services/single-undo-dance.test.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
d43cd569d6
commit
c2c2f66427
@@ -1,4 +1,5 @@
|
||||
import type { AnyNode, AnyNodeId, CeilingNode, DragAction } from '@pascal-app/core'
|
||||
import { triggerSFX } from '@pascal-app/editor'
|
||||
|
||||
/**
|
||||
* Phase 5 Stage D — whole-ceiling move drag affordance.
|
||||
@@ -6,7 +7,8 @@ import type { AnyNode, AnyNodeId, CeilingNode, DragAction } from '@pascal-app/co
|
||||
* Mirrors `slab/actions/move.ts` shape but ceiling snaps purely to a
|
||||
* 0.5m grid (no wall/fence corner snap — ceilings are typically
|
||||
* placed independent of the floor layout). Drag anchor is latched on
|
||||
* the first preview tick so the ceiling doesn't jump.
|
||||
* the first preview tick so the ceiling doesn't jump. Emits a grid-
|
||||
* snap sfx when the snapped position changes between ticks.
|
||||
*
|
||||
* Single-undo dance on commit, same recipe as slab/fence.
|
||||
*/
|
||||
@@ -17,6 +19,10 @@ function snap(value: number): number {
|
||||
return Math.round(value / GRID_STEP) * GRID_STEP
|
||||
}
|
||||
|
||||
function sameSnap(a: [number, number] | null, b: [number, number]): boolean {
|
||||
return a !== null && a[0] === b[0] && a[1] === b[1]
|
||||
}
|
||||
|
||||
function translatePolygon(
|
||||
polygon: Array<[number, number]>,
|
||||
deltaX: number,
|
||||
@@ -30,6 +36,7 @@ export type MoveCeilingCtx = {
|
||||
originalPolygon: Array<[number, number]>
|
||||
originalHoles: Array<Array<[number, number]>>
|
||||
dragAnchor: [number, number] | null
|
||||
lastSnapped: [number, number] | null
|
||||
}
|
||||
|
||||
export type MoveCeilingDraft = {
|
||||
@@ -50,13 +57,19 @@ export const moveCeilingDragAction: DragAction<MoveCeilingCtx, MoveCeilingDraft>
|
||||
h.map(([x, z]) => [x, z] as [number, number]),
|
||||
),
|
||||
dragAnchor: null,
|
||||
lastSnapped: null,
|
||||
}
|
||||
},
|
||||
|
||||
preview: (ctx, point, _modifiers) => {
|
||||
const sx = snap(point[0])
|
||||
const sz = snap(point[1])
|
||||
if (!ctx.dragAnchor) ctx.dragAnchor = [sx, sz]
|
||||
const snapped: [number, number] = [sx, sz]
|
||||
if (!sameSnap(ctx.lastSnapped, snapped)) {
|
||||
if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap')
|
||||
ctx.lastSnapped = snapped
|
||||
}
|
||||
if (!ctx.dragAnchor) ctx.dragAnchor = snapped
|
||||
const deltaX = sx - ctx.dragAnchor[0]
|
||||
const deltaZ = sz - ctx.dragAnchor[1]
|
||||
return {
|
||||
@@ -76,7 +89,9 @@ export const moveCeilingDragAction: DragAction<MoveCeilingCtx, MoveCeilingDraft>
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
if (draft.deltaX === 0 && draft.deltaZ === 0) return false
|
||||
// Always push — see fence/actions/curve.ts. No-movement still
|
||||
// records a pastState entry so Ctrl-Z doesn't fall through to the
|
||||
// ceiling-create step.
|
||||
scene.restoreAll()
|
||||
scene.resumeHistory()
|
||||
scene.update(ctx.ceilingId, {
|
||||
|
||||
@@ -22,7 +22,11 @@ export const CeilingMoveTool: React.FC<{ node: CeilingNode }> = ({ node }) => {
|
||||
const live = useScene((s) => s.nodes[ceilingId])
|
||||
const liveCeiling = live?.type === 'ceiling' ? (live as CeilingNode) : node
|
||||
const polygon = liveCeiling.polygon
|
||||
const holes = liveCeiling.holes ?? []
|
||||
// `?? []` would return a NEW empty array per render, busting downstream
|
||||
// useMemo deps and (under StrictMode) potentially triggering the
|
||||
// "getSnapshot result not cached" warning. Memoize against the source.
|
||||
const EMPTY_HOLES = useMemo(() => [] as Array<Array<[number, number]>>, [])
|
||||
const holes = liveCeiling.holes ?? EMPTY_HOLES
|
||||
|
||||
const center: [number, number] = useMemo(() => {
|
||||
if (polygon.length === 0) return [0, 0]
|
||||
|
||||
@@ -88,15 +88,17 @@ export const curveFenceDragAction: DragAction<CurveFenceCtx, CurveFenceDraft> =
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
// Reject when the offset didn't actually change — createDragSession
|
||||
// will fall through to cancel + scene.restoreAll() (no zundo entry).
|
||||
if (draft.curveOffset === ctx.originalCurveOffset) return false
|
||||
|
||||
// Single-undo dance: revert via the snapshot (paused history → no
|
||||
// zundo record), resume history, then re-apply the final draft so
|
||||
// zundo captures the whole drag as one undo step. Without this the
|
||||
// pause window's mutations never reach pastStates and Ctrl-Z jumps
|
||||
// past the drag back to the state before activation.
|
||||
// Single-undo dance — ALWAYS push a pastState entry, even when the
|
||||
// offset didn't actually change. The "no-op" case (small drag that
|
||||
// `normalizeWallCurveOffset` snaps back to 0) used to return false
|
||||
// here, but that bypassed pastStates entirely; the next Ctrl-Z then
|
||||
// fell through to whatever was on the stack before activation
|
||||
// (typically the fence creation), making it look like the bend
|
||||
// cancelled the create.
|
||||
//
|
||||
// Pushing on every commit means a no-op bend's first Ctrl-Z absorbs
|
||||
// a silent entry (no visible change), then subsequent Ctrl-Z's roll
|
||||
// back the real prior actions. Matches typical editor behavior.
|
||||
scene.restoreAll()
|
||||
scene.resumeHistory()
|
||||
scene.update(ctx.nodeId, { curveOffset: draft.curveOffset } as Partial<AnyNode>)
|
||||
|
||||
@@ -183,11 +183,12 @@ export const moveFenceEndpointDragAction: DragAction<MoveFenceEndpointCtx, MoveF
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
// Reject when the drag didn't move OR the resulting fence would
|
||||
// be shorter than the minimum length. createDragSession.cancel()
|
||||
// restores originals via the snapshot.
|
||||
const hasChanged = !samePoint(draft.movingPoint, ctx.originalMovingPoint)
|
||||
if (!hasChanged) return false
|
||||
// Min-length rejection still matters — too-short fence is invalid
|
||||
// and should bounce back via the cancel path (snapshot restore).
|
||||
// But the "no-change" rejection is removed: see
|
||||
// fence/actions/curve.ts for the rationale (no-op drag must still
|
||||
// push a pastState entry to avoid Ctrl-Z cancelling the fence
|
||||
// creation that preceded the activation).
|
||||
if (!isWallLongEnough(draft.start, draft.end)) return false
|
||||
|
||||
// Single-undo dance: revert to originals (paused history → no
|
||||
|
||||
@@ -9,9 +9,13 @@ import {
|
||||
useScene,
|
||||
type WallNode,
|
||||
} from '@pascal-app/core'
|
||||
import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor'
|
||||
import { type FencePlanPoint, snapFenceDraftPoint, triggerSFX } from '@pascal-app/editor'
|
||||
import type * as THREE from 'three'
|
||||
|
||||
function sameSnap(a: FencePlanPoint | null, b: FencePlanPoint): boolean {
|
||||
return a !== null && a[0] === b[0] && a[1] === b[1]
|
||||
}
|
||||
|
||||
/**
|
||||
* Phase 5 Stage D — whole-fence move drag affordance.
|
||||
*
|
||||
@@ -137,6 +141,10 @@ export type MoveFenceCtx = {
|
||||
// Mutable: latched on the first preview call to the snapped pointer
|
||||
// position. Subsequent previews compute delta = pointer - dragAnchor.
|
||||
dragAnchor: FencePlanPoint | null
|
||||
// Mutable: tracks the last snapped pointer so preview can emit a
|
||||
// grid-snap sfx when the snapped value changes. Matches the legacy
|
||||
// MoveFenceTool's per-tick sound.
|
||||
lastSnapped: FencePlanPoint | null
|
||||
}
|
||||
|
||||
export type MoveFenceDraft = {
|
||||
@@ -180,6 +188,7 @@ export const moveFenceDragAction: DragAction<MoveFenceCtx, MoveFenceDraft> = {
|
||||
levelWalls,
|
||||
levelFences,
|
||||
dragAnchor: null,
|
||||
lastSnapped: null,
|
||||
}
|
||||
},
|
||||
|
||||
@@ -190,6 +199,12 @@ export const moveFenceDragAction: DragAction<MoveFenceCtx, MoveFenceDraft> = {
|
||||
fences: ctx.levelFences,
|
||||
ignoreFenceIds: [ctx.fenceId as string],
|
||||
})
|
||||
// Emit grid-snap sfx when the snapped position changes between
|
||||
// ticks — matches the legacy MoveFenceTool's user feedback.
|
||||
if (!sameSnap(ctx.lastSnapped, snapped)) {
|
||||
if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap')
|
||||
ctx.lastSnapped = snapped
|
||||
}
|
||||
// Latch the anchor on the first preview tick — matches legacy
|
||||
// "drag is delta from first move" semantics so the fence doesn't
|
||||
// jump to wherever the activation click landed.
|
||||
@@ -229,16 +244,15 @@ export const moveFenceDragAction: DragAction<MoveFenceCtx, MoveFenceDraft> = {
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
// Reject when nothing moved — falls through to action.cancel which
|
||||
// clears the live-drag visual state.
|
||||
if (draft.deltaX === 0 && draft.deltaZ === 0) return false
|
||||
|
||||
// Single-undo dance — paused history during drag means nothing
|
||||
// was recorded. We didn't scene.update during apply either (live-
|
||||
// drag exception), so the snapshot is empty and restoreAll is a
|
||||
// no-op. Resume history, then write the final draft. Zundo
|
||||
// captures one diff: original → final.
|
||||
scene.restoreAll() // no-op (no scene updates during apply)
|
||||
// Always push a pastState entry — see fence/actions/curve.ts. The
|
||||
// no-movement case would otherwise let Ctrl-Z cancel the fence
|
||||
// creation that preceded the move.
|
||||
//
|
||||
// Single-undo dance: snapshot is empty (live-drag exception, no
|
||||
// scene.update during apply), so restoreAll is a no-op. Resume,
|
||||
// then write the final draft so zundo records original → final
|
||||
// as one diff.
|
||||
scene.restoreAll()
|
||||
scene.resumeHistory()
|
||||
scene.update(ctx.fenceId, {
|
||||
start: draft.start,
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
import { type FenceNode, useLiveTransforms } from '@pascal-app/core'
|
||||
import { CursorSphere, triggerSFX, useDragAction, useEditor } from '@pascal-app/editor'
|
||||
import { useViewer } from '@pascal-app/viewer'
|
||||
import { useMemo } from 'react'
|
||||
import { moveFenceDragAction } from './actions/move'
|
||||
|
||||
/**
|
||||
@@ -10,8 +11,13 @@ import { moveFenceDragAction } from './actions/move'
|
||||
*
|
||||
* Replaces the legacy `MoveFenceTool` (302 LoC). The action owns all
|
||||
* the math (snap + linked cascade + live-drag mesh offsets +
|
||||
* single-undo dance on commit). The wrapper just renders the cursor
|
||||
* sphere and tracks its position from the live-transform store.
|
||||
* single-undo dance on commit). The wrapper renders the cursor sphere
|
||||
* tracking its position from the live-transform store.
|
||||
*
|
||||
* Selector stability: `originalCenter` is memoized so the live-transform
|
||||
* fallback doesn't return a new array per render — that pattern blows
|
||||
* up zustand's `Object.is` check and trips "getSnapshot result not
|
||||
* cached" → infinite re-render. Same recipe in slab/ceiling move-tool.
|
||||
*
|
||||
* Mounted by ToolManager when `useEditor.movingNode` is a fence
|
||||
* (capability-driven dispatch — fence has no `movable` capability, so
|
||||
@@ -20,19 +26,19 @@ import { moveFenceDragAction } from './actions/move'
|
||||
*/
|
||||
export const FenceMoveTool: React.FC<{ node: FenceNode }> = ({ node }) => {
|
||||
const fenceId = node.id
|
||||
const originalCenter: [number, number, number] = [
|
||||
(node.start[0] + node.end[0]) / 2,
|
||||
0,
|
||||
(node.start[1] + node.end[1]) / 2,
|
||||
]
|
||||
const originalCenter: [number, number, number] = useMemo(
|
||||
() => [(node.start[0] + node.end[0]) / 2, 0, (node.start[1] + node.end[1]) / 2],
|
||||
[node.start, node.end],
|
||||
)
|
||||
|
||||
// Live position from the live-transforms store — the action writes
|
||||
// here every preview tick (live-drag exception). Falls back to the
|
||||
// original center until the first move.
|
||||
const liveCenter = useLiveTransforms((s) => {
|
||||
const t = s.get(fenceId)
|
||||
return t?.position ?? originalCenter
|
||||
})
|
||||
// Subscribe to the live-transform reference only (stable across
|
||||
// renders unless set/clear was called). Derive position via useMemo
|
||||
// so the selector itself stays cached.
|
||||
const liveTransform = useLiveTransforms((s) => s.get(fenceId))
|
||||
const liveCenter: [number, number, number] = useMemo(
|
||||
() => liveTransform?.position ?? originalCenter,
|
||||
[liveTransform, originalCenter],
|
||||
)
|
||||
|
||||
const exitMoveMode = (committed: boolean) => {
|
||||
if (committed) triggerSFX('sfx:item-place')
|
||||
@@ -53,7 +59,7 @@ export const FenceMoveTool: React.FC<{ node: FenceNode }> = ({ node }) => {
|
||||
|
||||
return (
|
||||
<group>
|
||||
<CursorSphere position={liveCenter as [number, number, number]} showTooltip={false} />
|
||||
<CursorSphere position={liveCenter} showTooltip={false} />
|
||||
</group>
|
||||
)
|
||||
}
|
||||
|
||||
@@ -8,7 +8,11 @@ import {
|
||||
useScene,
|
||||
type WallNode,
|
||||
} from '@pascal-app/core'
|
||||
import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor'
|
||||
import { type FencePlanPoint, snapFenceDraftPoint, triggerSFX } from '@pascal-app/editor'
|
||||
|
||||
function sameSnap(a: FencePlanPoint | null, b: FencePlanPoint): boolean {
|
||||
return a !== null && a[0] === b[0] && a[1] === b[1]
|
||||
}
|
||||
|
||||
/**
|
||||
* Phase 5 Stage D — whole-slab move drag affordance.
|
||||
@@ -16,7 +20,8 @@ import { type FencePlanPoint, snapFenceDraftPoint } from '@pascal-app/editor'
|
||||
* Translates the slab's boundary polygon (and any holes) rigidly under
|
||||
* the pointer. Snaps to walls / fences / grid at the level. Latches
|
||||
* the drag anchor on the first preview tick so the slab doesn't jump
|
||||
* to wherever the activation click landed.
|
||||
* to wherever the activation click landed. Emits grid-snap sfx when
|
||||
* the snapped position changes between ticks (matches legacy UX).
|
||||
*
|
||||
* Unlike fence move, the slab port does **not** use the live-drag
|
||||
* exception — polygon CSG geometry is expensive to rebuild per frame,
|
||||
@@ -52,6 +57,7 @@ export type MoveSlabCtx = {
|
||||
levelWalls: WallNode[]
|
||||
levelFences: FenceNode[]
|
||||
dragAnchor: FencePlanPoint | null
|
||||
lastSnapped: FencePlanPoint | null
|
||||
}
|
||||
|
||||
export type MoveSlabDraft = {
|
||||
@@ -98,6 +104,7 @@ export const moveSlabDragAction: DragAction<MoveSlabCtx, MoveSlabDraft> = {
|
||||
levelWalls,
|
||||
levelFences,
|
||||
dragAnchor: null,
|
||||
lastSnapped: null,
|
||||
}
|
||||
},
|
||||
|
||||
@@ -107,6 +114,11 @@ export const moveSlabDragAction: DragAction<MoveSlabCtx, MoveSlabDraft> = {
|
||||
walls: ctx.levelWalls,
|
||||
fences: ctx.levelFences,
|
||||
})
|
||||
// Emit grid-snap sfx when the snapped position changes.
|
||||
if (!sameSnap(ctx.lastSnapped, snapped)) {
|
||||
if (ctx.lastSnapped !== null) triggerSFX('sfx:grid-snap')
|
||||
ctx.lastSnapped = snapped
|
||||
}
|
||||
if (!ctx.dragAnchor) ctx.dragAnchor = snapped
|
||||
const deltaX = snapped[0] - ctx.dragAnchor[0]
|
||||
const deltaZ = snapped[1] - ctx.dragAnchor[1]
|
||||
@@ -130,11 +142,9 @@ export const moveSlabDragAction: DragAction<MoveSlabCtx, MoveSlabDraft> = {
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
if (draft.deltaX === 0 && draft.deltaZ === 0) return false
|
||||
|
||||
// Single-undo dance — revert via snapshot, resume history, re-apply
|
||||
// the final polygon/holes. Zundo captures the whole drag as one
|
||||
// Ctrl-Z step.
|
||||
// Always push — see fence/actions/curve.ts. Even on a no-movement
|
||||
// commit, the dance must push a pastState entry so Ctrl-Z doesn't
|
||||
// cancel whatever was on the stack before activation.
|
||||
scene.restoreAll()
|
||||
scene.resumeHistory()
|
||||
scene.update(ctx.slabId, {
|
||||
|
||||
@@ -3,32 +3,42 @@
|
||||
import { type SlabNode, useScene } from '@pascal-app/core'
|
||||
import { CursorSphere, triggerSFX, useDragAction, useEditor } from '@pascal-app/editor'
|
||||
import { useViewer } from '@pascal-app/viewer'
|
||||
import { useMemo } from 'react'
|
||||
import { moveSlabDragAction } from './actions/move'
|
||||
|
||||
/**
|
||||
* Phase 5 Stage D — thin React wrapper around `moveSlabDragAction`.
|
||||
*
|
||||
* Replaces the legacy `MoveSlabTool` (182 LoC). All math + history
|
||||
* dance lives in the action; this wrapper just renders the cursor
|
||||
* sphere following the live polygon center.
|
||||
* dance lives in the action; this wrapper renders the cursor sphere
|
||||
* at the live polygon center.
|
||||
*
|
||||
* NOTE on selector stability: the live polygon center MUST be derived
|
||||
* via `useMemo` over the node reference rather than computed inside
|
||||
* the `useScene` selector — returning a fresh `[x, z]` tuple from the
|
||||
* selector on every call triggers "getSnapshot result not cached"
|
||||
* → infinite re-render. Same pattern in fence/ceiling move-tool.
|
||||
*/
|
||||
export const SlabMoveTool: React.FC<{ node: SlabNode }> = ({ node }) => {
|
||||
const slabId = node.id
|
||||
const initialCenter: [number, number] = useMemo(() => {
|
||||
if (node.polygon.length === 0) return [0, 0]
|
||||
let sx = 0
|
||||
let sz = 0
|
||||
for (const [x, z] of node.polygon) {
|
||||
sx += x
|
||||
sz += z
|
||||
}
|
||||
return [sx / node.polygon.length, sz / node.polygon.length]
|
||||
}, [node.polygon])
|
||||
|
||||
const initialCenter: [number, number] =
|
||||
node.polygon.length > 0
|
||||
? [
|
||||
node.polygon.reduce((s, [x]) => s + x, 0) / node.polygon.length,
|
||||
node.polygon.reduce((s, [, z]) => s + z, 0) / node.polygon.length,
|
||||
]
|
||||
: [0, 0]
|
||||
|
||||
// Live polygon center — re-derived from the scene store every tick
|
||||
// since the action writes the translated polygon onto the slab.
|
||||
const liveCenter = useScene((s) => {
|
||||
const live = s.nodes[slabId]
|
||||
if (live?.type !== 'slab') return initialCenter
|
||||
const poly = (live as SlabNode).polygon
|
||||
// Subscribe to the live node reference (stable across renders when
|
||||
// the node hasn't changed; new reference per scene update). Derive
|
||||
// the center inside `useMemo` so the selector itself stays cached.
|
||||
const liveNode = useScene((s) => s.nodes[slabId])
|
||||
const liveCenter = useMemo<[number, number]>(() => {
|
||||
if (liveNode?.type !== 'slab') return initialCenter
|
||||
const poly = (liveNode as SlabNode).polygon
|
||||
if (poly.length === 0) return initialCenter
|
||||
let sx = 0
|
||||
let sz = 0
|
||||
@@ -36,8 +46,8 @@ export const SlabMoveTool: React.FC<{ node: SlabNode }> = ({ node }) => {
|
||||
sx += x
|
||||
sz += z
|
||||
}
|
||||
return [sx / poly.length, sz / poly.length] as [number, number]
|
||||
})
|
||||
return [sx / poly.length, sz / poly.length]
|
||||
}, [liveNode, initialCenter])
|
||||
|
||||
const exitMoveMode = (committed: boolean) => {
|
||||
if (committed) triggerSFX('sfx:item-place')
|
||||
|
||||
@@ -68,7 +68,9 @@ export const curveWallDragAction: DragAction<CurveWallCtx, CurveWallDraft> = {
|
||||
},
|
||||
|
||||
commit: (draft, ctx, scene) => {
|
||||
if (draft.curveOffset === ctx.originalCurveOffset) return false
|
||||
// Always push a pastState entry — see fence/actions/curve.ts for
|
||||
// the rationale (no-op-bend would otherwise let Ctrl-Z cancel the
|
||||
// wall creation).
|
||||
scene.restoreAll()
|
||||
scene.resumeHistory()
|
||||
scene.update(ctx.nodeId, { curveOffset: draft.curveOffset } as Partial<AnyNode>)
|
||||
|
||||
Reference in New Issue
Block a user