From 6491888f568edc15c2ef9839e9b29bd624d239b9 Mon Sep 17 00:00:00 2001 From: MelekTaus Date: Thu, 1 Oct 2026 07:45:05 +0200 Subject: [PATCH] Stop offering private variables as new animation rows (#270) A behavior variable switched private is hidden from the placed object and from the Properties node, but the row dialog of an animation kept offering it. Rows that exist already stay as they are, as decided in the issue. - offeredDefs in src/core/model/animation.ts is what the dialog offers now: what the target can animate, less private variables and less what another row animates already. The filter moved there from RowDialog.ts, so it can be tested without the editor. - animatableDefsOf is unchanged. The label and the runtime read it, so a row on a variable that went private keeps playing and keeps its label. - Deviation from the proposal: under Change target the row's own variable stays on offer, private or not. The dialog would open with no property and a disabled button otherwise. Other private variables are not offered there either. - The command does not refuse a private variable. Only the dialog does not offer it. Finding, not fixed: e2e 56 fails when it runs alone, in 56a-ii from #257 (typing "9" did not arrive: focus is BODY). With 55 before it the module passes, so a full run does not show it. It was not measured on HEAD without this change. Tests: - Gate: 198 files, 2883 tests (2882 + 1), exit 0 - e2e 55,56: 27 checks passed, 56 has +56e - e2e 25: 31 checks passed, it uses the same dialog - Three unit counter-probes each fail the new test - The browser counter-probe fails 56e, mood shows up in the dialog - e2e total 1610 is computed, not measured in a full run Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/core/model/animation.ts | 42 +++++++++++++++++++++------ src/editor/animation/RowDialog.ts | 18 ++---------- tests/core/animation.test.ts | 30 ++++++++++++++++++- tools/e2e/56-private-variables.mjs | 46 ++++++++++++++++++++++++++++-- 5 files changed, 109 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index dd272371..5ab83beb 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ npm run build # writes dist/ and regenerates index.html; the folder is wh The browser suite and the three example games need a machine with a GPU: ```bash -node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1609 checks (GM_STEPS=20 for one step) +node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1610 checks (GM_STEPS=20 for one step) # every tool signs in first – see the Accounts page node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs diff --git a/src/core/model/animation.ts b/src/core/model/animation.ts index f38659c1..b153d15c 100644 --- a/src/core/model/animation.ts +++ b/src/core/model/animation.ts @@ -1,14 +1,15 @@ import { isKindWith } from '../properties/kinds'; import { type PropertyDef, schemaFor, shaderParamRows } from '../properties/PropertySchema'; import { shaderOf } from '../properties/shaders'; -import type { - AnimationAsset, - AnimationKey, - AnimationTrack, - Asset, - EasingCurve, - EasingDirection, - KeyInterpolation, +import { + type AnimationAsset, + type AnimationKey, + type AnimationTrack, + type Asset, + type EasingCurve, + type EasingDirection, + isPrivateVariable, + type KeyInterpolation, } from './Asset'; import type { AssetType } from './AssetTypes'; import { type CurveLookup, curveValue, NO_CURVES } from './curve'; @@ -480,6 +481,31 @@ export function trackDef( return targetDefs(project, animation, track.target)?.find((d) => d.key === track.property); } +/** + * The rows the dialog offers for the object `target` names: what it can animate, less a behavior's + * private variables (#270) and what another row animates already. A row on a private variable keeps + * playing – only a new one is not offered. `changing` is the row being aimed elsewhere: its keys + * stay, so only properties of its kind are offered, and its own property is, private or not. + */ +export function offeredDefs( + project: Project, + animation: AnimationAsset, + target: string, + changing: AnimationTrack | null = null, +): PropertyDef[] { + const aimed = aimedAt(project, animation, target); + if (!aimed) return []; + const kind = changing ? (trackDef(project, animation, changing)?.kind ?? null) : null; + return animatableDefsOf(project, aimed.asset, aimed.bag).filter((def) => { + if (kind !== null && def.kind !== kind) return false; + const own = changing?.target === target && changing.property === def.key; + if (isPrivateVariable(aimed.asset, def.key) && !own) return false; + return !animation.tracks.some( + (t) => t.id !== changing?.id && t.target === target && t.property === def.key, + ); + }); +} + /** How the timeline names what a row aims at: the object (or the root itself) and the property. */ export function trackLabel(project: Project, animation: AnimationAsset, track: AnimationTrack): string { const aimed = aimedAt(project, animation, track.target); diff --git a/src/editor/animation/RowDialog.ts b/src/editor/animation/RowDialog.ts index aee0c0bc..717fd2cd 100644 --- a/src/editor/animation/RowDialog.ts +++ b/src/editor/animation/RowDialog.ts @@ -2,7 +2,7 @@ import { AddAnimationTrackCommand, RetargetAnimationTrackCommand, } from '../../core/commands/animationCommands'; -import { animationTargets, targetDefs, trackDef } from '../../core/model/animation'; +import { animationTargets, offeredDefs } from '../../core/model/animation'; import type { AnimationAsset, AnimationTrack } from '../../core/model/Asset'; import { findAsset } from '../../core/model/Project'; import { objectName } from '../../core/model/SceneObject'; @@ -96,22 +96,8 @@ export class RowDialog extends FormDialog { return choices; } - /** The properties the picked object offers, less those another row already animates. */ private properties(): PropertyDef[] { - const defs = targetDefs(this.state.project, this.asset, this.target); - if (!defs) return []; - const was = this.trackKind(); - return defs.filter((def) => { - if (was !== null && def.kind !== was) return false; - return !this.asset.tracks.some( - (t) => t.id !== this.track?.id && t.target === this.target && t.property === def.key, - ); - }); - } - - /** The kind the row being changed has; null when its target or property is gone, so it may go anywhere. */ - private trackKind(): string | null { - return this.track ? (trackDef(this.state.project, this.asset, this.track)?.kind ?? null) : null; + return offeredDefs(this.state.project, this.asset, this.target, this.track); } private openTargets(): void { diff --git a/tests/core/animation.test.ts b/tests/core/animation.test.ts index e68b090e..b1d88c1d 100644 --- a/tests/core/animation.test.ts +++ b/tests/core/animation.test.ts @@ -27,12 +27,13 @@ import { CommandError } from '../../src/core/commands/Command'; import { CreateContainerCommand } from '../../src/core/commands/containerCommands'; import { AddNodeCommand } from '../../src/core/commands/graphCommands'; import { SetOverrideCommand } from '../../src/core/commands/sceneObjectCommands'; -import { RenameVariableCommand } from '../../src/core/commands/variableCommands'; +import { RenameVariableCommand, SetVariablePrivateCommand } from '../../src/core/commands/variableCommands'; import { animatableDefs, animationDuration, frameCount, isPositionRow, + offeredDefs, pauseAnimation, PLACED_ROOT_ID, playAnimation, @@ -1226,6 +1227,33 @@ describe('behavior variables and shader parameters as rows (#104)', () => { f.stack.execute(new RetargetAnimationTrackCommand(walk.id, speed, moverObject, 'enabled')), ).toThrow(CommandError); }); + + it('does not offer a private variable for a new row, and a row on one keeps playing (#270)', () => { + const { f, box, mover, moverObject, walk } = rig(); + const offered = (changing: string | null = null) => + offeredDefs( + f.project, + current(f, walk), + moverObject, + changing === null ? null : rowOf(current(f, walk), changing), + ).map((d) => d.key); + const row = addRow(f, walk, 'speed', ramp, moverObject); + expect(offered()).toEqual(['enabled', 'runInEditor', 'runWhilePaused', 'x']); + f.stack.execute(new SetVariablePrivateCommand(mover.id, 'x', true)); + f.stack.execute(new SetVariablePrivateCommand(mover.id, 'speed', true)); + expect(offered()).toEqual(['enabled', 'runInEditor', 'runWhilePaused']); + // The row being changed keeps its own property on offer, and no other private one of its kind. + expect(offered(row)).toEqual(['speed']); + expect(keysOn(f, walk, moverObject)).toContain('speed'); + expect(trackLabel(f.project, current(f, walk), rowOf(current(f, walk), row))).toBe('Mover · speed'); + const { world, frame } = startWorld(f, { animations: true }); + playAnimation(world.findByObjectId(box)!, walk.id); + frame(0.1); + frame(0.5); + expect(world.findByObjectId(moverObject)!.props.speed).toBeCloseTo(50); + f.stack.execute(new SetVariablePrivateCommand(mover.id, 'x', false)); + expect(offered()).toContain('x'); + }); }); describe('the animation nodes', () => { diff --git a/tools/e2e/56-private-variables.mjs b/tools/e2e/56-private-variables.mjs index 2724a8f3..fb6fd060 100644 --- a/tools/e2e/56-private-variables.mjs +++ b/tools/e2e/56-private-variables.mjs @@ -2,11 +2,11 @@ * Private behavior variables (#257), with the real mouse: the lock next to a variable's Keep hides it * from the placed object – no row, no override left – and from the `+ Property` menu of a * `Properties` node. A port that names it by hand is still a property of the behavior, and in a run - * it reads the value. + * it reads the value. The row dialog of an animation does not offer it either (#270). * * Everything it adds carries a `pv56` id and goes again at the end. */ -import { MAKE_OBJECT } from './lib/scaffold.mjs'; +import { MAKE_OBJECT, openTab } from './lib/scaffold.mjs'; export async function privateVariables(t) { const { check, json, evaluate, mutate, click, press, typeText, sleep, shot, select, undo, stop } = t; @@ -156,12 +156,52 @@ export async function privateVariables(t) { JSON.stringify(logged), ); + // 56e. The row dialog of an animation over the scene offers hp for the placed behavior, and neither + // private variable (#270) – `mood` has no row, so it is not the "already has a row" rule that + // hides it. The row that aimed at timer before it went private stays, label and all. + await mutate(` + const p = __gm.state.project; + const brain = p.assets.find((a) => a.id === 'pv56-brain'); + brain.variables.push({ key: 'mood', label: 'mood', kind: 'number', default: 1 }); + brain.props.mood = 1; + brain.privateVariables = ['timer', 'mood']; + p.assets.push({ + id: 'pv56-walk', + type: 'animation', + name: 'Walk 56', + props: { fps: 10, playback: 'loop' }, + root: ${JSON.stringify(sceneId)}, + tracks: [{ id: 'pv56-row', target: 'pv56-obj', property: 'timer', keys: [] }], + }); + __gm.state.select({ kind: 'asset', assetId: 'pv56-walk' }); + `); + await openTab(t, 'animation'); + await sleep(300); + await press_(await json('__gm.animation.addRowAt()'), '+ Row'); + await evaluate("__gm.animation.rowDialogPick('pv56-obj', 'hp')"); + await sleep(150); + const dialog = await json('__gm.animation.rowDialog()'); + const rows = await json('__gm.animation.rows()'); + const properties = dialog?.properties ?? []; + check( + 'the row dialog offers hp and no private variable, the row on the private timer stays', + properties.includes('hp') && + !properties.includes('mood') && + !properties.includes('timer') && + dialog.ok === true && + JSON.stringify(rows) === JSON.stringify(['Brain 56 · timer']), + JSON.stringify({ properties, ok: dialog?.ok, rows }), + ); + await shot('04-row-dialog'); + await press_(await json("__gm.locate('Cancel', true)"), 'Cancel'); + // Clean up. await evaluate('__gm.state.select(null)'); await mutate(` const p = __gm.state.project; const scene = __gm.state.activeScene; scene.objects = scene.objects.filter((o) => o.id !== 'pv56-obj'); - p.assets = p.assets.filter((a) => a.id !== 'pv56-brain'); + p.assets = p.assets.filter((a) => a.id !== 'pv56-brain' && a.id !== 'pv56-walk'); `); + await openTab(t, 'scene'); }