From 5021665b6e555cea9c365672af879217cd0e8c27 Mon Sep 17 00:00:00 2001 From: Ole Bueker Date: Tue, 7 Jul 2026 08:14:33 +0200 Subject: [PATCH] perf(animation): reuse persistent scratch buffers in AnimationGraph/AnimationStateMachine (#445) Renderer3D::DrawAnimatedMesh already writes bone matrices into FrameDataBuffer via offset+count, but AnimationGraph::Update and AnimationStateMachine::Update still built fresh std::vector scratch (per-layer, per cross-fade tick) as function locals every frame. Promote them to persistent per-instance members, resized in place instead of reallocated, since every downstream writer fully overwrites its buffer and each entity owns its own graph instance. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 1 + .../OloEngine/Animation/AnimationGraph.cpp | 10 +- .../src/OloEngine/Animation/AnimationGraph.h | 12 ++ .../Animation/AnimationStateMachine.cpp | 9 +- .../Animation/AnimationStateMachine.h | 7 + OloEngine/tests/AnimationStateMachineTest.cpp | 192 ++++++++++++++++++ docs/agent-rules/per-frame-scratch-reuse.md | 59 ++++++ 7 files changed, 285 insertions(+), 5 deletions(-) create mode 100644 docs/agent-rules/per-frame-scratch-reuse.md diff --git a/CLAUDE.md b/CLAUDE.md index a71145295..2386f9692 100644 --- a/CLAUDE.md +++ b/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. diff --git a/OloEngine/src/OloEngine/Animation/AnimationGraph.cpp b/OloEngine/src/OloEngine/Animation/AnimationGraph.cpp index 18dd29c23..2fe0c0e0c 100644 --- a/OloEngine/src/OloEngine/Animation/AnimationGraph.cpp +++ b/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 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& accumulatedTransforms = m_ScratchAccumulated; BlendTree::FillBindPose(ctx, boneCount, accumulatedTransforms); // Evaluate each layer bottom-to-top @@ -52,7 +58,7 @@ namespace OloEngine continue; } - std::vector layerTransforms; + std::vector& layerTransforms = m_ScratchLayer; layer.StateMachine->Update(dt, Parameters, boneCount, ctx, layerTransforms); if (layerTransforms.size() < boneCount) diff --git a/OloEngine/src/OloEngine/Animation/AnimationGraph.h b/OloEngine/src/OloEngine/Animation/AnimationGraph.h index d46c2160d..63ea6a09a 100644 --- a/OloEngine/src/OloEngine/Animation/AnimationGraph.h +++ b/OloEngine/src/OloEngine/Animation/AnimationGraph.h @@ -70,5 +70,17 @@ namespace OloEngine 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 m_ScratchAccumulated; + std::vector m_ScratchLayer; }; } // namespace OloEngine diff --git a/OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp b/OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp index 02cfd03f0..8bc3c44f1 100644 --- a/OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp +++ b/OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp @@ -142,9 +142,12 @@ namespace OloEngine return; } - // Cross-fade between current and target states - std::vector currentTransforms; - std::vector 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& currentTransforms = m_ScratchCurrentTransforms; + std::vector& targetTransforms = m_ScratchTargetTransforms; + targetTransforms.clear(); { f32 normalizedTime = 0.0f; diff --git a/OloEngine/src/OloEngine/Animation/AnimationStateMachine.h b/OloEngine/src/OloEngine/Animation/AnimationStateMachine.h index 56dc55270..8d7f42333 100644 --- a/OloEngine/src/OloEngine/Animation/AnimationStateMachine.h +++ b/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 m_ScratchCurrentTransforms; + std::vector m_ScratchTargetTransforms; }; } // namespace OloEngine diff --git a/OloEngine/tests/AnimationStateMachineTest.cpp b/OloEngine/tests/AnimationStateMachineTest.cpp index 847608bac..7e1e3db8d 100644 --- a/OloEngine/tests/AnimationStateMachineTest.cpp +++ b/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 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 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 boneNames = { "Bone0", "Bone1" }; + + auto baseClip = Ref::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::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::Create(); + + AnimationLayer baseLayer; + baseLayer.Name = "Base"; + baseLayer.Weight = 1.0f; + baseLayer.StateMachine = Ref::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::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 finalBones; + std::vector parentIndices = { -1, -1 }; + std::vector 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"; + } +} diff --git a/docs/agent-rules/per-frame-scratch-reuse.md b/docs/agent-rules/per-frame-scratch-reuse.md new file mode 100644 index 000000000..54c1c6dab --- /dev/null +++ b/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 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.