From b900fb84105d449fe2a40703d6c7a6a7d148f693 Mon Sep 17 00:00:00 2001 From: Alex O'Neill Date: Sun, 25 Oct 2026 19:54:51 +0000 Subject: [PATCH] fix(director): reconcile only stale layers on same-shot seeks Compare declared visibility with the manager's effective intent, and treat in-flight transitions as stale so a toggle or superseded LOAD rollback that is still settling cannot slip past the check. Reconcile only those layers inside the lightweight seek instead of rebuilding the shot, and report a refusal without resetting the seek position or camera. Co-Authored-By: Claude Opus 5.5 --- changelog.d/1374-director-seek-visibility.md | 5 +- docs/CURRENT-STATE.md | 7 +- scripts/qa-director-timing.mjs | 9 ++- .../interactions/interactions.test.mjs | 2 +- src/scenes/director.js | 57 +++++++++++------ src/scenes/director.test.mjs | 64 ++++++++++++++++++- 6 files changed, 115 insertions(+), 29 deletions(-) diff --git a/changelog.d/1374-director-seek-visibility.md b/changelog.d/1374-director-seek-visibility.md index f7c366de9..d4f93a1bd 100644 --- a/changelog.d/1374-director-seek-visibility.md +++ b/changelog.d/1374-director-seek-visibility.md @@ -1,3 +1,4 @@ - Restore a shot's declared layer visibility when seeking within it after manual - layer changes. Refused restoration reports a failed seek, while undeclared - operator layers and the unchanged-stack shortcut are preserved (@oneill9, #1374). + layer changes, including toggles still in flight. Only changed layers are + reconciled; refused restoration reports a failed seek and keeps the last + position, while undeclared operator layers are preserved (@oneill9, #1374). diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 38736c32e..9eb26d911 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -507,9 +507,10 @@ and roll the short way for any authored angles. A seek to a shot's start, as a shot action makes, selects that shot rather than the end of the previous one. Same-shot seeking restores declared layer visibility after a manual toggle, -using the cancellable layer admission path and reporting refused restoration -as a failed seek. Undeclared operator layers remain untouched, and unchanged -declared visibility retains the lightweight seek path. +including one still in flight, by reconciling only the changed layers through +the cancellable layer admission path. Refused restoration reports a failed +seek and keeps the last settled position. Undeclared operator layers remain +untouched, and the shot is not rebuilt. See [camera directions](DIRECTOR-CAMERA.md). Realtime voice composes separate connection, response/tool, Radio handoff, input diff --git a/scripts/qa-director-timing.mjs b/scripts/qa-director-timing.mjs index fb96dd0bb..63934140e 100644 --- a/scripts/qa-director-timing.mjs +++ b/scripts/qa-director-timing.mjs @@ -122,10 +122,15 @@ try { const loaded = await director.loadShot(scene.id, scene.shots[0].id, { flyDuration: 0.2, }); - await dataManager.setEnabled(layerId, false, { origin: 'user' }); + const disabled = + (await dataManager.setEnabled(layerId, false, { origin: 'user' })) && + !dataManager.layers.get(layerId).enabled; const sought = await director.seekScene(scene.id, 0.5); return ( - loaded?.started && sought && dataManager.layers.get(layerId).enabled + loaded?.started && + disabled && + sought && + dataManager.layers.get(layerId).enabled ); } finally { director.stopScene(); diff --git a/src/director/interactions/interactions.test.mjs b/src/director/interactions/interactions.test.mjs index fa0aa10ae..3e80c3121 100644 --- a/src/director/interactions/interactions.test.mjs +++ b/src/director/interactions/interactions.test.mjs @@ -188,7 +188,7 @@ test('settled pack shots cannot take the same-shot seek shortcut after Stop rele _claimCameraOwnership: () => assert.fail('Must reload the released pack before camera seek'), }); - assert.equal(director._seekLoadedShot({ id: 'scene' }, { shot }), false); + assert.equal(await director._seekLoadedShot({ id: 'scene' }, { shot }), null); }); test('actions preserve camera refusal, layer admission signal and explicit transition cap', async () => { diff --git a/src/scenes/director.js b/src/scenes/director.js index 4ad02c8ac..7a78ef6b2 100644 --- a/src/scenes/director.js +++ b/src/scenes/director.js @@ -1516,9 +1516,13 @@ export class SceneDirector { return new Promise((resolve) => this._runIdleResolvers.add(resolve)); } - /** Apply a new time inside the already-loaded shot without rebuilding its stack. */ - _seekLoadedShot(scene, seekState) { - if (this._destroyed) return false; + /** + * Apply a new time inside the already-loaded shot without rebuilding its stack. + * @returns {Promise} Null when the shortcut does not apply and + * LOAD must rebuild the shot; otherwise whether the seek settled. + */ + async _seekLoadedShot(scene, seekState) { + if (this._destroyed) return null; const shot = seekState?.shot; if ( !scene || @@ -1528,28 +1532,41 @@ export class SceneDirector { this._loadedSceneId !== scene.id || this._selectedShotId !== shot.id ) - return false; + return null; const states = this._layerStatesForShot(scene, shot, { cameraSettled: seekState.cameraProgress >= 1, sceneSeek: seekState, }); - const layers = new Map( - this.dataManager.getAll().map((layer) => [layer.id, layer.enabled]), + // A manual toggle, or a superseded LOAD transition whose rollback is still + // settling, leaves declared layers off the shot's stack. Reconcile only + // those through the cancellable admission path. + const staleStates = Object.fromEntries( + Object.entries(states).filter(([id, state]) => { + const lifecycle = this.dataManager.getLayerLifecycleState(id); + return ( + lifecycle && + (lifecycle.lifecycleState === 'enabling' || + lifecycle.lifecycleState === 'disabling' || + this.dataManager.isEffectivelyEnabled(id) !== !!state?.enabled) + ); + }), ); - // A manual toggle invalidates the loaded stack. Let LOAD reconcile it - // through the normal cancellable admission path before reporting success. - if ( - sceneLayerPlan(states, new Set(layers.keys())).some( - ({ id, enabled }) => Boolean(layers.get(id)) !== enabled, - ) - ) - return false; - if (!this._claimCameraOwnership()) return false; + if (!this._claimCameraOwnership()) return null; this._cancelActiveSceneTravel(); this._loadAbort?.abort(); this._loadAbort = null; - this._loadGeneration += 1; + const generation = ++this._loadGeneration; this._clock.stopShot(); + if (Object.keys(staleStates).length) { + const controller = new AbortController(); + this._loadAbort = controller; + const layerResult = await this._applyLayerStates( + staleStates, + this._loadToken(generation, controller.signal), + ); + if (this._loadAbort === controller) this._loadAbort = null; + if (layerResult.cancelled || layerResult.refused.length) return false; + } let applied = true; for (const [layerId, state] of Object.entries(states)) { if (!state?.enabled || !state.params) continue; @@ -1561,7 +1578,7 @@ export class SceneDirector { applied = false; } } - if (!applied) return false; + if (!applied) return null; this._setCameraView( seekState.camera || resolveCameraPose(scene, shot.camera), ); @@ -1597,7 +1614,11 @@ export class SceneDirector { return false; const seekState = this._sceneSeekState(scene, progress); if (!seekState) return false; - if (this._seekLoadedShot(scene, seekState)) return true; + const seeked = await this._seekLoadedShot(scene, seekState); + if (seeked !== null) + return ( + seeked && !this._destroyed && generation === this._sceneSeekGeneration + ); const result = await this._loadShot(scene.id, seekState.shot.id, { sceneSeek: seekState, }); diff --git a/src/scenes/director.test.mjs b/src/scenes/director.test.mjs index ccbd5e38d..968ffa1ed 100644 --- a/src/scenes/director.test.mjs +++ b/src/scenes/director.test.mjs @@ -175,7 +175,7 @@ test('scene clock seek resolves the exact shot phase and camera in both directio }); test('same-shot seeking restores declared layer visibility after manual changes', async () => { - const { director, dataManager, restore } = makeDirector(); + const { director, dataManager, styleManager, restore } = makeDirector(); try { const shot = director._project.scenes[0].shots[0]; shot.layers = { flights: { enabled: true }, traffic: { enabled: false } }; @@ -184,8 +184,11 @@ test('same-shot seeking restores declared layer visibility after manual changes' await dataManager.setEnabled('traffic', true); await dataManager.setEnabled('satellites', true); dataManager.setEnabledCalls.length = 0; + const visualCalls = styleManager.visualCalls.length; assert.equal(await director.seekScene('scene-1', 0.25), true); + assert.equal(styleManager.visualCalls.length, visualCalls, 'do not rebuild the shot'); + assert.equal(director.getPlaybackTimingState().snapshot.sceneProgress, 0.25); const enabled = new Map(dataManager.getAll().map(({ id, enabled }) => [id, enabled])); assert.equal(enabled.get('flights'), true, 'restore the shot\'s enabled layer'); assert.equal(enabled.get('traffic'), false, 'restore the shot\'s disabled layer'); @@ -199,17 +202,58 @@ test('same-shot seeking restores declared layer visibility after manual changes' test('same-shot seeking reports refusal when a manually disabled layer cannot be restored', async () => { let refuse = false; - const { director, dataManager, restore } = makeDirector({ + const { director, dataManager, styleManager, restore } = makeDirector({ data: { refuse: () => refuse }, }); try { await director.loadShot('scene-1', 'shot-a', { flyDuration: 0 }); + assert.equal(await director.seekScene('scene-1', 0.1), true); await dataManager.setEnabled('flights', false); refuse = true; + const visualCalls = styleManager.visualCalls.length; assert.equal(await director.seekScene('scene-1', 0.25), false); assert.equal(dataManager.getAll().find(({ id }) => id === 'flights').enabled, false); - assert.equal(director.getPlaybackTimingState().activeTimers, 0); + const timing = director.getPlaybackTimingState(); + assert.equal(timing.activeTimers, 0); + assert.ok(Math.abs(timing.snapshot.sceneProgress - 0.1) < 1e-9, 'keep the last settled seek position'); + assert.equal(styleManager.visualCalls.length, visualCalls, 'do not rebuild the shot'); + } finally { + await director.destroy(); + restore(); + } +}); + +test('same-shot seeking restores visibility a manual toggle is still changing', async () => { + const { director, dataManager, restore } = makeDirector(); + try { + await director.loadShot('scene-1', 'shot-a', { flyDuration: 0 }); + const disabling = dataManager.setEnabled('flights', false); + + assert.equal(await director.seekScene('scene-1', 0.25), true); + await disabling; + await settle(); + assert.equal(dataManager.getAll().find(({ id }) => id === 'flights').enabled, true); + } finally { + await director.destroy(); + restore(); + } +}); + +test('same-shot seeking restores a layer whose superseded LOAD transition rolls back', async () => { + const { director, dataManager, restore } = makeDirector(); + try { + await director.loadShot('scene-1', 'shot-a', { flyDuration: 0 }); + await dataManager.setEnabled('flights', false); + dataManager.setEnabledCalls.length = 0; + const reload = director.loadShot('scene-1', 'shot-a', { flyDuration: 0 }); + for (let i = 0; i < 20 && !dataManager.setEnabledCalls.length; i++) await Promise.resolve(); + assert.equal(dataManager.getLayerLifecycleState('flights').lifecycleState, 'enabling'); + + assert.equal(await director.seekScene('scene-1', 0.25), true); + await reload; + await settle(); + assert.equal(dataManager.getAll().find(({ id }) => id === 'flights').enabled, true); } finally { await director.destroy(); restore(); @@ -707,6 +751,7 @@ function installSceneRuntime(project = PROJECT_FIXTURE, { storage } = {}) { */ function fakeDataManager({ registered = REGISTERED, refuse = () => false } = {}) { const enabled = new Map(); + const pending = new Map(); const setEnabledCalls = []; const setParamsCalls = []; const committed = []; @@ -719,12 +764,25 @@ function fakeDataManager({ registered = REGISTERED, refuse = () => false } = {}) setParamsCalls, committed, getAll: () => registered.map((id) => ({ id, enabled: !!enabled.get(id) })), + getLayerLifecycleState: (id) => { + if (!registered.includes(id)) return null; + const on = !!enabled.get(id); + const target = pending.get(id); + return { + enabled: on, + lifecycleState: + target === undefined ? (on ? 'enabled' : 'disabled') : target ? 'enabling' : 'disabling', + }; + }, + isEffectivelyEnabled: (id) => pending.get(id) ?? !!enabled.get(id), getLayerParams: () => null, async setEnabled(id, shouldEnable, { signal } = {}) { setEnabledCalls.push({ id, enabled: shouldEnable, signal }); if (refuse(id, shouldEnable)) return false; // Yield once so a stop landing during the transition is observable. + pending.set(id, shouldEnable); await Promise.resolve(); + if (pending.get(id) === shouldEnable) pending.delete(id); if (signal?.aborted) return false; enabled.set(id, shouldEnable); committed.push({ id, enabled: shouldEnable });