perf(animation): reuse persistent scratch buffers in AnimationGraph/AnimationStateMachine

CLAUDE.md

@@ -42,6 +42,7 @@ Read before doing anything non-trivial; do not duplicate their content here:
 - [docs/agent-rules/testing-architecture.md](docs/agent-rules/testing-architecture.md) — renderer 11-layer pyramid, Functional/cross-subsystem axis, registration contract.
 - [docs/agent-rules/glsl-shaders.md](docs/agent-rules/glsl-shaders.md) — SPIR-V constraints, UBO bindings, MRT output.
 - [docs/agent-rules/render-pipeline-caches.md](docs/agent-rules/render-pipeline-caches.md) — process-wide render caches (blackboard fingerprint, etc.) must invalidate on every topology reset, not just on a fingerprint change; hash `RenderGraph::GetTopologyGeneration()` into per-frame cache keys (issue #530).
+- [docs/agent-rules/per-frame-scratch-reuse.md](docs/agent-rules/per-frame-scratch-reuse.md) — a per-tick hot-path local (`std::vector` scratch built inside `AnimationGraph::Update`/`AnimationStateMachine::Update`) should become persistent per-instance state instead, but only after checking three things: every writer fully overwrites the buffer (or `.clear()`s it first), the owning instance isn't shared across entities, and the call isn't running on an unsynchronized `.Parallelizable()` path (issue #445).
 - [docs/agent-rules/binary-format-versioning.md](docs/agent-rules/binary-format-versioning.md) — versioning a fixed-order binary archive (save-game / asset-pack): relax the header check to a range, thread the file's version into `FArchive`, gate each new field behind `HasFieldsSince`/`ar.GetArchiveVersion()` instead of trusting the header check to exclude old data (issue #454).
 - [docs/agent-rules/sonarqube-review-alignment.md](docs/agent-rules/sonarqube-review-alignment.md) — what SonarQube's *C++ Extended* profile suppresses / relaxes / enforces; **read before `/code-review`** so local review matches the cloud scan (don't re-flag our deliberate suppressions, use the same thresholds, pre-empt the high-severity rules).
 - [docs/ops/build.md](docs/ops/build.md) — full Windows / Linux / WSL build matrix.

OloEngine/src/OloEngine/Animation/AnimationGraph.cpp

@@ -41,7 +41,13 @@ namespace OloEngine
         // Start each bone at its bind-pose local transform so a full-weight
         // Override layer that leaves a bone un-keyed rests it at bind pose
         // rather than blending toward identity.
-        std::vector<BoneTransform> accumulatedTransforms;
+        //
+        // Reuse persistent per-instance scratch (m_ScratchAccumulated /
+        // m_ScratchLayer) instead of Update()-local vectors — see the member
+        // declaration comment in AnimationGraph.h. Every callee below only
+        // ever resize()s these, so after the first tick / bone-count change
+        // this loop performs zero heap allocations (issue #445).
+        std::vector<BoneTransform>& accumulatedTransforms = m_ScratchAccumulated;
         BlendTree::FillBindPose(ctx, boneCount, accumulatedTransforms);
 
         // Evaluate each layer bottom-to-top
@@ -52,7 +58,7 @@ namespace OloEngine
                 continue;
             }
 
-            std::vector<BoneTransform> layerTransforms;
+            std::vector<BoneTransform>& layerTransforms = m_ScratchLayer;
             layer.StateMachine->Update(dt, Parameters, boneCount, ctx, layerTransforms);
 
             if (layerTransforms.size() < boneCount)

OloEngine/src/OloEngine/Animation/AnimationGraph.h

@@ -14,7 +14,7 @@
 
 namespace OloEngine
 {
     class AnimationGraph : public RefCounted
     {
       public:
         AnimationParameterSet Parameters;
@@ -70,5 +70,17 @@
         static glm::mat4 BoneTransformToMatrix(const BoneTransform& transform);
 
         static const std::string s_EmptyString;
+
+        // Per-instance pose-evaluation scratch, reused across Update() calls
+        // (issue #445): every callee here (FillBindPose / StateMachine::Update /
+        // BlendTree::Evaluate) only ever resize()s these, never clears+regrows
+        // from empty, so keeping them as instance storage instead of Update()
+        // locals turns the per-frame allocation into a one-time warm-up cost
+        // (first tick / bone-count change) instead of steady-state heap churn.
+        // Each entity owns its own AnimationGraph instance (Scene.cpp Clone()s
+        // one per entity) and the AnimationGraph/AnimationGraphSystem gameplay
+        // step is not marked .Parallelizable(), so this is safe without locking.
+        std::vector<BoneTransform> m_ScratchAccumulated;
+        std::vector<BoneTransform> m_ScratchLayer;
     };
 } // namespace OloEngine

OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp

@@ -142,9 +142,12 @@ namespace OloEngine
                 return;
             }
 
-            // Cross-fade between current and target states
-            std::vector<BoneTransform> currentTransforms;
-            std::vector<BoneTransform> targetTransforms;
+            // Cross-fade between current and target states. Reuse persistent
+            // scratch (see the member declaration comment) instead of locals
+            // that would heap-allocate on every ticked frame of the transition.
+            std::vector<BoneTransform>& currentTransforms = m_ScratchCurrentTransforms;
+            std::vector<BoneTransform>& targetTransforms = m_ScratchTargetTransforms;
+            targetTransforms.clear();
 
             {
                 f32 normalizedTime = 0.0f;

OloEngine/src/OloEngine/Animation/AnimationStateMachine.h

@@ -78,5 +78,12 @@ namespace OloEngine
 
         // Cached for GetCurrentStateNormalizedTime
         AnimationParameterSet m_LastParams;
+
+        // Cross-fade scratch, reused across Update() calls while m_InTransition
+        // is true (issue #445) — Evaluate() always resize()s + fully overwrites
+        // these, so reusing them avoids a heap allocation on every ticked frame
+        // of every transition instead of just the steady (non-transitioning) case.
+        std::vector<BoneTransform> m_ScratchCurrentTransforms;
+        std::vector<BoneTransform> m_ScratchTargetTransforms;
     };
 } // namespace OloEngine

OloEngine/tests/AnimationStateMachineTest.cpp

@@ -7,6 +7,8 @@
 #include "OloEngine/Animation/AnimationParameter.h"
 #include "OloEngine/Animation/BlendNode.h"
 #include "OloEngine/Animation/AnimationClip.h"
+#include "OloEngine/Animation/AnimationGraph.h"
+#include "OloEngine/Animation/AnimationLayer.h"
 #include "Animation/AnimationTestHelpers.h"
 
 using namespace OloEngine;
@@ -377,3 +379,193 @@ TEST(AnimationStateMachineTest, CrossFadeBlending)
     EXPECT_EQ(sm.GetCurrentStateName(), "Walk");
     EXPECT_FALSE(sm.IsInTransition());
 }
+
+//==============================================================================
+// Scratch-reuse regression tests (issue #445).
+//
+// AnimationStateMachine::Update and AnimationGraph::Update used to build
+// std::vector<BoneTransform> cross-fade / layer-accumulation scratch as
+// function-local variables, heap-allocating on every ticked transition frame
+// (state machine) or every layer of every tick (graph). Both now reuse
+// persistent per-instance scratch members instead. These tests exercise the
+// specific hazard that reuse introduces: stale data from a PREVIOUS
+// transition/layer bleeding into the current one because the buffer was
+// never cleared, only resized. Every Evaluate() path fully overwrites
+// [0, boneCount) rather than only some indices, but this test would break if
+// that invariant is ever violated for a reused buffer, whereas the old
+// fresh-locals code masked the bug entirely (an under-filled local vector is
+// still empty/default rather than stale).
+//==============================================================================
+
+TEST(AnimationStateMachineTest, SequentialCrossFadesDoNotLeakScratchBetweenTransitions)
+{
+    // Idle(x=0) -> Walk(x=10) -> Run(x=20): each clip is distinguishable so a
+    // stale carry-over from the FIRST transition's scratch would show up as a
+    // wrong blended value in the SECOND transition.
+    auto idleClip = CreateTestClip("Idle", 1.0f, glm::vec3(0.0f), glm::vec3(0.0f));
+    auto walkClip = CreateTestClip("Walk", 1.0f, glm::vec3(10.0f, 0.0f, 0.0f), glm::vec3(10.0f, 0.0f, 0.0f));
+    auto runClip = CreateTestClip("Run", 1.0f, glm::vec3(20.0f, 0.0f, 0.0f), glm::vec3(20.0f, 0.0f, 0.0f));
+
+    AnimationStateMachine sm;
+
+    AnimationState idleState;
+    idleState.Name = "Idle";
+    idleState.Clip = idleClip;
+    sm.AddState(idleState);
+
+    AnimationState walkState;
+    walkState.Name = "Walk";
+    walkState.Clip = walkClip;
+    sm.AddState(walkState);
+
+    AnimationState runState;
+    runState.Name = "Run";
+    runState.Clip = runClip;
+    sm.AddState(runState);
+
+    sm.SetDefaultState("Idle");
+
+    AnimationTransition idleToWalk;
+    idleToWalk.SourceState = "Idle";
+    idleToWalk.DestinationState = "Walk";
+    idleToWalk.BlendDuration = 0.2f;
+    TransitionCondition walkCond;
+    walkCond.ParameterName = "Speed";
+    walkCond.Op = TransitionCondition::Comparison::Equal;
+    walkCond.FloatThreshold = 1.0f;
+    idleToWalk.Conditions.push_back(walkCond);
+    sm.AddTransition(idleToWalk);
+
+    AnimationTransition walkToRun;
+    walkToRun.SourceState = "Walk";
+    walkToRun.DestinationState = "Run";
+    walkToRun.BlendDuration = 0.2f;
+    TransitionCondition runCond;
+    runCond.ParameterName = "Speed";
+    runCond.Op = TransitionCondition::Comparison::Equal;
+    runCond.FloatThreshold = 2.0f;
+    walkToRun.Conditions.push_back(runCond);
+    sm.AddTransition(walkToRun);
+
+    AnimationParameterSet params;
+    params.DefineFloat("Speed", 0.0f);
+    sm.Start(params);
+
+    std::vector<BoneTransform> bones;
+
+    // First transition: Idle -> Walk. Complete it fully.
+    params.SetFloat("Speed", 1.0f);
+    sm.Update(0.01f, params, 1, s_Bone0Ctx, bones);
+    ASSERT_TRUE(sm.IsInTransition());
+    sm.Update(0.3f, params, 1, s_Bone0Ctx, bones);
+    ASSERT_EQ(sm.GetCurrentStateName(), "Walk");
+    ASSERT_FALSE(sm.IsInTransition());
+    EXPECT_NEAR(bones[0].Translation.x, 10.0f, 1e-3f) << "settled Walk pose is wrong before the second transition";
+
+    // Second transition: Walk -> Run, reusing the same scratch buffers the
+    // first transition used.
+    params.SetFloat("Speed", 2.0f);
+    sm.Update(0.01f, params, 1, s_Bone0Ctx, bones);
+    ASSERT_TRUE(sm.IsInTransition());
+
+    // Mid-blend: result must be strictly between Walk (10) and Run (20) — a
+    // stale Idle (0) leaking in from the first transition's scratch would
+    // pull this below 10 or otherwise off the Walk/Run blend line.
+    sm.Update(0.1f, params, 1, s_Bone0Ctx, bones);
+    EXPECT_GT(bones[0].Translation.x, 10.0f) << "second transition's blend was polluted by stale scratch data";
+    EXPECT_LT(bones[0].Translation.x, 20.0f) << "second transition's blend was polluted by stale scratch data";
+
+    sm.Update(0.3f, params, 1, s_Bone0Ctx, bones);
+    EXPECT_EQ(sm.GetCurrentStateName(), "Run");
+    EXPECT_FALSE(sm.IsInTransition());
+    EXPECT_NEAR(bones[0].Translation.x, 20.0f, 1e-3f) << "settled Run pose is wrong after the second transition";
+}
+
+// AnimationGraph::Update reuses one persistent accumulation buffer and one
+// persistent per-layer buffer across ALL layers of a tick, and across every
+// subsequent tick. A two-layer graph with a masked override layer is the
+// scenario where a stale layer-scratch value (rather than a freshly-Start()ed
+// layer stream) would show up as wrong output on the un-masked bone.
+TEST(AnimationStateMachineTest, MultiLayerGraphUpdateReusesScratchAcrossLayersAndTicksWithoutBleed)
+{
+    // Two-bone names: Bone0 (base-only) and Bone1 (overridden by layer 2).
+    // AnimationGraph::Update builds its own PoseEvalContext internally.
+    const std::vector<std::string> boneNames = { "Bone0", "Bone1" };
+
+    auto baseClip = Ref<AnimationClip>::Create();
+    baseClip->Name = "Base";
+    baseClip->Duration = 1.0f;
+    {
+        BoneAnimation b0;
+        b0.BoneName = "Bone0";
+        b0.PositionKeys.push_back({ 0.0, glm::vec3(1.0f, 0.0f, 0.0f) });
+        b0.PositionKeys.push_back({ 1.0, glm::vec3(1.0f, 0.0f, 0.0f) });
+        baseClip->BoneAnimations.push_back(b0);
+
+        BoneAnimation b1;
+        b1.BoneName = "Bone1";
+        b1.PositionKeys.push_back({ 0.0, glm::vec3(2.0f, 0.0f, 0.0f) });
+        b1.PositionKeys.push_back({ 1.0, glm::vec3(2.0f, 0.0f, 0.0f) });
+        baseClip->BoneAnimations.push_back(b1);
+    }
+    baseClip->InitializeBoneCache();
+
+    auto overrideClip = Ref<AnimationClip>::Create();
+    overrideClip->Name = "Override";
+    overrideClip->Duration = 1.0f;
+    {
+        BoneAnimation b1;
+        b1.BoneName = "Bone1";
+        b1.PositionKeys.push_back({ 0.0, glm::vec3(50.0f, 0.0f, 0.0f) });
+        b1.PositionKeys.push_back({ 1.0, glm::vec3(50.0f, 0.0f, 0.0f) });
+        overrideClip->BoneAnimations.push_back(b1);
+    }
+    overrideClip->InitializeBoneCache();
+
+    auto graph = Ref<AnimationGraph>::Create();
+
+    AnimationLayer baseLayer;
+    baseLayer.Name = "Base";
+    baseLayer.Weight = 1.0f;
+    baseLayer.StateMachine = Ref<AnimationStateMachine>::Create();
+    AnimationState baseState;
+    baseState.Name = "Base";
+    baseState.Clip = baseClip;
+    baseLayer.StateMachine->AddState(baseState);
+    baseLayer.StateMachine->SetDefaultState("Base");
+    graph->Layers.push_back(std::move(baseLayer));
+
+    AnimationLayer overrideLayer;
+    overrideLayer.Name = "Override";
+    overrideLayer.Weight = 1.0f;
+    overrideLayer.Mode = AnimationLayer::BlendMode::Override;
+    overrideLayer.AffectedBones = { "Bone1" }; // only overrides Bone1
+    overrideLayer.StateMachine = Ref<AnimationStateMachine>::Create();
+    AnimationState overrideState;
+    overrideState.Name = "Override";
+    overrideState.Clip = overrideClip;
+    overrideLayer.StateMachine->AddState(overrideState);
+    overrideLayer.StateMachine->SetDefaultState("Override");
+    graph->Layers.push_back(std::move(overrideLayer));
+
+    graph->Start();
+
+    std::vector<glm::mat4> finalBones;
+    std::vector<i32> parentIndices = { -1, -1 };
+    std::vector<BoneTransform> bindPose; // empty: identity fallback
+
+    auto ExtractX = [](const glm::mat4& m)
+    { return m[3][0]; };
+
+    // Tick several times; the base layer's Bone0 (untouched by the override
+    // layer) and the override layer's Bone1 must both stay correct on EVERY
+    // tick, proving neither the accumulation nor the per-layer scratch
+    // buffer carries stale data forward.
+    for (int i = 0; i < 5; ++i)
+    {
+        graph->Update(0.016f, 2, finalBones, boneNames, parentIndices, bindPose);
+        ASSERT_EQ(finalBones.size(), 2u);
+        EXPECT_NEAR(ExtractX(finalBones[0]), 1.0f, 1e-3f) << "tick " << i << ": base-only Bone0 corrupted";
+        EXPECT_NEAR(ExtractX(finalBones[1]), 50.0f, 1e-3f) << "tick " << i << ": override-layer Bone1 corrupted";
+    }
+}

docs/agent-rules/per-frame-scratch-reuse.md

@@ -0,0 +1,59 @@
+# Reuse per-frame scratch buffers as persistent instance state, not function locals
+
+Short rule for any per-tick hot path that builds a temporary `std::vector`
+(or similar) inside a function that runs once per entity per frame — or
+worse, once per entity per **sub-step** per frame (issue #445).
+
+## The trap
+
+`AnimationGraph::Update()` evaluated each layer's pose into a fresh
+`std::vector<BoneTransform> layerTransforms;` **declared inside the per-layer
+loop** — reallocating on every layer of every skeleton of every tick.
+`AnimationStateMachine::Update()`'s cross-fade path had the same shape for
+its `currentTransforms`/`targetTransforms` locals, allocating on every ticked
+frame of every transition. Both looked "obviously fine" in isolation (small,
+short-lived, scoped to the call) — the cost only shows up as aggregate heap
+churn across many skinned characters every frame, which unit tests never
+notice because they don't measure allocation counts.
+
+Meanwhile, `Renderer3D::DrawAnimatedMesh` a layer below already wrote bone
+matrices into `FrameDataBuffer` via offset+count instead of copying a
+`std::vector` — the render side had already solved this class of problem.
+The gap was one layer up, in the *animation-compute* code that produces the
+data the renderer then submits correctly.
+
+## The rule
+
+**If a hot-path function is called every tick against a stable, per-instance
+owner (an entity's own `AnimationGraph`, `AnimationStateMachine`, etc.),
+promote its scratch containers to persistent members of that instance and
+`resize()` them in place instead of declaring fresh locals.** This is safe
+exactly when three things hold — check all three, not just the first:
+
+1. **Every callee that fills the buffer always fully overwrites
+   `[0, count)`, never partially.** A `resize()` that only *grows* — never
+   clears — is safe to reuse iff nothing downstream depends on
+   previously-unwritten elements being empty/default. Audit every write path
+   (here: `BlendTree::FillBindPose`, `AnimationStateMachine::Update`,
+   `BlendTree::Evaluate*`, `BlendTree::BlendBoneTransforms` — all resize +
+   fully overwrite). If a path only *conditionally* fills the buffer (e.g. a
+   null target state skips its write), explicitly `.clear()` it first so the
+   reused buffer can't leak the previous call's contents into this one — see
+   `AnimationStateMachine::Update`'s cross-fade `targetTransforms.clear()`.
+2. **The instance is not shared across concurrent callers.** Each entity
+   must own its own instance (here: `Scene.cpp` `Clone()`s a fresh
+   `AnimationGraph` per entity), not a shared template/asset-level object.
+3. **The call site is not running on a `.Parallelizable()` worker path**
+   without its own synchronization. Check `Scene::GetGameplayScheduler()`'s
+   registration for the system that drives the call (here: `AnimationGraph`
+   is not marked `.Parallelizable()`, so it's safe without a lock).
+
+## Guard
+
+`AnimationStateMachineTest.SequentialCrossFadesDoNotLeakScratchBetweenTransitions`
+and `AnimationStateMachineTest.MultiLayerGraphUpdateReusesScratchAcrossLayersAndTicksWithoutBleed`
+(`OloEngine/tests/AnimationStateMachineTest.cpp`) target the actual hazard
+reuse introduces — stale data bleeding across transitions/layers/ticks —
+rather than re-testing pose correctness the pre-existing tests already cover.
+A regression that breaks rule 1 above (a callee stops fully overwriting its
+buffer) would show up as one of these tests reading a stale prior value.