From d1bdb57df97c2f39d77a325a2358c389261cafc1 Mon Sep 17 00:00:00 2001 From: Microck Date: Tue, 8 Sep 2026 11:23:02 +0000 Subject: [PATCH] fix(startpos): restore captured poses and avoid repeated warm-load stalls --- CHANGELOG.md | 6 ++ Source/Actions/akron-startpos-actions.cs | 12 ++- Source/Commands/akron-qa-commands.cs | 4 + Source/SaveLoad/akron-reconstruction-graph.cs | 25 +++--- docs/feature-guide/startpos.mdx | 12 +++ docs/startpos-restore-verification.md | 86 +++++++++++++++++++ tests/startpos-persistence-tests.cs | 3 +- tests/startpos-reconstruction-tests.cs | 66 +++++++++++++- 8 files changed, 190 insertions(+), 24 deletions(-) create mode 100644 docs/startpos-restore-verification.md diff --git a/CHANGELOG.md b/CHANGELOG.md index bddb3c9..432a280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ This project uses version tags that match the mod version in `everest.yaml`, whi ## Unreleased +### Fixed + +- Restore StartPos snapshots whose running CrushBlock routine still holds a removed sound component, including when preparing other slots for instant loads. +- Keep the captured animation and frame when loading an ordinary StartPos, instead of recalculating the player's pose. +- Avoid a GPU readback stall when restoring StartPos render buffers, reducing repeated-load delays. + ## Akron Beta 79 ### Fixed diff --git a/Source/Actions/akron-startpos-actions.cs b/Source/Actions/akron-startpos-actions.cs index 81f2611..f4e45db 100644 --- a/Source/Actions/akron-startpos-actions.cs +++ b/Source/Actions/akron-startpos-actions.cs @@ -752,8 +752,8 @@ AkronStartPos startPos } private static void RefreshStartPosPlayerPose(Player player, bool clearMovementInput) { - // Placement and an old persisted slot can both change collision context without - // running Player.Update. Refresh the derived ground input, then let Celeste + // Spawn configuration can change collision context without running + // Player.Update. Refresh the derived ground input, then let Celeste // choose edge, edgeBack, dangling, falling, or the ordinary idle pose before // the restored room renders its first frame. player.onGround = player.OnGround(); @@ -1330,15 +1330,13 @@ Dictionary currentStartPositionsByMap if (restored == AkronSaveLoadResult.Success) { Level restoredLevel = Engine.Scene as Level ?? currentLevel; if (restoredLevel.Tracker.GetEntity() is Player player) { - // Position is part of every StartPos entry, so it remains the load-boundary - // contract even for snapshots written before placement pose refresh existed. - // Spawn configuration is metadata too: applying it after every cold or warm - // restore prevents a stale saved animation from reaching the first render. + // Explicit spawn options can change the captured pose. An ordinary + // snapshot already holds its exact animation and frame, so running + // UpdateSprite again would change the state the player saved. if (startPos.UsesSpawnConfig) { ApplyStartPosPlayerConfiguration(restoredLevel, player, startPos); } else { player.Position = ClampToRoom(restoredLevel, startPos.Position); - RefreshStartPosPlayerPose(player, clearMovementInput: false); } } ReportStartPosRestoreTiming(restoreTimer.Elapsed, usedSnapshot, prewarmHitsBeforeLoad); diff --git a/Source/Commands/akron-qa-commands.cs b/Source/Commands/akron-qa-commands.cs index 13f1cc0..e2c4638 100644 --- a/Source/Commands/akron-qa-commands.cs +++ b/Source/Commands/akron-qa-commands.cs @@ -817,6 +817,8 @@ private static void LogControlledPlayerProbe(Level level, string prefix) { Log(prefix + "-position: " + FormatVector(player.Position)); Log(prefix + "-speed: " + FormatVector(player.Speed)); Log(prefix + "-facing: " + player.Facing); + Log(prefix + "-animation: " + player.Sprite.CurrentAnimationID); + Log(prefix + "-animation-frame: " + player.Sprite.CurrentAnimationFrame.ToString(CultureInfo.InvariantCulture)); Log(prefix + "-state: " + player.StateMachine.State.ToString(CultureInfo.InvariantCulture)); Log(prefix + "-stamina: " + player.Stamina.ToString("0.##", CultureInfo.InvariantCulture)); Log(prefix + "-dashes: " + player.Dashes.ToString(CultureInfo.InvariantCulture)); @@ -833,6 +835,8 @@ private static void RecordControlledPlayerProbe(Level level, string prefix) { AkronAutomationService.RecordOutput(prefix + "-position: " + FormatVector(player.Position)); AkronAutomationService.RecordOutput(prefix + "-speed: " + FormatVector(player.Speed)); AkronAutomationService.RecordOutput(prefix + "-facing: " + player.Facing); + AkronAutomationService.RecordOutput(prefix + "-animation: " + player.Sprite.CurrentAnimationID); + AkronAutomationService.RecordOutput(prefix + "-animation-frame: " + player.Sprite.CurrentAnimationFrame.ToString(CultureInfo.InvariantCulture)); AkronAutomationService.RecordOutput(prefix + "-state: " + player.StateMachine.State.ToString(CultureInfo.InvariantCulture)); AkronAutomationService.RecordOutput(prefix + "-stamina: " + player.Stamina.ToString("0.##", CultureInfo.InvariantCulture)); AkronAutomationService.RecordOutput(prefix + "-dashes: " + player.Dashes.ToString(CultureInfo.InvariantCulture)); diff --git a/Source/SaveLoad/akron-reconstruction-graph.cs b/Source/SaveLoad/akron-reconstruction-graph.cs index fcc2d89..3a87018 100644 --- a/Source/SaveLoad/akron-reconstruction-graph.cs +++ b/Source/SaveLoad/akron-reconstruction-graph.cs @@ -612,12 +612,10 @@ public static void RestoreBestEffort(IReadOnlyList // Gameplay buffers are derived presentation state. A camera or graphics // mod can resize them after Set, so an incompatible buffer must not turn // an otherwise valid StartPos into a half-applied failed restore. + // SetData writes the validated pixel payload. Reading it straight + // back stalls the GPU on every warm Load; compare pixels in QA. if (!Adapter.RestoreExisting(snapshot.Payload, renderTarget)) { LogSkippedBuffer(field.Name, "current render target dimensions differ"); - continue; - } - if (!Adapter.Verify(snapshot.Payload, renderTarget)) { - LogSkippedBuffer(field.Name, "restored pixels differ"); } } catch (Exception exception) { LogSkippedBuffer(field.Name, exception.GetType().Name + ": " + exception.Message); @@ -5259,15 +5257,18 @@ Type type return false; } - // Component.RemoveSelf leaves Component.Entity intact. CrushBlock relies on - // that while its authenticated attack iterator's delayed-removal closure - // keeps the old SoundSource alive. The two independent back-references must - // name the same entity, so a closure cannot adopt another entity's component. + // A routine can retain a component after Component.Removed clears Entity. + // The proved iterator and its declared closure local still own that detached + // object. An attached component must belong to the iterator's entity instead. AkronReconstructionValue iteratorOwner = FindReferenceField(iteratorNode, "<>4__this"); - AkronReconstructionValue componentOwner = FindReferenceField(node, "k__BackingField"); + AkronReconstructionValue componentOwner = node.FieldsOrNull? + .FirstOrDefault(field => field.Name == "k__BackingField" && + field.DeclaringTypeName == typeof(Component).AssemblyQualifiedName)?.Value; return iteratorOwner != null && componentOwner != null && - iteratorOwner.NodeId == componentOwner.NodeId; + (componentOwner.Kind == NullValueKind || + (componentOwner.Kind == ReferenceValueKind && + iteratorOwner.NodeId == componentOwner.NodeId)); } // Everest wraps every coroutine frame in SwapImmediatelyExtension's @@ -5758,7 +5759,9 @@ target.Kind is AnchorKind or PersistentResourceKind or DelegateKind or EventInst (target.ParentKind == "field" || target.ParentKind == "array"); bool reconstructedOwnedComponentAlias = authenticatedOwnedComponent && - IsAuthenticatedOwnedComponentAlias(target, edgeParent); + (IsAuthenticatedOwnedComponentAlias(target, edgeParent) || + (savedOwnerEdge && target.ParentNodeId == edgeParent.Id && + IsAuthenticatedIteratorClosureOwnedComponent(target, targetType))); bool reconstructedOwnedComponentOwnerEdge = authenticatedEdgeParentOwnedComponent && edgeField?.Name == "k__BackingField" && diff --git a/docs/feature-guide/startpos.mdx b/docs/feature-guide/startpos.mdx index f436137..77ab254 100644 --- a/docs/feature-guide/startpos.mdx +++ b/docs/feature-guide/startpos.mdx @@ -23,6 +23,18 @@ captured frame. Akron stores this state in a local v10 snapshot file so it survives closing and restarting Celeste. Set and Load hold the room state until Celeste renders the restored frame. +An ordinary Load keeps the captured player animation and frame. Explicit spawn +options can change that pose; Load does not otherwise recalculate it. + +Captured coroutine state includes removed components still held by a running +routine, such as a CrushBlock's sound after its delayed removal. Those +components restore as detached objects, without adding them back to the room. + +A successful first Load prepares the other slots in the active chapter within +the machine's memory budget. Later Loads reuse those room states. Restoring +the saved render-buffer pixels must not add a GPU readback to each Load; +pixel comparisons belong in the QA capture checks. + When **Wait for input after load** is enabled, Akron keeps gameplay paused after the restored frame until a fresh gameplay input arrives. Controls held before the Load do not release the pause. Backdrops and respawn wipes keep moving while diff --git a/docs/startpos-restore-verification.md b/docs/startpos-restore-verification.md new file mode 100644 index 0000000..9f1ca48 --- /dev/null +++ b/docs/startpos-restore-verification.md @@ -0,0 +1,86 @@ +# StartPos restore verification + +Verified on 2026-09-08 with the user's five supplied Reflection snapshots. + +## Fixes + +- A CrushBlock attack iterator can retain its sound after `Component.Removed` + clears `Component.Entity`. Reconstruction now accepts that detached component + through the already-proved iterator closure and validates its canonical + reference. Attached components still have to name the iterator's entity. +- Ordinary StartPos loads preserve the captured animation and frame. Explicit + spawn configuration still refreshes the pose when applying its overrides. +- Gameplay buffer restoration uploads the validated saved pixels without + immediately reading them back from the GPU. Pixel comparison remains a QA + check rather than a synchronous step in every warm load. + +## Supplied pack + +The pack's SHA-256 is +`d27cb79bf2576ada876e82e5cde4df11466bd9b3b3dfe45081b85ba332d18f42`. +Public beta 79 imports its `akron-setup-v9` container. The original verification +build also contained a separate, unpublished setup-compression change, so the +pack was imported with beta 79 first. Verification then used those same native +`akron-reconstruction-v10` snapshot files with the fixes applied. This PR is +based on beta 79 and excludes that compression change; its setup format remains +`akron-setup-v9`. No snapshot re-export or compatibility path was required. + +Beta 79 reproduced the removed-sound refusal in slots 4 and 5. A later slot 2 +load itself took 62.5 ms, but preparation spent another 17,067.6 ms retrying +those failing slots. This explains why an already-warm slot still appeared to +load slowly. The fixed build prepares all five slots successfully; subsequent +loads report zero additional preparation work. + +## Linux Mint game checks + +Used the documented jc141 installation with Everest 1.6418. The supplied +snapshots require Vidcutter, so Vidcutter 1.12.1 was installed. Other optional +mods were temporarily disabled after the full mod set exhausted the machine's +available memory. The original mod blacklist was backed up and restored. +Mod and save backups are under +`/home/microck/akron-startpos-pack-test-20260908/`. + +End-of-frame player probes matched the supplied snapshot state: + +| Slot | Room | Position | Animation | Frame | +| --- | --- | --- | --- | --- | +| 1 | b-00b | 9657, -2968 | idle | 0 | +| 2 | b-00 | 9842, -2800 | edge | 7 | +| 3 | b-02 | 10740, -2552 | idle | 0 | +| 4 | b-02 | 10783, -2136 | idle | 4 | +| 5 | b-02 | 10604, -1685 | empty ID, as captured | 1 | + +QA readback of the presented 320x180 Level buffer matched the saved SHA-256 +for all five slots. The game was also inspected visually. These buffer checks +confirm pixel restoration; the separate player probes check reconstructed +player state. A saved-buffer hash alone cannot prove subsequent simulation. + +Before removing the redundant readback, temporary phase probes measured +697.6-945.7 ms inside buffer restoration on the slower repeated loads. After +removal, the same sequence measured 2.7-4.0 ms there. Warm restore totals for +slots 2, 3, 4, 5, 1, 2 were 46.2, 111.0, 83.3, 102.9, 39.4, and 58.8 ms. +All five remained warm, using 316.2 MB, with no preparation retries. +The temporary phase probes were removed from the final build. +The clean test archive was built with zero warnings and errors and installed +on Mint. Its SHA-256 is +`74e5057197e9c1e5a792edb0db4930ca2de9074dce1ec0ef7f4ed720a511e85c`. +Celeste was stopped after verification. + +## Automated checks + +- Original verification build: 1,900 Release tests passed, zero failed or skipped. +- Regression coverage includes Celeste's actual CrushBlock attack iterator, + hoisted closure, detached SoundSource, and serialization round trip. +- Attached component ownership checks remain covered. +- Controlled live capture/load checks separately verified that ordinary loads + preserve animation rather than recalculating it from movement speed. + +## Limits + +Cold reconstruction is still expensive: the timed fixed-build run spent +10.3 seconds loading slot 1, then 58.7 seconds preparing the other four slots. +This change fixes failed preparation and repeated-load stalls, not that initial +reconstruction cost. Timings apply to the reduced mod configuration above. + +The separate preparation-recovery guard failure in the original log was not +reproduced. No guard was bypassed or weakened, and this case remains unresolved. diff --git a/tests/startpos-persistence-tests.cs b/tests/startpos-persistence-tests.cs index 0c3a6ca..1034b92 100644 --- a/tests/startpos-persistence-tests.cs +++ b/tests/startpos-persistence-tests.cs @@ -2755,7 +2755,7 @@ public void StartPosCaptureOnlyBlocksDuringTheNativeSetBoundary() { } [Fact] - public void EveryStartPosRefreshesTheNativePoseAtCaptureOrLoadBoundary() { + public void ConfiguredStartPosRefreshesTheNativePoseAtCaptureOrLoadBoundary() { string source = File.ReadAllText(GetActionsSourcePath()); string placement = SourceSlice( source, @@ -2786,7 +2786,6 @@ public void EveryStartPosRefreshesTheNativePoseAtCaptureOrLoadBoundary() { Assert.True(successfulRestore >= 0 && applyConfiguration > successfulRestore && timingReport > applyConfiguration); Assert.Contains("ApplyStartPosPlayerConfiguration(restoredLevel, player, startPos);", load); - Assert.Contains("RefreshStartPosPlayerPose(player, clearMovementInput: false);", load); Assert.Contains("startPos.Position", load); string playerSnapshot = SourceSlice( diff --git a/tests/startpos-reconstruction-tests.cs b/tests/startpos-reconstruction-tests.cs index 9b0b82d..21ef34a 100644 --- a/tests/startpos-reconstruction-tests.cs +++ b/tests/startpos-reconstruction-tests.cs @@ -5789,9 +5789,18 @@ public void AMidFlightIteratorClosureRestoresWhenTheFreshRoutineIsIdle() { Assert.Single(GetRuntimeField>(freshOwner.Routine!, "enumerators")); } - [Fact] - public void IteratorClosureCanRetainARuntimeComponentOwnedByTheSameEntity() { - (SavedSceneRoot saved, _) = CreateClosureRoutineScene(midFlight: true, withOwnedComponent: true); + [Theory] + [InlineData(false)] + [InlineData(true)] + public void IteratorClosureCanRetainARuntimeComponentOwnedByTheSameEntity(bool removed) { + (SavedSceneRoot saved, ClosureRoutineEntity owner) = CreateClosureRoutineScene(midFlight: true, withOwnedComponent: true); + if (removed) { + IEnumerator running = GetRuntimeField>(owner.Routine!, "enumerators").Peek(); + object capturedClosure = GetRuntimeField(running, "<>8__1"); + OwnedTestComponent capturedComponent = GetRuntimeField(capturedClosure, "component"); + // Component.Removed clears Entity while the iterator retains its local. + SetRuntimeField(capturedComponent, "k__BackingField", null); + } (SavedSceneRoot baseline, _) = CreateClosureRoutineScene(midFlight: false, withOwnedComponent: true); AkronReconstructionGraph graph = new AkronReconstructionGraph(IsLiveResource); AkronReconstructionCapture capture = graph.Capture(saved, baseline); @@ -5810,7 +5819,56 @@ public void IteratorClosureCanRetainARuntimeComponentOwnedByTheSameEntity() { "enumerators").Peek(); object closure = GetRuntimeField(iterator, "<>8__1"); OwnedTestComponent component = GetRuntimeField(closure, "component"); - Assert.Same(freshOwner, GetRuntimeField(component, "k__BackingField")); + Assert.Same(removed ? null : freshOwner, GetRuntimeField(component, "k__BackingField")); + } + + [Fact] + public void CrushBlockAttackRestoresItsRemovedSoundSource() { + SavedSceneRoot saved = CreateCrushBlockRoutineScene(midFlight: true); + SavedSceneRoot baseline = CreateCrushBlockRoutineScene(midFlight: false); + AkronReconstructionGraph graph = new AkronReconstructionGraph(IsLiveResource); + AkronReconstructionCapture capture = graph.Capture(saved, baseline); + Assert.True(capture.Success, capture.Error); + AkronReconstructionNode soundNode = Assert.Single(capture.Document.Nodes, node => + node.TypeName == typeof(SoundSource).AssemblyQualifiedName); + Assert.Contains("<>8__1.sfx", soundNode.Path); + + AkronReconstructionDocument document = graph.Deserialize(graph.Serialize(capture.Document)); + AkronReconstructionRestore restore = graph.Restore(document, CreateCrushBlockRoutineScene(midFlight: false)); + + Assert.True(restore.Success, restore.Error); + SoundSource sound = Assert.IsType(restore.Objects[soundNode.Id]); + Assert.Null(GetRuntimeField(sound, "k__BackingField")); + } + + private static SavedSceneRoot CreateCrushBlockRoutineScene(bool midFlight) { + Scene scene = (Scene) RuntimeHelpers.GetUninitializedObject(typeof(Scene)); + EntityList entities = LinkSceneEntities(scene, CreateDetachedEntityList()); + CrushBlock owner = CreateUninitializedEntity(); + ComponentList components = CreateDetachedComponentList(owner); + SetRuntimeField(owner, "k__BackingField", scene); + SetRuntimeField(owner, "k__BackingField", CreateEntityId("b-00b", 9)); + Coroutine routine = (Coroutine) RuntimeHelpers.GetUninitializedObject(typeof(Coroutine)); + SetRuntimeField(routine, "k__BackingField", owner); + Stack iterators = new Stack(); + if (midFlight) { + // Use Celeste's actual iterator, hoisted closure and SoundSource types. + // Only the stopped sound remains; the delayed alarm already removed it. + MethodInfo attack = typeof(CrushBlock).GetMethod("AttackSequence", BindingFlags.Instance | BindingFlags.NonPublic)!; + Type iteratorType = attack.GetCustomAttribute()!.StateMachineType; + object iterator = RuntimeHelpers.GetUninitializedObject(iteratorType); + FieldInfo closureField = iteratorType.GetField("<>8__1", BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)!; + object closure = RuntimeHelpers.GetUninitializedObject(closureField.FieldType); + SetRuntimeField(iterator, "<>4__this", owner); + closureField.SetValue(iterator, closure); + SetRuntimeField(closure, "sfx", RuntimeHelpers.GetUninitializedObject(typeof(SoundSource))); + iterators.Push((IEnumerator) iterator); + } + SetRuntimeField(routine, "enumerators", iterators); + SetRuntimeField(components, "components", new List { routine }); + SetRuntimeField(components, "current", new HashSet { routine }); + AddDetachedEntity(entities, owner); + return new SavedSceneRoot { Scene = scene, Entities = entities }; } // The containment side of the closure-lambda licence: a document that moves