From 1155b12b32cc80950ffdea8fc4227167b89c8a1c Mon Sep 17 00:00:00 2001 From: Tanish Manku Date: Fri, 1 May 2026 04:51:11 -0700 Subject: [PATCH] PageScene: coalesce burst undo entries via UndoableAction.tryMerge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous panel-undo fix used session boundaries (open on first edit, commit on selection change / unmount). It overshot: while editing the same object's panel without changing selection, the session never committed, so Ctrl+Z fell through to the most recent *previous* entry — typically the CreateAction that added the shape. Result: editing a shape's transform then hitting Ctrl+Z deleted the shape. Replace session boundaries with action-level coalescing: - Add optional `tryMerge(other)` to the UndoableAction interface. HistoryManager.record now offers each new action to the previous entry's tryMerge first; if absorbed, the new action drops out and the existing entry's `after` extends to cover the burst. - TransformAction: constructor now takes (editor, uuid, before, after) directly; drop the begin/commit pattern. tryMerge merges a follow-up TransformAction whose `before` matches this one's `after` (chain continuity) — same uuid only. - ColorAction: same treatment. tryMerge folds the native color picker's per-pixel onChange burst into a single undo entry. Caller-side: drop all session machinery from the panel (transformSessionRef, colorSessionRef, lastUuidRef, beginPanelTransform, commitPanelTransform, the unmount-only effect). On every input change the panel calls `transformObject` / `setObjectColor`, which snapshot before, apply, snapshot after, and record. The manager handles the rest. Gizmo path in editor.ts updated to the new TransformAction shape: capture before-snap at dragstart on a small `activeDragStart` ref, build TransformAction(before, after) at dragend. Side benefit: gizmo-drag and panel-edit bursts on the same object now coalesce into a single undo entry across input sources, since both produce TransformActions whose chain continues. --- .../pages/PageScene/actions/setObjectColor.ts | 52 ++------- .../PageScene/actions/transformObject.ts | 62 +++++----- .../ControlPanelSceneObject.tsx | 110 +++++------------- .../app/src/pages/PageScene/engine/editor.ts | 47 ++++++-- .../PageScene/engine/editor/HistoryManager.ts | 16 +++ .../engine/editor/actions/ColorAction.ts | 20 +++- .../engine/editor/actions/TransformAction.ts | 59 ++++------ 7 files changed, 160 insertions(+), 206 deletions(-) diff --git a/frontend/apps/artcraft/app/src/pages/PageScene/actions/setObjectColor.ts b/frontend/apps/artcraft/app/src/pages/PageScene/actions/setObjectColor.ts index 76263612b0..330df13029 100644 --- a/frontend/apps/artcraft/app/src/pages/PageScene/actions/setObjectColor.ts +++ b/frontend/apps/artcraft/app/src/pages/PageScene/actions/setObjectColor.ts @@ -1,14 +1,17 @@ import type Editor from "../engine/editor"; import { ColorAction } from "../engine/editor/actions/ColorAction"; -// One-shot apply: use when the change is atomic (e.g. a button click -// or programmatic setter). Records a single ColorAction. +// Apply a color change and record it for undo. // -// For interactive flows where a single user gesture fires many -// intermediate values (the native dialog spams -// onChange per slider pixel), use beginColorSession instead — it -// applies each intermediate value to the engine for visual feedback -// but records only ONE ColorAction on commit. +// Burst inputs (the native dialog spams onChange +// per slider pixel) call this on every onChange. HistoryManager.record +// coalesces consecutive ColorActions for the same uuid into one undo +// entry via ColorAction.tryMerge — so the caller doesn't need +// session-state. +// +// Reads the before-state from the live engine object so the helper is +// self-contained and stays correct even after undo/redo (where the +// engine state diverges from any local React state the view may hold). export function setObjectColor( editor: Editor, uuid: string, @@ -21,38 +24,3 @@ export function setObjectColor( editor.activeScene.setColor(uuid, color); editor.history.record(new ColorAction(editor, uuid, before, color)); } - -// Session-style color edit: snapshots the before-state at construction -// (focus / picker open), applies each intermediate value to the engine -// (visual feedback during slider drag), and records exactly one -// ColorAction on commit (focus loss / picker close). No-op on commit -// if the final color matches the before-state. -export type ColorSession = { - apply(color: string): void; - commit(): void; -}; - -export function beginColorSession( - editor: Editor, - uuid: string, -): ColorSession { - const startObj = editor.activeScene.scene.getObjectByProperty( - "uuid", - uuid, - ); - const before = (startObj?.userData.color as string) ?? "#ffffff"; - return { - apply(color: string) { - editor.activeScene.setColor(uuid, color); - }, - commit() { - const endObj = editor.activeScene.scene.getObjectByProperty( - "uuid", - uuid, - ); - const after = (endObj?.userData.color as string) ?? before; - if (before === after) return; - editor.history.record(new ColorAction(editor, uuid, before, after)); - }, - }; -} diff --git a/frontend/apps/artcraft/app/src/pages/PageScene/actions/transformObject.ts b/frontend/apps/artcraft/app/src/pages/PageScene/actions/transformObject.ts index 4cdf449645..ca0316723e 100644 --- a/frontend/apps/artcraft/app/src/pages/PageScene/actions/transformObject.ts +++ b/frontend/apps/artcraft/app/src/pages/PageScene/actions/transformObject.ts @@ -1,39 +1,39 @@ import type Editor from "../engine/editor"; import { TransformAction } from "../engine/editor/actions/TransformAction"; +import { + snapshotTransform, + transformsEqual, +} from "../engine/editor/actions/snapshots"; +import type { XYZ } from "../datastructures/common"; -// A transform "session" — opened on the first edit of a multi-input -// editing flow (e.g. Object Panel position/rotation/scale text fields), -// committed at the end of the session (selection change, panel -// unmount, focus moving away from the panel). +// Apply a transform change and record it for undo. // -// The session captures the before-state at construction. On commit it -// captures the after-state, pushes a single TransformAction onto the -// undo stack only if the transform actually changed, and re-syncs the -// Zustand `objectPanel` slice from the engine so the panel's view of -// the selection matches the engine state. +// Burst inputs (panel keystrokes, scrub drags) call this on every +// engine update. HistoryManager.record coalesces consecutive +// TransformActions for the same uuid into one undo entry, so the +// caller doesn't need session-state — `tryMerge` on TransformAction +// folds successive entries into the most recent one. // -// Engine-level concerns (capture / apply / revert) stay in -// `engine/editor/actions/TransformAction.ts`; this file is the -// orchestration wrapper that the action layer (UI code) uses so the -// view never has to construct an UndoableAction directly. -export type TransformSession = { - commit(): void; -}; - -export function beginTransformSession( +// Snapshots before and after so the recorded action holds the actual +// engine state delta (not the panel's local React state). Also +// re-syncs the Zustand objectPanel slice so the panel's +// currentSceneObject reflects the post-update transform. +export function transformObject( editor: Editor, uuid: string, -): TransformSession { - const action = new TransformAction(editor, uuid); - return { - commit() { - if (action.commit()) { - editor.history.record(action); - } - // Push the engine's post-edit transform back into the Zustand - // objectPanel slice so the panel's currentSceneObject vectors - // are in sync with what the user just committed. Idempotent. - editor.selection.updateSelectedUI(); - }, - }; + position: XYZ, + rotation: XYZ, + scale: XYZ, +): void { + const obj = editor.activeScene.scene.getObjectByProperty("uuid", uuid); + if (!obj) return; + const before = snapshotTransform(obj); + + editor.sceneManager?.updateSelectedTransform(position, rotation, scale); + + const after = snapshotTransform(obj); + if (!transformsEqual(before, after)) { + editor.history.record(new TransformAction(editor, uuid, before, after)); + } + editor.selection.updateSelectedUI(); } diff --git a/frontend/apps/artcraft/app/src/pages/PageScene/comps/ControlPanelSceneObject/ControlPanelSceneObject.tsx b/frontend/apps/artcraft/app/src/pages/PageScene/comps/ControlPanelSceneObject/ControlPanelSceneObject.tsx index cab0d6c34b..2cbd148a48 100644 --- a/frontend/apps/artcraft/app/src/pages/PageScene/comps/ControlPanelSceneObject/ControlPanelSceneObject.tsx +++ b/frontend/apps/artcraft/app/src/pages/PageScene/comps/ControlPanelSceneObject/ControlPanelSceneObject.tsx @@ -3,7 +3,6 @@ import { useContext, useEffect, useId, - useRef, useState, } from "react"; import { Transition } from "@headlessui/react"; @@ -25,14 +24,8 @@ import { objectMismatch } from "~/pages/PageScene/comps/ControlPanelSceneObject/ import { XYZ } from "~/pages/PageScene/datastructures/common"; import { pageHeight } from "~/signals"; import { DraggablePrecisionMutator } from "./DraggablePrecisionMutator"; -import { - beginColorSession, - ColorSession, -} from "~/pages/PageScene/actions/setObjectColor"; -import { - beginTransformSession, - TransformSession, -} from "~/pages/PageScene/actions/transformObject"; +import { setObjectColor } from "~/pages/PageScene/actions/setObjectColor"; +import { transformObject } from "~/pages/PageScene/actions/transformObject"; // TODO this will be useful later to fix the bug on leading zeros // const formatNumber = (input: string): number => { @@ -72,34 +65,11 @@ export const ControlPanelSceneObject = () => { const [color, setColor] = useState("#ffffff"); - // Pending transform session — opened on first panel edit, committed - // when the *selection's uuid* actually changes (NOT on every per-frame - // updateSelectedUI re-push that just creates a new currentSceneObject - // reference for the same uuid). lastUuidRef tracks the previous uuid - // so the commit effect can distinguish a real selection change from - // a same-uuid resync. - const transformSessionRef = useRef(null); - const lastUuidRef = useRef(null); - - const beginPanelTransform = () => { - if (transformSessionRef.current) return; - if (!editorEngine || !currentSceneObject) return; - transformSessionRef.current = beginTransformSession( - editorEngine, - currentSceneObject.object_uuid, - ); - }; - - const commitPanelTransform = () => { - transformSessionRef.current?.commit(); - transformSessionRef.current = null; - }; - - // Color picker session — opened on input focus (picker dialog opens), - // committed on input blur (picker dialog closes). Records exactly one - // ColorAction per pick regardless of how many onChange events the - // native picker fires while the user drags the slider. - const colorSessionRef = useRef(null); + // No session machinery here — burst inputs (keystrokes, scrub drags, + // native color-picker per-pixel onChange) record per engine update. + // HistoryManager coalesces consecutive same-target same-kind actions + // into one undo entry via tryMerge, so each editing run collapses to + // a single Ctrl+Z step automatically. const colorInputId = useId(); @@ -123,37 +93,32 @@ export const ControlPanelSceneObject = () => { } useEffect(() => { - if (!inputsUpdated || !editorEngine) { + if (!inputsUpdated || !editorEngine || !currentSceneObject) { return; } setInputsUpdated(false); - editorEngine.sceneManager?.updateSelectedTransform( + transformObject( + editorEngine, + currentSceneObject.object_uuid, localToEngine(localPosition), localToEngine(localRotation), localToEngine(localScale), ); - }, [inputsUpdated, localPosition, localRotation, localScale, editorEngine]); + }, [ + inputsUpdated, + localPosition, + localRotation, + localScale, + editorEngine, + currentSceneObject, + ]); useEffect(() => { if (!editorEngine || !currentSceneObject) { return; } - // The per-frame `updateSelectedUI` in the engine pushes a fresh - // `currentSceneObject` object into the store every frame the - // selected obj's transform changes. That re-fires this effect with - // the *same* uuid but new vectors. Only an actual selection change - // (different uuid) is a real session boundary — commit the pending - // session for the previous uuid before we resync local state. - if ( - lastUuidRef.current !== null && - lastUuidRef.current !== currentSceneObject.object_uuid - ) { - commitPanelTransform(); - } - lastUuidRef.current = currentSceneObject.object_uuid; - const vectors = currentSceneObject.objectVectors; // local state relies on strings @@ -163,15 +128,8 @@ export const ControlPanelSceneObject = () => { setLocked(editorEngine.selection.isObjectLocked(editorEngine?.selected?.uuid || "")); setColor(editorEngine?.selected?.userData.color); - // No cleanup function — uuid-change commit lives in the body - // above. Unmount commit is handled by the separate effect below. }, [currentSceneObject, editorEngine]); - // Final commit on panel unmount. - useEffect(() => { - return () => commitPanelTransform(); - }, []); - if (!currentSceneObject) { return null; } @@ -191,7 +149,6 @@ export const ControlPanelSceneObject = () => { } const cleanXyz = sanitize(xyz); if (objectMismatch(localPosition, cleanXyz)) { - beginPanelTransform(); setInputsUpdated(true); } setLocalPosition(xyz); @@ -204,7 +161,6 @@ export const ControlPanelSceneObject = () => { } const cleanXyz = sanitize(xyz); if (objectMismatch(localRotation, cleanXyz)) { - beginPanelTransform(); setInputsUpdated(true); } setLocalRotation(xyz); @@ -226,7 +182,6 @@ export const ControlPanelSceneObject = () => { } const cleanXyz = sanitize(xyz); if (objectMismatch(localScale, cleanXyz)) { - beginPanelTransform(); setInputsUpdated(true); } setLocalScale(xyz); @@ -290,31 +245,18 @@ export const ControlPanelSceneObject = () => { type="color" value={color} disabled={locked} - onFocus={() => { - // Picker is opening — open a session that captures the - // before-state once. apply()/commit() handle visual - // feedback during the drag and the final undo entry on - // close. + onChange={(e: ChangeEvent) => { + // The native picker fires onChange per slider pixel. + // setObjectColor records each one as a small ColorAction; + // HistoryManager.record uses ColorAction.tryMerge to + // collapse the burst into a single undo entry. + const after = e.target.value; const uuid = editorEngine?.selected?.uuid; if (uuid && editorEngine) { - colorSessionRef.current = beginColorSession( - editorEngine, - uuid, - ); + setObjectColor(editorEngine, uuid, after); } - }} - onChange={(e: ChangeEvent) => { - // Native color picker fires per-pixel during slider - // drag. apply() updates the engine for visual feedback - // but does NOT record. Recording happens once on blur. - const after = e.target.value; - colorSessionRef.current?.apply(after); setColor(after); }} - onBlur={() => { - colorSessionRef.current?.commit(); - colorSessionRef.current = null; - }} />