perf(animation): reuse persistent scratch buffers in AnimationGraph/AnimationStateMachine#581
Conversation
…nimationStateMachine (#445) Renderer3D::DrawAnimatedMesh already writes bone matrices into FrameDataBuffer via offset+count, but AnimationGraph::Update and AnimationStateMachine::Update still built fresh std::vector<BoneTransform> 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 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAnimationGraph and AnimationStateMachine now reuse persistent per-instance scratch vectors for bone transforms instead of allocating local vectors each tick. New regression tests verify no stale data leaks across cross-fade transitions and multi-layer graph updates. A companion documentation page and CLAUDE.md entry describe the reuse rule. ChangesPer-frame scratch buffer reuse
Related Issues: Suggested Labels: performance, animation, tests, documentation Suggested Reviewers: drsnuggles8 🥁 A tick, a fade, a vector reused, 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp (1)
64-68: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winSame stale-scratch hazard as flagged in
AnimationGraph.cpp— see that comment for the full analysis and fix.These bare
resize(boneCount)calls (with no element writes) are safe only as long as the output buffer starts empty each call. SinceAnimationGraph::Update()now binds this parameter to the persistentm_ScratchLayer(reused across layers/ticks), hitting any of these branches will silently surface stale data from a previous evaluation instead of a fresh/identity pose.Also applies to: 74-78, 135-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp` around lines 64 - 68, The early-return paths in AnimationStateMachine::Update leave outBoneTransforms only resized and not rewritten, which can leak stale pose data when the buffer is reused. Update the branches in AnimationStateMachine::Update that currently only call resize(boneCount) so they explicitly fill every BoneTransform with a fresh default/identity value before returning. Apply the same fix to the other matching branches noted in this method so the persistent scratch buffer always receives fully initialized output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/OloEngine/Animation/AnimationGraph.cpp`:
- Around line 61-62: The scratch layer is being reused without being fully
reinitialized, so stale bone transforms can survive when the layer is only
resized. Update the `AnimationGraph::Update` path that prepares `m_ScratchLayer`
before `layer.StateMachine->Update(...)` so it always overwrites the full pose,
using `BlendTree::FillBindPose(...)` or an equivalent full initialization
instead of relying on `resize(boneCount)`. Make sure the early-return handling
in `AnimationStateMachine` also leaves `m_ScratchLayer` in a valid bind-pose
state before `ApplyLayerTransforms()` consumes it.
In `@OloEngine/tests/AnimationStateMachineTest.cpp`:
- Around line 400-482: Add a regression test for the missing-state fallback in
AnimationStateMachineTest: after a tick has populated the scratch buffers with a
non-identity pose, remove the currently active state with RemoveState() (or
force a transition target to be removed) and update again. Use
AnimationStateMachine, RemoveState, Start, and Update to verify the fallback
path clears or resets m_ScratchLayer and m_ScratchCurrentTransforms instead of
reusing stale pose data, and assert the resulting BoneTransform output is reset
rather than leaking the previous state.
---
Duplicate comments:
In `@OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp`:
- Around line 64-68: The early-return paths in AnimationStateMachine::Update
leave outBoneTransforms only resized and not rewritten, which can leak stale
pose data when the buffer is reused. Update the branches in
AnimationStateMachine::Update that currently only call resize(boneCount) so they
explicitly fill every BoneTransform with a fresh default/identity value before
returning. Apply the same fix to the other matching branches noted in this
method so the persistent scratch buffer always receives fully initialized
output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9da1f409-ab68-499e-b2fe-6a52a85c69e3
📒 Files selected for processing (7)
CLAUDE.mdOloEngine/src/OloEngine/Animation/AnimationGraph.cppOloEngine/src/OloEngine/Animation/AnimationGraph.hOloEngine/src/OloEngine/Animation/AnimationStateMachine.cppOloEngine/src/OloEngine/Animation/AnimationStateMachine.hOloEngine/tests/AnimationStateMachineTest.cppdocs/agent-rules/per-frame-scratch-reuse.md
| std::vector<BoneTransform>& layerTransforms = m_ScratchLayer; | ||
| layer.StateMachine->Update(dt, Parameters, boneCount, ctx, layerTransforms); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm BoneTransform's default value and Evaluate()'s write pattern.
rg -n -A6 'struct BoneTransform' OloEngine/src
rg -n -A20 'void AnimationState::Evaluate' OloEngine/src/OloEngine/Animation
rg -n -A15 'FillBindPose' OloEngine/src/OloEngine/AnimationRepository: drsnuggles8/OloEngineBase
Length of output: 18991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the state machine update path and the graph blending path in context.
ast-grep outline OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp --view expanded || true
ast-grep outline OloEngine/src/OloEngine/Animation/AnimationGraph.cpp --view expanded || true
printf '\n--- AnimationStateMachine.cpp ---\n'
sed -n '1,260p' OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp
printf '\n--- AnimationGraph.cpp ---\n'
sed -n '1,220p' OloEngine/src/OloEngine/Animation/AnimationGraph.cppRepository: drsnuggles8/OloEngineBase
Length of output: 18711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the state machine implementation and any helper paths that write outputs.
rg -n -A20 -B10 'outBoneTransforms\.resize|FillBindPose|Evaluate\(|Update\(' OloEngine/src/OloEngine/Animation/AnimationStateMachine.cpp OloEngine/src/OloEngine/Animation/AnimationState.cpp OloEngine/src/OloEngine/Animation/BlendTree.cppRepository: drsnuggles8/OloEngineBase
Length of output: 39173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '284,410p' OloEngine/src/OloEngine/Animation/AnimationGraph.cppRepository: drsnuggles8/OloEngineBase
Length of output: 3445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how layer outputs are consumed so the neutral fallback is clear.
sed -n '284,410p' OloEngine/src/OloEngine/Animation/AnimationGraph.cppRepository: drsnuggles8/OloEngineBase
Length of output: 3445
Initialize the scratch pose on the early-return paths in AnimationStateMachine.cpp
m_ScratchLayer is reused, so resize(boneCount) preserves prior layer data whenever the vector is already large enough. A missing/removed state can then feed stale transforms into ApplyLayerTransforms(). Use a full overwrite here (BlendTree::FillBindPose(...) or equivalent) instead of bare resize().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/src/OloEngine/Animation/AnimationGraph.cpp` around lines 61 - 62,
The scratch layer is being reused without being fully reinitialized, so stale
bone transforms can survive when the layer is only resized. Update the
`AnimationGraph::Update` path that prepares `m_ScratchLayer` before
`layer.StateMachine->Update(...)` so it always overwrites the full pose, using
`BlendTree::FillBindPose(...)` or an equivalent full initialization instead of
relying on `resize(boneCount)`. Make sure the early-return handling in
`AnimationStateMachine` also leaves `m_ScratchLayer` in a valid bind-pose state
before `ApplyLayerTransforms()` consumes it.
| 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"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Good coverage for the happy path; add a case for the missing-state fallback.
Both new tests validate reuse correctness only when states/transitions resolve successfully. Given the stale-scratch issue flagged in AnimationStateMachine.cpp (bare resize() on the m_CurrentState.empty() / !currentState fallback paths), consider adding a regression test that calls RemoveState() on the currently-active state mid-playback (or targets a transition to a since-removed state) after a prior tick has populated m_ScratchLayer/m_ScratchCurrentTransforms with non-identity values, then asserts the output is reset (not stale) rather than leaking the prior pose.
Want me to draft this test once the underlying resize()→assign() fix lands?
Also applies to: 489-571
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/AnimationStateMachineTest.cpp` around lines 400 - 482, Add a
regression test for the missing-state fallback in AnimationStateMachineTest:
after a tick has populated the scratch buffers with a non-identity pose, remove
the currently active state with RemoveState() (or force a transition target to
be removed) and update again. Use AnimationStateMachine, RemoveState, Start, and
Update to verify the fallback path clears or resets m_ScratchLayer and
m_ScratchCurrentTransforms instead of reusing stale pose data, and assert the
resulting BoneTransform output is reset rather than leaking the previous state.
1 participant
Summary
Closes #445.
Renderer3D::DrawAnimatedMesh/DrawAnimatedMeshParallel(the issue's headline example) already write bone matrices intoFrameDataBuffervia offset+count (AllocateBoneMatrices/WriteBoneMatrices) — in place since commit789c48f2, predating this issue. No changes needed there.AnimationGraph::Update()built a freshstd::vector<BoneTransform>inside its per-layer loop every tick, andAnimationStateMachine::Update()'s cross-fade path did the same for every ticked frame of a transition. Both are now persistent per-instance scratch members,resize()d in place instead of reallocated.BlendTree::FillBindPose/Evaluate*/BlendBoneTransforms,AnimationStateMachine::Update) fully overwrites[0, boneCount); each entity owns its ownAnimationGraphinstance (Scene.cppClone()s one per entity); theAnimationGraph/AnimationGraphSystemgameplay step is not.Parallelizable().targetTransforms.clear()) for the one case where a reused buffer could otherwise carry over a previous transition's target pose.docs/agent-rules/per-frame-scratch-reuse.md.Test plan
AnimationStateMachineTest.cpptargeting the reuse-specific hazard (stale data bleeding across transitions/layers/ticks):SequentialCrossFadesDoNotLeakScratchBetweenTransitions,MultiLayerGraphUpdateReusesScratchAcrossLayersAndTicksWithoutBleed.OloEngine-Testssuite: 4184 passed, 5 skipped (env-gated), 0 failed.fox.oloscene in the editor — clean frame, no shader/GL errors,DrawAnimatedMeshstill logs correct bone-matrix offsets.🤖 Generated with Claude Code
Summary by CodeRabbit