feat(animation): two-way blend preview + bake-to-clip (Phase 5 slice B) - #361
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Qt/QML singleton AnimationBlender (live two-way A/B blend, modes, weight, bake-to-clip), integrates it into MainWindow's per-frame animation advancement, exposes UI controls in PropertiesPanel.qml (blend subgroup, bake name, mode, weight), updates PropertiesPanelController theme/wiring (controlBgColor, signal forwarding), and adds unit + Ogre integration tests and build wiring. ChangesAnimation Blending + UI Integration
Sequence DiagramsequenceDiagram
participant UI as QML UI
participant Blender as AnimationBlender
participant Controller as AnimationControlController
participant MainWindow as MainWindow::frameRenderingQueued
participant Skeleton as Ogre::Skeleton
rect rgba(100,200,100,0.5)
Note over UI,Blender: Live preview setup
UI->>Blender: setActive(true)
UI->>Blender: setAnimA(nameA), setAnimB(nameB)
UI->>Blender: setWeight(0.5), setMode(Mix)
end
MainWindow->>Blender: apply(entity, dt)
activate Blender
Blender->>Controller: query selected animation / playback speed
Blender->>Skeleton: setBlendMode(...) (if additive)
Blender->>Skeleton: configure stateA/stateB enabled & weight
Blender->>Skeleton: advance state times (via Controller or addTime)
Blender-->>MainWindow: returns true (consumed frame)
deactivate Blender
rect rgba(100,150,200,0.5)
Note over UI,Skeleton: Bake flow
UI->>Blender: bake(clipName, fps)
activate Blender
loop samples 1..N
Blender->>Skeleton: position A/B states for sample
Blender->>Skeleton: _notifyDirty() + update()
Blender->>Skeleton: read bone transforms
Blender->>Blender: write per-bone TRS keyframes to new animation
end
Blender->>Skeleton: restore prior states & blend mode
Blender-->>UI: clipBaked(clipName)
deactivate Blender
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 901d2b2f75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+222
to
+224
| for (const auto& [name, state] : set->getAnimationStates()) { | ||
| if (name != m_animA && name != m_animB) state->setEnabled(false); | ||
| } |
There was a problem hiding this comment.
bake() disables every animation state except A/B before sampling, but only restores A/B afterward, so any other state that was enabled before baking remains permanently turned off. This breaks the “preview isn’t disturbed” behavior and is user-visible whenever the active entity had layered/auxiliary clips enabled (for example, additional additive states), because those states stop contributing immediately after Bake completes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/AnimationBlender.cpp (1)
169-170: ⚡ Quick winAdd a breadcrumb for bake-to-clip.
This is a user-visible operation that mutates the entity's animation set, but it currently leaves no Sentry trail. Please log the action/result here.
As per coding guidelines,
**/*.cpp: "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationBlender.cpp` around lines 169 - 170, The bake operation currently doesn't emit a Sentry breadcrumb; inside AnimationBlender::bake add SentryReporter::addBreadcrumb calls to record the intent and outcome using the prescribed category (use "ai.tool_call" for this tool-like mutation). Specifically, add a breadcrumb at the start of AnimationBlender::bake indicating the clipName and fps, and another breadcrumb after the bake completes (or on error) that includes the resulting clip name/status so callers can trace success/failure; use SentryReporter::addBreadcrumb("ai.tool_call", <descriptive message>) with the clipName, fps and result/status in the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1644-1645: The blend panel is incorrectly hidden because
Visibility is gated by AnimationControlController.hasAnimation (which only means
a clip is selected), so change the visible condition to rely on the actual
animations list instead; update the expression that controls visibility
(currently using AnimationControlController.hasAnimation &&
AnimationBlender.animations.length >= 2) to remove the hasAnimation check and
use AnimationBlender.animations.length >= 2 (or an equivalent
AnimationBlender.hasAnimations boolean) so the panel shows whenever the active
entity actually has two or more animation clips.
In `@src/AnimationBlender_test.cpp`:
- Around line 121-129: The fixture Setup currently hard-fails when Ogre/mesh
fixtures are unavailable using ASSERT_*; change it to detect missing resources
and skip the test instead: replace the ASSERT_NE(app, nullptr) and
ASSERT_TRUE(tryInitOgre()) with runtime checks and call GTEST_SKIP() <<
"Ogre/Xvfb/GL not available" (or similar) to gracefully skip the Ogre-backed
tests; ensure the same pattern is applied to the mesh-related checks around
createStandardOgreMaterials() and the block at lines 164-176 so
AnimationBlender::kill(), AnimationControlController::kill(), Manager::kill(),
tryInitOgre(), createStandardOgreMaterials(), and related setup logic guard and
skip rather than assert.
In `@src/AnimationBlender.cpp`:
- Around line 220-225: The bake path is disabling every animation state but the
restore logic only reenables A and B (m_animA, m_animB), which leaves previously
enabled secondary layers turned off; update both places (the sampling block
around entity->getAllAnimationStates() / getAnimationStates() and the
corresponding restore block at the later location) to first record each state's
original enabled flag (e.g., in a map keyed by name from getAnimationStates())
before mutating, then after baking restore each state's enabled flag from that
saved map rather than only re-enabling m_animA/m_animB so all pre-bake
enabled/disabled states are preserved.
- Around line 151-164: When in ModeOverride the code directly calls
a->addTime(...) / b->addTime(...), which bypasses
AnimationControlController::advanceTime(...) and therefore ignores slice-A
loop-region logic; update the override branch in AnimationBlender (the block
using m_mode == ModeOverride and symbols a, b, useB) to advance the active clip
via the existing AnimationControlController::advanceTime(...) path (or otherwise
invoke the same controller method that enforces loop regions) instead of calling
addTime(...) directly, ensuring the selected slice-A loop-region is honored
during blend preview.
- Around line 47-52: When activating a preview you must capture and later
restore the entity's pre-blend animation state; update
AnimationBlender::setActive(bool) to, when turning on, snapshot each relevant
animation layer's enabled flag, weight and blendMode (store in a m_prevState map
keyed by layer or entity), and when turning off (or when switching target
entity) iterate that snapshot to restore the original enabled/weight/blendMode
values and clear the snapshot. Also update apply() (and the code paths
referenced around the other affected blocks) to only mutate live state after
taking the snapshot, and ensure switching entities triggers restoration of the
previous entity's saved state before modifying the new one. Use unique symbols
like AnimationBlender::setActive, AnimationBlender::apply, m_active, m_entity
and add a m_prevState member to hold the saved per-layer state.
- Around line 183-196: The current bake removes an existing clip after resolving
sa/sb, which can invalidate sa or sb if clipName equals m_animA or m_animB;
before calling skel->removeAnimation(clipStd) check if clipStd matches m_animA
or m_animB and reject/early-return (or force-rename) to prevent baking over a
source clip, or alternatively resolve sa/sb after removal; update the logic
around sa/sb, m_animA, m_animB, clipStd and skel->removeAnimation so you never
remove the animation backing a live AnimationState used by the bake.
---
Nitpick comments:
In `@src/AnimationBlender.cpp`:
- Around line 169-170: The bake operation currently doesn't emit a Sentry
breadcrumb; inside AnimationBlender::bake add SentryReporter::addBreadcrumb
calls to record the intent and outcome using the prescribed category (use
"ai.tool_call" for this tool-like mutation). Specifically, add a breadcrumb at
the start of AnimationBlender::bake indicating the clipName and fps, and another
breadcrumb after the bake completes (or on error) that includes the resulting
clip name/status so callers can trace success/failure; use
SentryReporter::addBreadcrumb("ai.tool_call", <descriptive message>) with the
clipName, fps and result/status in the message.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6c2e57a-fe8f-474d-8a66-370546272df9
📒 Files selected for processing (6)
qml/PropertiesPanel.qmlsrc/AnimationBlender.cppsrc/AnimationBlender.hsrc/AnimationBlender_test.cppsrc/CMakeLists.txtsrc/mainwindow.cpp
Comment on lines
+121
to
+129
| void SetUp() override { | ||
| AnimationBlender::kill(); | ||
| AnimationControlController::kill(); | ||
| Manager::kill(); | ||
| QThread::msleep(20); | ||
| app = qobject_cast<QApplication*>(QCoreApplication::instance()); | ||
| ASSERT_NE(app, nullptr); | ||
| ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; | ||
| createStandardOgreMaterials(); |
There was a problem hiding this comment.
Skip the Ogre-backed fixture instead of hard-failing.
When Ogre or the mesh fixtures are unavailable, this suite currently fails with ASSERT_TRUE(...) / nullptr assertions instead of reporting an intentional skip. That makes non-Linux or partially provisioned test runs noisy even though the pure-data fixture already covers the portable surface.
As per coding guidelines, src/**/*_test.cpp: "Features depending on optional components should be guarded with #ifdef ENABLE_LOCAL_LLM or skip gracefully."
Also applies to: 164-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationBlender_test.cpp` around lines 121 - 129, The fixture Setup
currently hard-fails when Ogre/mesh fixtures are unavailable using ASSERT_*;
change it to detect missing resources and skip the test instead: replace the
ASSERT_NE(app, nullptr) and ASSERT_TRUE(tryInitOgre()) with runtime checks and
call GTEST_SKIP() << "Ogre/Xvfb/GL not available" (or similar) to gracefully
skip the Ogre-backed tests; ensure the same pattern is applied to the
mesh-related checks around createStandardOgreMaterials() and the block at lines
164-176 so AnimationBlender::kill(), AnimationControlController::kill(),
Manager::kill(), tryInitOgre(), createStandardOgreMaterials(), and related setup
logic guard and skip rather than assert.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/AnimationBlender_test.cpp (1)
146-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the Ogre-backed fixture when prerequisites are missing.
This still hard-fails on runners without Ogre/Xvfb/GL or mesh fixtures, even though the pure-data suite already covers the portable surface. Replace these environment assertions with
GTEST_SKIP()so unsupported environments report an intentional skip instead of a red failure.Example adjustment
void SetUp() override { AnimationBlender::kill(); AnimationControlController::kill(); Manager::kill(); QThread::msleep(20); app = qobject_cast<QApplication*>(QCoreApplication::instance()); ASSERT_NE(app, nullptr); - ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; + if (!tryInitOgre()) { + GTEST_SKIP() << "Ogre init failed (Xvfb/GL required in CI)"; + } createStandardOgreMaterials(); } TEST_F(AnimationBlenderTest, RefreshExposesEntityAnimations) { - ASSERT_TRUE(canLoadMeshFiles()); + if (!canLoadMeshFiles()) { + GTEST_SKIP() << "Animated mesh fixtures unavailable"; + } Ogre::Entity* entity = setupBlendEntity("ABT_RefreshTest"); ASSERT_NE(entity, nullptr);As per coding guidelines,
src/**/*_test.cpp: "Features depending on optional components should be guarded with#ifdefENABLE_LOCAL_LLM or skip gracefully."Also applies to: 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationBlender_test.cpp` around lines 146 - 155, The fixture Setup currently hard-fails when Ogre/Xvfb/GL or the Qt app is missing; change it to skip the test instead: in SetUp(), after calling AnimationBlender::kill(), AnimationControlController::kill(), Manager::kill() and QThread::msleep(20), replace the ASSERT_NE(app, nullptr) check and the ASSERT_TRUE(tryInitOgre()) assertion with conditional checks that call GTEST_SKIP() with a short message if app is null or tryInitOgre() returns false, and similarly guard/createStandardOgreMaterials() so missing prerequisites result in GTEST_SKIP() rather than a test failure; apply the same replacement for the other occurrence around the create/initialization checks (the block referenced at lines ~207-210).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationBlender.cpp`:
- Around line 54-77: Prevent turning off or double-updating animation states
when one or both blend sides are invalid: in AnimationBlender::setActive(bool
on) check that both m_animA and m_animB are non-empty/valid and not equal before
setting m_active true, and refuse activation (leave existing states untouched)
if that guard fails; similarly add the same guard at the start of apply() and
the bake routine used for precomputing blends so they return early when m_animA
or m_animB is unset or when m_animA == m_animB. Also ensure
resolveActiveEntity() and captureSnapshot(entity) are only invoked after the
guard so you don't disable every animation state when activation should be
rejected.
---
Duplicate comments:
In `@src/AnimationBlender_test.cpp`:
- Around line 146-155: The fixture Setup currently hard-fails when Ogre/Xvfb/GL
or the Qt app is missing; change it to skip the test instead: in SetUp(), after
calling AnimationBlender::kill(), AnimationControlController::kill(),
Manager::kill() and QThread::msleep(20), replace the ASSERT_NE(app, nullptr)
check and the ASSERT_TRUE(tryInitOgre()) assertion with conditional checks that
call GTEST_SKIP() with a short message if app is null or tryInitOgre() returns
false, and similarly guard/createStandardOgreMaterials() so missing
prerequisites result in GTEST_SKIP() rather than a test failure; apply the same
replacement for the other occurrence around the create/initialization checks
(the block referenced at lines ~207-210).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a7a882d-f74c-4266-960e-85bd129dec13
📒 Files selected for processing (7)
qml/PropertiesPanel.qmlsrc/AnimationBlender.cppsrc/AnimationBlender.hsrc/AnimationBlender_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/PropertiesPanelController_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AnimationBlender.h
7 tasks
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/AnimationBlender_test.cpp (1)
178-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip Ogre-backed tests when Ogre or mesh fixtures are unavailable.
This suite still turns expected environment gaps into failures: the fixture asserts on Ogre init, and
setupBlendEntity()just returnsnullptrwhen the assets are missing. Converting those paths into runtime skips would keep the portable property tests green while making the Ogre-backed coverage opt out cleanly on runners without Xvfb/GL or fixture assets.As per coding guidelines,
src/**/*_test.cpp: "Features depending on optional components should be guarded with#ifdef ENABLE_LOCAL_LLM or skip gracefully." Also: "Linux: Tests must work under Xvfb (headless X11) — avoid assumptions about a real display."Also applies to: 221-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationBlender_test.cpp` around lines 178 - 186, The test fixture should skip (not fail) when Ogre or mesh assets are unavailable: replace the hard ASSERT_NE/ASSERT_TRUE in SetUp() that force failures with runtime skips (use GTEST_SKIP() or equivalent) when qobject_cast<QApplication*> returns nullptr or tryInitOgre() is false, and change callers of setupBlendEntity() (e.g. in tests referencing setupBlendEntity() around lines 221-223) to detect a nullptr return and call GTEST_SKIP() instead of proceeding; update SetUp(), the tryInitOgre() check, and any tests using setupBlendEntity() so missing display/fixtures opt out gracefully rather than asserting.
🧹 Nitpick comments (1)
src/AnimationBlender_test.cpp (1)
251-274: ⚡ Quick winAdd an endpoint assertion for the baked clip.
These tests validate sample count and mid-clip poses, but they never check the last keyframe. That leaves the
t == lengthsampling path untested, so a bake that wraps the final frame back to the start pose will still pass. A small assertion on the last keyframe for theweight=0/weight=1cases would close that gap.Also applies to: 276-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationBlender_test.cpp` around lines 251 - 274, Add an assertion that verifies the last keyframe at t == length matches the expected endpoint pose to cover the sampling-at-length path: locate the test BakeProducesNewClipWithExpectedLength (and the similar tests around lines 276-328) after obtaining baked and track (getNodeTrack(1)), fetch the last keyframe (index track->getNumKeyFrames()-1) and assert its transform/pose equals the expected pose for the boundary cases (run once for weight=0 and once for weight=1 or compare against known endpoint pose), ensuring the bake does not wrap the final frame back to the start; update the relevant EXPECT/ASSERT to check the last keyframe values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationBlender.cpp`:
- Around line 80-110: The setActive method should record Sentry breadcrumbs for
preview activation and deactivation: call SentryReporter::addBreadcrumb with an
appropriate category (e.g., "preview") and a short message when turning the
preview on and when turning it off; include contextual details such as m_animA,
m_animB, the target entity identifier (from resolveActiveEntity() if non-null),
and the new state (on/off) so breadcrumbs show what clips and entity were
affected; place the "activation" breadcrumb just before
captureSnapshot/disableAllStates and the "deactivation" breadcrumb just before
restoreSnapshot so transitions that mutate animation flags, weights, and
snapshot state are recorded.
- Around line 367-373: positionForSample currently wraps timestamps with
std::fmod so when the bake t equals the overall baked clip length you sample
equal-length sources at 0 instead of their end pose; modify
positionForSample(Ogre::AnimationState* sa, Ogre::AnimationState* sb, float t,
int mode, double weight) to accept the bake's total length (e.g. add a float
bakeLength parameter) and compute ta/tb as: if the source length <= 0 -> 0.0f;
else if t == bakeLength -> source.getLength() else -> std::fmod(t,
source.getLength()); update both call sites (including the other occurrence
around lines 490-492) to pass the bake loop's length so the final baked sample
preserves end poses instead of wrapping to 0.0.
- Around line 112-118: When a live preview is active, changing animation setters
(e.g. setAnimA / setAnimB where m_animA or m_animB are updated) can make the
current blend invalid (animA == animB or side cleared) so apply() will return
false on the next frame but the entity remains in the last blended state; fix
this by detecting that transition inside those setters: after updating m_animA /
m_animB, if the preview is active (check the preview/active flag used by
apply()) and the new state would make apply() return false (e.g. m_animA.empty()
|| m_animB.empty() || m_animA==m_animB), immediately restore the saved snapshot
(call the existing restoreSnapshot() or equivalent) and disable the live preview
(toggle the same flag that apply()/MainWindow::frameRenderingQueued() checks),
then emit selectionChanged() as before so the UI and frame loop are consistent.
---
Duplicate comments:
In `@src/AnimationBlender_test.cpp`:
- Around line 178-186: The test fixture should skip (not fail) when Ogre or mesh
assets are unavailable: replace the hard ASSERT_NE/ASSERT_TRUE in SetUp() that
force failures with runtime skips (use GTEST_SKIP() or equivalent) when
qobject_cast<QApplication*> returns nullptr or tryInitOgre() is false, and
change callers of setupBlendEntity() (e.g. in tests referencing
setupBlendEntity() around lines 221-223) to detect a nullptr return and call
GTEST_SKIP() instead of proceeding; update SetUp(), the tryInitOgre() check, and
any tests using setupBlendEntity() so missing display/fixtures opt out
gracefully rather than asserting.
---
Nitpick comments:
In `@src/AnimationBlender_test.cpp`:
- Around line 251-274: Add an assertion that verifies the last keyframe at t ==
length matches the expected endpoint pose to cover the sampling-at-length path:
locate the test BakeProducesNewClipWithExpectedLength (and the similar tests
around lines 276-328) after obtaining baked and track (getNodeTrack(1)), fetch
the last keyframe (index track->getNumKeyFrames()-1) and assert its
transform/pose equals the expected pose for the boundary cases (run once for
weight=0 and once for weight=1 or compare against known endpoint pose), ensuring
the bake does not wrap the final frame back to the start; update the relevant
EXPECT/ASSERT to check the last keyframe values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22297ecf-f2c6-4b02-861f-53f2559e86c9
📒 Files selected for processing (3)
CMakeLists.txtsrc/AnimationBlender.cppsrc/AnimationBlender_test.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
Adds the slice B work from #260 / #360. - New AnimationBlender singleton (QML-registered as AnimationControl.AnimationBlender). Holds animA/animB names, weight (0..1), and mode (Mix / Additive / Override). Tracks the entity currently selected in the Animation Control panel. - Mix: weights (1-w, w), both states enabled, ANIMBLEND_AVERAGE. - Additive: same weights, skeleton blend mode ANIMBLEND_CUMULATIVE. - Override: single state enabled (B if w >= 0.5, else A). - MainWindow::frameRenderingQueued routes the active entity through blender->apply(); inactive entities follow the slice-A path (per-state speed scaling + selected-clip loop wrap). - bake() samples the blended pose at 30 fps (configurable), captures each bone's local TRS via Skeleton::_updateTransforms(), and writes a new Ogre::Animation with one node track per bone. Live state is saved + restored so the preview isn't disturbed by the bake. An existing clip with the same name is replaced. QML - New "Blend" section in PropertiesPanel.qml's Animations group: active checkbox, two animation pickers, weight slider, mode combo, bake-name field, Bake button. Visible only when the active entity has at least two animations. Tests - Pure-data fixture (10 cases, no Ogre): defaults, weight clamp, mode validation, signal emission, no-op safety. - Ogre fixture (Linux CI): refresh exposes both clips, bake produces the expected length + keyframe count, weight=0 ⇒ pure A, weight=1 ⇒ pure B, repeat-bake replaces the existing clip. Issue: #360 Plan: #260 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rces
The MaterialEditorQML_{test,qml_test,perf_test} executables maintain
their own duplicated source list in tests/CMakeLists.txt. Slice B
added AnimationBlender to src/CMakeLists.txt but not here, which
caused undefined-reference link errors on the QML test targets in CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit (Critical): - Refuse to bake when clipName matches animA or animB. The state pointers sa/sb resolve before removeAnimation() runs, so reusing the source name would invalidate them mid-bake. Codex P1 + CodeRabbit Major (preview state restore): - Snapshot every animation state's enabled+weight (and skeleton blend mode) when the blender activates and restore on deactivate / entity switch. Before, only A and B were touched, so any auxiliary layers enabled on the entity stayed off after toggling Active or Bake. CodeRabbit Major (slice-A loop region): - apply() now routes the active clip's time advance through AnimationControlController::advanceTime(), so the slice-A loop region still wraps the selected animation while blend preview is on. Non-active clip uses speed-scaled dt directly. - mainwindow.cpp passes raw dt to apply() (advanceTime applies speed itself); the lambda inside apply() recomputes scaledDt for non-A/B. CodeRabbit Major (QML visibility): - Drop AnimationControlController.hasAnimation from the blend panel's visible binding — that property is a "is a clip selected for KF edit", not "does the active entity have animations". Now gated only on AnimationBlender.animations.length >= 2. CodeRabbit Major (bake drops layers): - bake() now snapshots+restores every state in the set (not just A/B) so the live preview is fully preserved across a bake. SonarCloud cleanup: - Cast fps to float for the sample-count math (S5276). - Extract positionForSample() and writeAllBoneKeyframes() helpers to bring bake()'s cognitive complexity below the threshold (S3776). - Mark singleton new/delete with NOSONAR — pattern is shared across the project's controllers and changing it would be a separate refactor. Did NOT address (intentional): - CodeRabbit's GTEST_SKIP suggestion in AnimationBlender_test.cpp: the project's own convention (PR #355) is ASSERT_TRUE(tryInitOgre) to fail fast in CI. Switching to skip would mask CI regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract findEntityByName, configureBlend, muteOtherLayers, advanceState, captureAllStates, restoreAllStates, disableNonAB, createBoneTracks helpers in AnimationBlender.cpp. apply() drops from CC=31 → ~15, bake() from CC=35 → ~12 (S3776). - Extract advanceEntityStates() in mainwindow.cpp; restructure frameRenderingQueued with an early return so the inner loop is ≤ 3 levels deep (S134). - Const-correct refreshFromSelection's entity pointer (S5350) and use init-in-if for activeEntity (S6004). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UX - Blend section is now a collapsible subgroup in the Animations panel, starts collapsed; header shows "(active)" hint when on. - Bake now deactivates the blender so the per-frame apply() stops re-imposing weights on top of the restored pre-bake state. - After bake, both AnimationControlController (Animation Control panel) and PropertiesPanelController (Inspector) refresh so the new clip appears in their lists without needing a re-select. - Activating the blender disables every per-animation Enable flag on the active entity (deactivation restores them via the snapshot). Inspector's per-anim Enable/Loop checkboxes show as 40 % opacity and ignore clicks while the blender is active for that entity, so the panel and the blender no longer fight over setEnabled() each frame. - New "Active" toggle in the Blend group uses the same 14×14 Rectangle + ✓ pattern as the per-anim Enable/Loop boxes (was a stock CheckBox). - New PropertiesPanelController.controlBgColor — a lightened Button shade — used as the unchecked background for all custom checkboxes (was "transparent", which disappeared on dark mode). Sentry - bake() emits a "ui.action" breadcrumb with clip name, mode, weight, fps, length, and sample count (per CLAUDE.md guidance). Tests - AnimationBlender_test pure-data: refuses bake on empty A/B, refuses bake over source clip names without an entity, default activeEntityName. - AnimationBlender_test Ogre fixture: bake refuses to overwrite source clip; activate disables every state; deactivate restores enabled flags via snapshot; bake auto-deactivates the blender; clipBaked signal emits with the new clip name; activeEntityName tracks the controller's selected entity. - PropertiesPanelController_test: controlBgColor matches button.lighter(115) and differs from panelColor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit Major:
- setActive() and apply()/bake() now refuse to operate when animA or
animB is empty, or when they're equal. Previously, hitting Active
with no clips picked would disable every state on the entity but
apply() would bail out — leaving the rig frozen. Same problem for
A == B (mix/additive would advance the same state twice per frame).
SonarCloud:
- S134 (critical): extract disableAllStates() helper from setActive()
so the inner loop is no longer 4 levels deep.
- S3358 (major): replace nested ternary in the bake breadcrumb with
a small modeName() switch helper.
- S5817 (major): apply() mutates skeleton+state pointers indirectly,
so it can't be const. Mark with NOSONAR + rationale.
Tests:
- ActivateRefusedWhen{AnimAEmpty, AnimBEmpty, AEqualsB}: setActive(true)
is rejected and active() stays false.
- BakeRefusedWhenAEqualsB: bake returns empty when both clips match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Slice B (animation blend preview + bake-to-clip) is a feature addition since 2.33.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new A/B guard in setActive() rejects activation when animA or animB is empty. The pre-existing ActiveTogglesEmitSignal test didn't set them, so setActive(true) was a silent no-op and the signal never fired. Set A/B in the test before toggling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n, breadcrumbs) CodeRabbit Major: - positionForSample now clamps to clip length when t == clipLen instead of fmod-wrapping to 0. Previously, the closing keyframe of an equal-length bake captured the start pose, producing a visible pop on weight=0/1 bakes. fmod still applies for the bake-length > clip-length looping case. CodeRabbit Major: - New deactivateIfInvalid() helper called from setAnimA/setAnimB. If the user clears one side or makes A == B while preview is active, the blender now restores the snapshot and flips off — previously it was left with a stale enabled/weight configuration that kept playing until the user manually toggled Active off. CodeRabbit Minor: - Sentry breadcrumbs for blend preview activate/deactivate (matches CLAUDE.md guidance and the slice-B precedent for bake). Tests: - ClearingAnimAWhileActiveDeactivates / MakingAEqualBWhileActiveDeactivates. Rebased onto master (#359 / 0351755). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/phase5-slice-b-anim-blend
branch
from
May 2, 2026 15:14
5975183 to
4835eaa
Compare
Sonar counts Q_PROPERTY getters/setters/signals + QML singleton boilerplate (instance/qmlInstance/kill) as separate methods, putting the class at 36 vs the 35 threshold. The class is cohesive — live preview and bake share the same selection/snapshot state — so splitting it would just fragment the wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/AnimationBlender_test.cpp (1)
200-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the Ogre-backed fixture when the environment isn't provisioned.
These checks still hard-fail when GL/Xvfb or mesh fixtures are missing. That makes the suite noisy on non-provisioned lanes even though this is an environment precondition, not a product regression.
💡 Suggested pattern
void SetUp() override { AnimationBlender::kill(); AnimationControlController::kill(); Manager::kill(); QThread::msleep(20); app = qobject_cast<QApplication*>(QCoreApplication::instance()); - ASSERT_NE(app, nullptr); - ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; + if (!app) GTEST_SKIP() << "QApplication not initialized"; + if (!tryInitOgre()) GTEST_SKIP() << "Ogre/Xvfb/GL not available"; + if (!canLoadMeshFiles()) GTEST_SKIP() << "Mesh fixtures not available"; createStandardOgreMaterials(); } @@ Ogre::Entity* setupBlendEntity(const std::string& name) { - if (!canLoadMeshFiles()) return nullptr; Ogre::Entity* entity = createAnimatedTestEntity(name); if (!entity) return nullptr;As per coding guidelines,
src/**/*_test.cpp: "Features depending on optional components should be guarded with#ifdefENABLE_LOCAL_LLM or skip gracefully."Also applies to: 243-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationBlender_test.cpp` around lines 200 - 208, The SetUp method for the Ogre-backed fixture currently hard-fails when GL/Xvfb or mesh fixtures are missing; change it to skip gracefully by detecting the environment and invoking the test-skip path instead of asserting: inside the SetUp override (the method that calls AnimationBlender::kill(), AnimationControlController::kill(), Manager::kill(), and then calls tryInitOgre() and createStandardOgreMaterials()), replace the ASSERT_TRUE(tryInitOgre()) with a conditional that if tryInitOgre() returns false calls GTEST_SKIP() (or the test framework’s skip mechanism) with a clear message and returns early so createStandardOgreMaterials() is not called; apply the same guard/skip logic to the other similar checks around the createStandardOgreMaterials() usage (the code noted at lines 243-245) so tests are skipped instead of failing on non-provisioned CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationBlender.cpp`:
- Around line 347-353: In ModeOverride the code only advances the currently
dominant clip, causing the other clip's timeline to stall; change the
ModeOverride branch so it advances both clip timelines unconditionally by
calling advanceState(a, m_animA, activeAnim, ctrl, dt, scaledDt) and
advanceState(b, m_animB, activeAnim, ctrl, dt, scaledDt) (i.e., mirror the
non-override branch) so both m_animA and m_animB progress and avoid stale-time
jumps when weight crosses 0.5.
---
Duplicate comments:
In `@src/AnimationBlender_test.cpp`:
- Around line 200-208: The SetUp method for the Ogre-backed fixture currently
hard-fails when GL/Xvfb or mesh fixtures are missing; change it to skip
gracefully by detecting the environment and invoking the test-skip path instead
of asserting: inside the SetUp override (the method that calls
AnimationBlender::kill(), AnimationControlController::kill(), Manager::kill(),
and then calls tryInitOgre() and createStandardOgreMaterials()), replace the
ASSERT_TRUE(tryInitOgre()) with a conditional that if tryInitOgre() returns
false calls GTEST_SKIP() (or the test framework’s skip mechanism) with a clear
message and returns early so createStandardOgreMaterials() is not called; apply
the same guard/skip logic to the other similar checks around the
createStandardOgreMaterials() usage (the code noted at lines 243-245) so tests
are skipped instead of failing on non-provisioned CI.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa17bf44-779a-4509-a78b-73a9b6784498
📒 Files selected for processing (11)
CMakeLists.txtqml/PropertiesPanel.qmlsrc/AnimationBlender.cppsrc/AnimationBlender.hsrc/AnimationBlender_test.cppsrc/CMakeLists.txtsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/PropertiesPanelController_test.cppsrc/mainwindow.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (4)
- CMakeLists.txt
- src/CMakeLists.txt
- tests/CMakeLists.txt
- src/PropertiesPanelController_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- qml/PropertiesPanel.qml
- src/mainwindow.cpp
- src/AnimationBlender.h
Comment on lines
+347
to
+353
| if (m_mode == ModeOverride) { | ||
| const bool useB = (m_weight >= 0.5); | ||
| advanceState(useB ? b : a, useB ? m_animB : m_animA, | ||
| activeAnim, ctrl, dt, scaledDt); | ||
| } else { | ||
| advanceState(a, m_animA, activeAnim, ctrl, dt, scaledDt); | ||
| advanceState(b, m_animB, activeAnim, ctrl, dt, scaledDt); |
There was a problem hiding this comment.
Keep both clip timelines advancing in override mode.
Right now only the dominant state moves. If the user drags the weight across 0.5 after preview has been running, the other clip resumes from stale time, so override preview jumps and can disagree with what bake() samples at that same moment.
💡 Minimal fix
- if (m_mode == ModeOverride) {
- const bool useB = (m_weight >= 0.5);
- advanceState(useB ? b : a, useB ? m_animB : m_animA,
- activeAnim, ctrl, dt, scaledDt);
- } else {
- advanceState(a, m_animA, activeAnim, ctrl, dt, scaledDt);
- advanceState(b, m_animB, activeAnim, ctrl, dt, scaledDt);
- }
+ advanceState(a, m_animA, activeAnim, ctrl, dt, scaledDt);
+ advanceState(b, m_animB, activeAnim, ctrl, dt, scaledDt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationBlender.cpp` around lines 347 - 353, In ModeOverride the code
only advances the currently dominant clip, causing the other clip's timeline to
stall; change the ModeOverride branch so it advances both clip timelines
unconditionally by calling advanceState(a, m_animA, activeAnim, ctrl, dt,
scaledDt) and advanceState(b, m_animB, activeAnim, ctrl, dt, scaledDt) (i.e.,
mirror the non-override branch) so both m_animA and m_animB progress and avoid
stale-time jumps when weight crosses 0.5.
|
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #360 (slice B of #260).
AnimationControl.AnimationBlender). Holds two animation names, a 0..1 weight, and a Mix / Additive / Override mode. Tracks whatever entity is selected in the Animation Control panel.MainWindow::frameRenderingQueuedroutes the active entity throughblender->apply(). Inactive entities still follow the slice-A code path (speed-scaledaddTime+ selected-clip loop wrap).setWeight(1-w, w), both states enabled,ANIMBLEND_AVERAGE.ANIMBLEND_CUMULATIVE.w >= 0.5, else A).Skeleton::_updateTransforms()per sample, writes one node track per bone with the captured local TRS. Live state is saved + restored so the preview isn't disturbed. Existing clip with the same name is replaced.Test plan
AnimationBlenderPropertyTest.*(10 cases) +AnimationBlenderTest.*(5 cases)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Tests
Chores