Move: pure imperative via sceneRegistry (smooth, zero re-renders); drop dev logs
User feedback: updating the store per tick caused tons of React
re-renders → laggy drag. Switched to pure imperative.
MoveRegistryNodeTool now:
- Mutates `sceneRegistry.nodes.get(id).position` directly per
grid:move tick. No useScene.updateNode during drag. No store
change → no renderer re-render → R3F doesn't reapply
`position={node.position}` → the imperative mutation sticks.
- On commit: single tracked `useScene.updateNode(id, { position })`.
Undo replays one step (original → final), no per-tick spam.
- On cancel / unmount: imperatively snap the mesh back to original.
Store was never touched so no data revert needed.
Trade-off vs the items pattern (which does update the store per tick
and re-renders per tick): our approach is faster but assumes the
renderer doesn't re-render mid-drag. Items get away with constant
re-renders because their renderer is heavily optimized; for parametric
shelves (and future kinds) the imperative path is simpler and faster.
Cleanup: removed the dev `[shelf] rendered` and `[shelf] placed`
console.info logs from the shelf renderer and tool. They were Phase 2
verification scaffolding — no longer needed now that everything works.
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
7bd34e5ff0
commit
166e860bfe
@@ -22,23 +22,27 @@ const roundToHalf = (value: number) => Math.round(value * 2) / 2
|
|||||||
/**
|
/**
|
||||||
* Generic move tool for any registry-backed kind.
|
* Generic move tool for any registry-backed kind.
|
||||||
*
|
*
|
||||||
* Pattern: matches the item move tool (which works). The
|
* Imperative-only motion during drag:
|
||||||
* `useLiveTransforms` + `sceneRegistry.position.set` approach used by
|
* - On every `grid:move` we mutate `sceneRegistry.nodes.get(id).position`
|
||||||
* MoveColumnTool is broken — the renderer doesn't visibly follow.
|
* directly. The node's store data is unchanged → the renderer doesn't
|
||||||
|
* re-render → R3F doesn't reapply `position={node.position}` → the
|
||||||
|
* imperative mutation sticks. Movement is smooth, framerate-locked,
|
||||||
|
* and React-free.
|
||||||
*
|
*
|
||||||
* Instead, on every `grid:move` we directly `useScene.updateNode(id, { position })`
|
* Store update happens only on commit (single undoable action).
|
||||||
* while history is paused. The kind's renderer already reads
|
|
||||||
* `node.position` from the store, so the mesh visibly follows the cursor.
|
|
||||||
*
|
*
|
||||||
* Commit: pause-revert-resume-update sequence so undo replays one
|
* Cancel imperatively snaps the mesh back to its original position and
|
||||||
* coherent action (revert → final position) instead of the per-tick
|
* resumes history without ever having touched the store mid-drag.
|
||||||
* spam that the move generated.
|
|
||||||
*
|
*
|
||||||
* Cancel: restore the original position before unmounting.
|
* This is faster than the items pattern (which updates the store per tick
|
||||||
|
* and re-renders the renderer on every mouse move). Trade-off: if the
|
||||||
|
* renderer happens to re-render for some other reason mid-drag, R3F will
|
||||||
|
* reapply node.position and snap the mesh back. Mitigation: history is
|
||||||
|
* paused, no upstream state subscribed by the renderer changes during the
|
||||||
|
* drag, so re-renders are rare in practice. If a kind needs guaranteed
|
||||||
|
* stability, it can opt into a "live position" hook in Phase 4.
|
||||||
*/
|
*/
|
||||||
export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
||||||
// Snapshot the original position once at mount — used for cancel revert
|
|
||||||
// and for the pause-revert-resume sequence at commit.
|
|
||||||
const originalPosition: [number, number, number] = useMemo(
|
const originalPosition: [number, number, number] = useMemo(
|
||||||
() =>
|
() =>
|
||||||
'position' in node && Array.isArray((node as { position?: unknown }).position)
|
'position' in node && Array.isArray((node as { position?: unknown }).position)
|
||||||
@@ -54,25 +58,25 @@ export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
|||||||
}, [])
|
}, [])
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
// Pause history so per-tick updateNode calls don't fill the undo stack.
|
// Pause history so the eventual commit lands as a single undoable step,
|
||||||
|
// not the per-tick spam that would happen if we updated the store on
|
||||||
|
// each move.
|
||||||
useScene.temporal.getState().pause()
|
useScene.temporal.getState().pause()
|
||||||
previousSnapRef.current = null
|
previousSnapRef.current = null
|
||||||
let committed = false
|
let committed = false
|
||||||
|
|
||||||
const applyPreview = (position: [number, number, number]) => {
|
|
||||||
setCursorPosition(position)
|
|
||||||
// Update the actual scene node — the renderer reads node.position and
|
|
||||||
// re-renders. The mesh visibly follows. Same pattern as the item move
|
|
||||||
// tool (useDraftNode.adopt).
|
|
||||||
useScene.getState().updateNode(node.id, { position } as Partial<AnyNode>)
|
|
||||||
}
|
|
||||||
|
|
||||||
const onGridMove = (event: GridEvent) => {
|
const onGridMove = (event: GridEvent) => {
|
||||||
const x = roundToHalf(event.localPosition[0])
|
const x = roundToHalf(event.localPosition[0])
|
||||||
const z = roundToHalf(event.localPosition[2])
|
const z = roundToHalf(event.localPosition[2])
|
||||||
applyPreview([x, 0, z])
|
setCursorPosition([x, 0, z])
|
||||||
|
|
||||||
// Click sound on grid-cell cross, matching the placement tools.
|
// Pure imperative: move the mesh via its registered Object3D ref.
|
||||||
|
// No React re-render. No store update. The shelf (or any registry
|
||||||
|
// kind) follows the cursor smoothly because nothing competes with
|
||||||
|
// this position write until commit.
|
||||||
|
sceneRegistry.nodes.get(node.id)?.position.set(x, 0, z)
|
||||||
|
|
||||||
|
// SFX on cell-cross, matching placement.
|
||||||
const prev = previousSnapRef.current
|
const prev = previousSnapRef.current
|
||||||
if (!prev || prev[0] !== x || prev[1] !== z) {
|
if (!prev || prev[0] !== x || prev[1] !== z) {
|
||||||
sfxEmitter.emit('sfx:grid-snap')
|
sfxEmitter.emit('sfx:grid-snap')
|
||||||
@@ -88,12 +92,9 @@ export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
|||||||
]
|
]
|
||||||
|
|
||||||
if (useScene.getState().nodes[node.id]) {
|
if (useScene.getState().nodes[node.id]) {
|
||||||
// Restore original position while still paused, then resume and
|
// Store still has the original position (we didn't touch it during
|
||||||
// do a single tracked update. Undo replays the (original → final)
|
// drag). Resume history and do one tracked update. Undo replays the
|
||||||
// single step, not every grid-move tick.
|
// (original → final) single step.
|
||||||
useScene.getState().updateNode(node.id, {
|
|
||||||
position: originalPosition,
|
|
||||||
} as Partial<AnyNode>)
|
|
||||||
useScene.temporal.getState().resume()
|
useScene.temporal.getState().resume()
|
||||||
useScene.getState().updateNode(node.id, { position } as Partial<AnyNode>)
|
useScene.getState().updateNode(node.id, { position } as Partial<AnyNode>)
|
||||||
useScene.temporal.getState().pause()
|
useScene.temporal.getState().pause()
|
||||||
@@ -121,11 +122,7 @@ export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const onCancel = () => {
|
const onCancel = () => {
|
||||||
// Restore original position while paused — this won't enter undo.
|
// Snap mesh back to original visually. Store was never touched.
|
||||||
useScene.getState().updateNode(node.id, {
|
|
||||||
position: originalPosition,
|
|
||||||
} as Partial<AnyNode>)
|
|
||||||
// Defensive Three.js reset in case React render lags.
|
|
||||||
sceneRegistry.nodes
|
sceneRegistry.nodes
|
||||||
.get(node.id)
|
.get(node.id)
|
||||||
?.position.set(originalPosition[0], originalPosition[1], originalPosition[2])
|
?.position.set(originalPosition[0], originalPosition[1], originalPosition[2])
|
||||||
@@ -142,13 +139,13 @@ export function MoveRegistryNodeTool({ node }: { node: AnyNode }) {
|
|||||||
emitter.off('grid:move', onGridMove)
|
emitter.off('grid:move', onGridMove)
|
||||||
emitter.off('grid:click', onGridClick)
|
emitter.off('grid:click', onGridClick)
|
||||||
emitter.off('tool:cancel', onCancel)
|
emitter.off('tool:cancel', onCancel)
|
||||||
// If we unmount without committing (e.g., user picks a different
|
// If we unmount without committing (e.g., the user switches tools or
|
||||||
// tool), restore original position so the scene doesn't show the
|
// navigates away), restore the mesh imperatively and resume history.
|
||||||
// stale preview state, and resume history.
|
// Store was never touched so no data revert is needed.
|
||||||
if (!committed) {
|
if (!committed) {
|
||||||
useScene.getState().updateNode(node.id, {
|
sceneRegistry.nodes
|
||||||
position: originalPosition,
|
.get(node.id)
|
||||||
} as Partial<AnyNode>)
|
?.position.set(originalPosition[0], originalPosition[1], originalPosition[2])
|
||||||
useScene.temporal.getState().resume()
|
useScene.temporal.getState().resume()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,7 +2,7 @@
|
|||||||
|
|
||||||
import { useLiveTransforms, useRegistry } from '@pascal-app/core'
|
import { useLiveTransforms, useRegistry } from '@pascal-app/core'
|
||||||
import { useNodeEvents } from '@pascal-app/viewer'
|
import { useNodeEvents } from '@pascal-app/viewer'
|
||||||
import { useEffect, useMemo, useRef } from 'react'
|
import { useMemo, useRef } from 'react'
|
||||||
import { Color, type Group } from 'three'
|
import { Color, type Group } from 'three'
|
||||||
import type { ShelfNode } from './schema'
|
import type { ShelfNode } from './schema'
|
||||||
|
|
||||||
@@ -40,11 +40,6 @@ const ShelfRenderer = ({ node }: { node: ShelfNode }) => {
|
|||||||
: Math.max(0.02, node.depth * 0.12)
|
: Math.max(0.02, node.depth * 0.12)
|
||||||
const bracketDepth = node.bracketStyle === 'industrial' ? node.depth * 0.95 : node.depth * 0.7
|
const bracketDepth = node.bracketStyle === 'industrial' ? node.depth * 0.95 : node.depth * 0.7
|
||||||
|
|
||||||
useEffect(() => {
|
|
||||||
// biome-ignore lint/suspicious/noConsole: dev-only verification log
|
|
||||||
console.info('[shelf] rendered', node.id, 'at', node.position)
|
|
||||||
}, [node.id, node.position])
|
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<group
|
<group
|
||||||
position={liveTransform?.position ?? node.position}
|
position={liveTransform?.position ?? node.position}
|
||||||
|
|||||||
@@ -75,8 +75,6 @@ const ShelfTool = () => {
|
|||||||
useScene.getState().createNode(shelf, activeLevelId)
|
useScene.getState().createNode(shelf, activeLevelId)
|
||||||
useViewer.getState().setSelection({ selectedIds: [shelf.id] })
|
useViewer.getState().setSelection({ selectedIds: [shelf.id] })
|
||||||
triggerSFX('sfx:structure-build')
|
triggerSFX('sfx:structure-build')
|
||||||
// biome-ignore lint/suspicious/noConsole: dev-only verification log
|
|
||||||
console.info('[shelf] placed', shelf.id, 'level-local', position, 'parent', activeLevelId)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
emitter.on('grid:move', onGridMove)
|
emitter.on('grid:move', onGridMove)
|
||||||
|
|||||||
Reference in New Issue
Block a user