feat(anim): tolerance-based redundant-keyframe simplifier - #310
Merged
Conversation
Adds a new animation operation that walks each track and removes any
keyframe whose value matches the lerp/slerp of its neighbors within
tolerance. First/last keys and sharp pose changes are preserved, so
Mixamo-style baked clips shed 40-60% of keys without visible drift.
Surfaces:
- AnimationMerger::simplifyAnimation + analyzeRedundantKeyframes
(tolerance-based key removal, antipodal-aware quaternion slerp).
- CLI: qtmesh anim --simplify | --analyze, with --preset
(conservative/balanced/aggressive) plus per-axis --tolerance and
--rotation-tolerance-deg overrides.
- Scan rule redundant_keyframes_pct: warns when the projected savings
exceed a threshold, reports % redundant keys + projected file size
("Simplify it to save ~X. Original size: Y, projected size: Z").
- MCP tools simplify_animation + analyze_animation (preset + per-axis
overrides, optional entity_name / animation_name).
- Inspector: per-animation Simplify button (scissors glyph) with a
per-entity tolerance preset selector and a tooltip showing how many
keys would be removed under the current preset.
Defaults are now the "balanced" preset (~1mm translation, 0.5deg
rotation) which is visually indistinguishable on meter-scale rigs.
The CLI/MCP/UI all share the same preset definitions.
Other changes:
- ThemedComboBox switched to ThemeManager so it works in any panel
(was MaterialEditorQML-only); QML loading test fixture now
registers ThemeManager so the existing Material editor QML tests
keep passing.
- ThemedComboBox aliased into the PropertiesPanel resource prefix so
the inspector can use it without cross-module imports.
- Bumped to 2.30.0.
Tests: AnimationMerger redundant-keyframe cases (linear collapse,
non-linear apex preservation, tolerance sensitivity, endpoint
preservation, null/missing inputs); ScanConfig parses the new rule;
ScanEngine evaluates the rule (default off, fires on Mixamo fixture,
respects high threshold); MCP tools/list exposes the new tools with
the expected schema.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis pull request introduces animation keyframe simplification functionality, bumping the version from 2.29.0 to 2.30.0. It adds tolerance-based redundant keyframe detection in AnimationMerger, integrates simplification into the CLI pipeline and MCP server as new tools, updates the QML UI with simplification controls, extends PropertiesPanelController with analysis/simplification methods, and adds redundant keyframe scanning to the ScanEngine with configurable tolerance thresholds. Changes
Sequence DiagramsequenceDiagram
participant UI as PropertiesPanel (QML)
participant Controller as PropertiesPanelController
participant Merger as AnimationMerger
participant State as Animation State
rect rgba(220, 100, 100, 0.5)
Note over UI,State: Simplification Workflow
end
UI->>Controller: simplifyAnimation(entityName, animName, preset)
Controller->>Controller: Stop playback, disable overlays
Controller->>Merger: analyzeRedundantKeyframes(anim, tolerances)
Merger->>Merger: Count original & redundant keys
Merger-->>Controller: {redundant, original, percentage}
Controller->>Merger: simplifyAnimation(skeleton, animName, tolerances)
Merger->>Merger: Align quaternions, interpolate, compare
Merger->>Merger: Remove redundant middle keys
Merger->>Merger: Recreate animation tracks
Merger-->>Controller: Removed key count
Controller->>State: Refresh animation states
Controller->>UI: animationStateChanged signal
UI->>UI: Update tooltip, modal summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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. 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: 78f32502ed
ℹ️ 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
+236
to
+240
| total += static_cast<int>(ch->mNumPositionKeys); | ||
| total += static_cast<int>(ch->mNumRotationKeys); | ||
| total += static_cast<int>(ch->mNumScalingKeys); | ||
| redundant += countRedundantVecKeys(ch->mPositionKeys, ch->mNumPositionKeys, tol.translation); | ||
| redundant += countRedundantQuatKeys(ch->mRotationKeys, ch->mNumRotationKeys, tol.rotationDeg); |
There was a problem hiding this comment.
The scanner currently sums and simplifies position, rotation, and scale key arrays independently, but AnimationMerger::simplifyAnimation only removes a keyframe when all transform components are within tolerance at the same timestamp. This means clips with static translation/scale but non-redundant rotation can be reported as highly redundant here, triggering false redundant_keyframes_pct warnings and inflated projected savings (especially when scan gating fails on warnings).
Useful? React with 👍 / 👎.
Comment on lines
+649
to
+650
| const_cast<AssetInfo&>(asset).totalKeyframes = total; | ||
| const_cast<AssetInfo&>(asset).redundantKeyframes = redundant; |
There was a problem hiding this comment.
evaluateRules takes const AssetInfo& but writes through it via const_cast, which is undefined behavior when the caller passes a genuinely const object (e.g., API consumers or tests storing const AssetInfo). This can lead to optimizer-dependent behavior and breaks the function’s const contract; the stats should be returned separately or the parameter should be non-const.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
1149-1151:⚠️ Potential issue | 🟡 MinorBreadcrumb mislabels
simplify/analyzeasmerge.
animOponly handles list/rename/resample/decimate, so thecli.animbreadcrumb here reports"Anim merge ..."for the new--simplifyand--analyzemodes. A more specific breadcrumb is added later inside the simplify/analyze branch, but this top-level one still tags the operation incorrectly in Sentry.🔧 Suggested fix
- QString animOp = listMode ? "list" : (renameMode ? "rename" : (resampleMode ? "resample" : (decimateMode ? "decimate" : "merge"))); + QString animOp = listMode ? "list" + : renameMode ? "rename" + : resampleMode ? "resample" + : decimateMode ? "decimate" + : simplifyMode ? "simplify" + : analyzeMode ? "analyze" + : "merge";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 1149 - 1151, The breadcrumb mislabels simplify/analyze as merge because animOp only checks listMode/renameMode/resampleMode/decimateMode; update the animOp computation used with SentryReporter::addBreadcrumb to include simplifyMode and analyzeMode (e.g., check simplifyMode and analyzeMode in the ternary chain before falling back to mergeMode) so animOp correctly becomes "simplify" or "analyze" when those flags are set, keeping the existing mergeMode branch and the mergeFiles suffix logic unchanged.
🧹 Nitpick comments (8)
src/MCPServer_test.cpp (1)
3988-4020: Extend schema assertions toanalyze_animationas well.Right now the test only validates schema fields for
simplify_animation. Ifanalyze_animationkeeps its name but loses required args, this test won’t catch it.Suggested patch
- for (const auto& v : tools) { - const QJsonObject t = v.toObject(); - if (t.value("name").toString() != "simplify_animation") continue; - const QJsonObject schema = t.value("inputSchema").toObject(); - const QJsonObject props = schema.value("properties").toObject(); - EXPECT_TRUE(props.contains("preset")); - EXPECT_TRUE(props.contains("tolerance")); - EXPECT_TRUE(props.contains("rotation_tolerance_deg")); - } + for (const auto& v : tools) { + const QJsonObject t = v.toObject(); + const QString toolName = t.value("name").toString(); + if (toolName != "simplify_animation" && toolName != "analyze_animation") continue; + const QJsonObject schema = t.value("inputSchema").toObject(); + const QJsonObject props = schema.value("properties").toObject(); + EXPECT_TRUE(props.contains("preset")) << toolName.toStdString(); + EXPECT_TRUE(props.contains("tolerance")) << toolName.toStdString(); + EXPECT_TRUE(props.contains("rotation_tolerance_deg")) << toolName.toStdString(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 3988 - 4020, The test ToolsListIncludesSimplifyAndAnalyzeAnimation currently checks inputSchema properties only for simplify_animation; extend the same schema assertions to also find the tool with name "analyze_animation" and assert its inputSchema->properties contains "preset", "tolerance", and "rotation_tolerance_deg". Locate the loop over tools (variable tools / const QJsonObject t) that checks t.value("name") for "simplify_animation" and duplicate or generalize that logic so it runs for "analyze_animation" as well, adding EXPECT_TRUE checks for the same properties on the analyze_animation schema.src/PropertiesPanelController.cpp (1)
616-635: Move preset mapping into shared code before it drifts.This is now another copy of the public preset table.
MCPServer.cppalready has its own mapping, and the CLI surface likely has one too. The next tolerance tweak is going to desync one of them. Please expose a sharedpreset -> SimplifyToleranceshelper fromAnimationMerger(or similar) and reuse it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 616 - 635, This function tolerancesForPreset duplicates the preset->SimplifyTolerances table; instead add and use a single shared helper on AnimationMerger (e.g. a public static method like AnimationMerger::simplifyTolerancesForPreset or similar) that returns AnimationMerger::SimplifyTolerances for a preset string, move the mapping logic into that new AnimationMerger method, and replace this local tolerancesForPreset implementation to call the new shared helper (update any callers such as this file and MCPServer.cpp / CLI to use the same AnimationMerger helper).src/AnimationMerger_test.cpp (1)
596-735: Add rotation/scale regression cases for the new simplifier.These tests only exercise translation tracks. The new antipodal quaternion slerp path and scale tolerance logic are the riskiest branches in this PR, and a regression there would still pass this suite. Please add at least one
qvs-qrotation case and one scale-only case.As per coding guidelines,
src/**/*_test.cpp: Add Google Test unit tests for new functionality. Test files live alongside source in src/ with _test.cpp suffix (e.g., Manager_test.cpp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger_test.cpp` around lines 596 - 735, Add unit tests that cover quaternion antipodal handling and scale-only simplification: create two new TEST_F cases (e.g., SimplifyAnimationAntipodalRotation and SimplifyAnimationScaleOnly) modeled after the existing SimplifyAnimation* tests that build a skeleton, create an animation/track, and populate keyframes; for the rotation test use keyframes whose rotations are q and -q (setRotation with Ogre::Quaternion and its negation) across multiple times and assert AnimationMerger::simplifyAnimation collapses redundant rotation keys while preserving endpoints; for the scale test use tracks that vary only in scale (setScale with small noise) and use AnimationMerger::SimplifyTolerances + analyzeRedundantKeyframes and simplifyAnimation to assert behavior under tight vs loose tolerances; reference the existing test helpers and methods used above (AnimationMerger::simplifyAnimation, AnimationMerger::analyzeRedundantKeyframes, AnimationMerger::SimplifyTolerances, track->createNodeKeyFrame, getNodeKeyFrame, setRotation, setScale) so the new tests exercise the antipodal slerp and scale-tolerance branches.src/MCPServer.cpp (2)
2258-2290: Optional: extract the entity/animation-list resolution into a helper.The ~30-line block that resolves
entity_name/animation_nameagainst the scene is duplicated verbatim betweentoolSimplifyAnimationandtoolAnalyzeAnimation. A small private helper returning{entity, skeleton, animNames, errorJson}would eliminate the duplication and the same pattern could later be shared withtoolResampleAnimation.Also applies to: 2346-2378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer.cpp` around lines 2258 - 2290, Extract the duplicated entity/animation resolution logic into a private helper (e.g., resolveEntityAndAnimations) that accepts the incoming args and SceneManager pointer and returns a struct or tuple containing Ogre::Entity* entity, Ogre::SkeletonPtr skeleton, std::vector<std::string> animNames and an optional error QVariant/JSON; replace the duplicated blocks in toolSimplifyAnimation and toolAnalyzeAnimation (and later toolResampleAnimation) to call resolveEntityAndAnimations, check the returned error and early-return if present, and otherwise use the returned entity/skeleton/animNames for the rest of each tool.
2292-2302: Avoid double-walking each animation just to computetotalOriginal.
analyzeRedundantKeyframesruns the fullsimplifyTrackKeyspass internally; calling it beforesimplifyAnimationdoubles the per-animation work and the second redundant count is discarded. Counting raw keyframes via the track list is O(N) and gives the sametotalOriginal.♻️ Suggested refactor
int totalRemoved = 0; int totalOriginal = 0; for (const auto& name : animNames) { - int origTotal = 0, origRedundant = 0; - AnimationMerger::analyzeRedundantKeyframes( - skel->getAnimation(name), tol, &origTotal, &origRedundant); - totalOriginal += origTotal; + const Ogre::Animation* anim = skel->getAnimation(name); + for (const auto& [handle, track] : anim->_getNodeTrackList()) + totalOriginal += track->getNumKeyFrames(); int removed = AnimationMerger::simplifyAnimation(skel.get(), name, tol); totalRemoved += removed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer.cpp` around lines 2292 - 2302, The loop currently calls AnimationMerger::analyzeRedundantKeyframes for each animation (doubling work because it internally runs simplifyTrackKeys) and then calls AnimationMerger::simplifyAnimation; instead, compute totalOriginal in O(N) by iterating the animation's track list and summing key counts from skel->getAnimation(name) (or equivalent track container) and remove the analyzeRedundantKeyframes call, leaving only AnimationMerger::simplifyAnimation(skel.get(), name, tol) to perform the actual simplification while accumulating totalRemoved and the precomputed totalOriginal.src/AnimationMerger.cpp (1)
547-573: Optional: default-initializeTrackDatamembers to silence the cppcheck hint.Cppcheck flags line 572 (
uninitStructMember) — it's a false positive (all members are assigned beforepush_back), but adding default member initializers makes the struct unconditionally safe under future edits and quiets the warning.♻️ Suggested cleanup
struct TrackData { - unsigned short handle; - Ogre::Node* associatedNode; - bool useShortestPath; + unsigned short handle = 0; + Ogre::Node* associatedNode = nullptr; + bool useShortestPath = true; std::vector<SimpleKey> keys; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 547 - 573, Cppcheck warns about possibly uninitialized members in struct TrackData; to silence this and make future edits safer, add default member initializers to TrackData (e.g., initialize handle, associatedNode, useShortestPath and keys) so every field is always initialized when the struct is constructed before being filled in the loop that iterates srcAnim->_getNodeTrackList(), where td is populated and later moved into tracks; update the TrackData definition accordingly so the warning is eliminated without changing the logic around simplifyTrackKeys(td.keys, tol) or tracks.push_back(std::move(td)).src/CLIPipeline.cpp (2)
1497-1501: Reported percentage uses pre-simplifytotalRedundant, not actualtotalRemoved.
pctTotalis computed from the analysis pass (totalRedundant / totalOriginal), but the success line showstotalRemovedpaired with that percentage. They should match in practice (same tolerances, same animations), but ifsimplifyAnimationever diverges fromanalyzeRedundantKeyframes(e.g., a track type one supports and the other doesn’t, or future iterative-passes), the printed percentage will silently drift from the reported counts. Consider deriving the printed percentage fromtotalRemoved / totalOriginalso the line is internally consistent:♻️ Suggested tweak
- cliWrite(QString("Simplified %1 animation(s): removed %2/%3 keyframes (%4%)\nOutput: %5\n") - .arg(animsProcessed).arg(totalRemoved).arg(totalOriginal) - .arg(pctTotal, 0, 'f', 1).arg(outFi.fileName())); + const double removedPct = totalOriginal > 0 + ? (100.0 * totalRemoved / totalOriginal) : 0.0; + cliWrite(QString("Simplified %1 animation(s): removed %2/%3 keyframes (%4%)\nOutput: %5\n") + .arg(animsProcessed).arg(totalRemoved).arg(totalOriginal) + .arg(removedPct, 0, 'f', 1).arg(outFi.fileName()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 1497 - 1501, The success message uses pctTotal (computed from totalRedundant) but prints totalRemoved, causing inconsistent output if analyzeRedundantKeyframes and simplifyAnimation diverge; update the code so the percent is computed from the actual removed count (totalRemoved/totalOriginal) before writing the success line (or recompute a new pctRemoved variable) and use that percent in the cliWrite call; refer to pctTotal, totalRedundant, totalRemoved, totalOriginal, analyzeRedundantKeyframes and simplifyAnimation to locate where to recompute and print the percentage.
254-258: Document--tolerancecoupling translation and scale.
--tolerance Twrites bothsimplifyTranslationTolandsimplifyScaleTolto the same value (Lines 1097–1098), but the usage strings (Lines 254, 1132) just describe it as "tolerance T". Users won’t know there’s no separate--scale-toleranceand that this single knob silently overrides the scale tolerance from--presetif specified after it. Consider either:
- Documenting the coupling in the help text (e.g.,
--tolerance T Translation/scale tolerance (world units)), or- Splitting into
--translation-toleranceand--scale-toleranceto mirror the three fields inAnimationMerger::SimplifyTolerances.Also applies to: 1096-1104, 1132-1133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 254 - 258, The help text and option parsing in CLIPipeline currently treat --tolerance T as a single knob that sets both simplifyTranslationTol and simplifyScaleTol (symbols: simplifyTranslationTol, simplifyScaleTol, and AnimationMerger::SimplifyTolerances), but the usage strings do not document this coupling; update the CLI to either (a) make the coupling explicit by changing the usage/help strings (the usage lines around the anim help and the analyze help where "--tolerance T" appears) to something like "--tolerance T Translation/scale tolerance (world units)" or (b) split the option into --translation-tolerance and --scale-tolerance and wire those new flags into the existing parsing logic so they populate simplifyTranslationTol and simplifyScaleTol separately (ensure the former single --tolerance remains supported for backward compatibility by mapping it to both if used); adjust parsing code that currently assigns both simplifyTranslationTol and simplifyScaleTol from --tolerance and update AnimationMerger::SimplifyTolerances callers accordingly.
🤖 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 1716-1749: The ToolTip currently calls
PropertiesPanelController.analyzeAnimationKeyframes in a live binding causing
expensive analysis every delegate refresh; change this so the analysis is
performed lazily on hover or when the preset changes and cache the result for
the row, and hide the whole simplify button for unsupported rows by gating its
visibility with grp.hasSkeleton. Concretely: remove the direct call from
ToolTip.text and instead run analyzeAnimationKeyframes once when
simplifyMouse.containsMouse becomes true (or onEntered) and when
entityGroupColumn.simplifyPreset changes, store the returned object on the
delegate (e.g., cachedAnalysis), update ToolTip.text to read from cachedAnalysis
(show a fallback like "analyze unavailable" while null), and set the
Rectangle/simplifyBtn.visible (or enabled) to grp.hasSkeleton so non-skeletal
rows never show the button; keep the existing simplifyAnimation call on
simplifyMouse.onClicked and invalidate cachedAnalysis when preset changes.
In `@src/CLIPipeline.cpp`:
- Around line 1407-1471: The projected/projectedSize and savedBytes use the
whole-file originalSize but pctTotal is computed only from matched animations,
so the "Simplify to save ~…" line misreports savings when an animation filter is
active; fix by either (A) restricting the printed projection to only when no
per-animation filter was provided (add a guard so the existing if
(totalRedundant > 0 && originalSize > 0) also requires "no animation filter is
active"), or (B) compute the projection only for the matched animations by
scaling the savings using matched_keyframes / total_keyframes_in_file (i.e.,
compute projectedSize = originalSize * (matched_keyframes /
total_keyframes_in_file) * (1.0 - pctTotal/100.0)), and update savedBytes
accordingly; change the code around pctTotal/projectedSize/savedBytes and the
printf that emits the "Simplify to save ~" line (references: originalSize,
totalOriginal, totalRedundant, pctTotal, projectedSize, savedBytes, and the
block that builds the human-readable report) to implement one of these fixes.
In `@src/MCPServer.cpp`:
- Around line 2213-2244: The header comment is stale: update the comment above
tolerancesFromMcpArgs to accurately describe that the function returns an
AnimationMerger::SimplifyTolerances struct and reports errors via the optional
bool outOk parameter (not by returning a JSON object); state that it reads
"preset", "tolerance", "rotation_tolerance_deg", and "scale_tolerance" from
args, applies preset defaults, overrides individual fields if present, and sets
*outOk = false for unknown preset values so callers can propagate the failure.
In `@src/MCPServer.h`:
- Around line 156-157: Add a new MCP interface version because you added
toolSimplifyAnimation and toolAnalyzeAnimation to the MCP surface: update the
SERVER_VERSION constant in MCPServer.h to a higher MCP version (e.g., from
"1.3.0" to "1.4.0" or whatever your versioning scheme dictates) so
capability/version negotiation reflects the new methods (referencing
SERVER_VERSION, toolSimplifyAnimation, and toolAnalyzeAnimation).
In `@src/ScanEngine.cpp`:
- Around line 234-241: The scanner is counting position/rotation/scale samples
separately (summing ch->mNumPositionKeys + mNumRotationKeys + mNumScalingKeys),
but AnimationMerger::analyzeRedundantKeyframes() treats a node keyframe
atomically, so the percentage is inflated; change the logic in ScanEngine.cpp
(the loop iterating anim->mChannels / aiNodeAnim) to compute per-node keyframe
timelines instead of per-sample counts — e.g., merge the position/rotation/scale
time arrays into a single set of unique key times per aiNodeAnim and use that
set's size for total and for redundancy computation (or call/reuse
AnimationMerger::analyzeRedundantKeyframes counting semantics) and replace uses
of countRedundantVecKeys/countRedundantQuatKeys aggregation with a per-node
redundancy check that mirrors the simplifier’s semantics so
redundant_keyframes_pct matches the simplifier.
- Around line 649-650: The code mutates AssetInfo via const_cast in
evaluateRules; change the signature of evaluateRules to take a non-const
AssetInfo& (not const AssetInfo&) so you can assign totalKeyframes and
redundantKeyframes safely, then update all callers (including tests that pass
const AssetInfo) to pass a mutable AssetInfo or create a local copy to pass by
reference; alternatively, refactor evaluateRules to return a small struct or
pair with the redundancy metrics instead of mutating AssetInfo. Ensure you
update references to totalKeyframes and redundantKeyframes assignments
accordingly.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 1149-1151: The breadcrumb mislabels simplify/analyze as merge
because animOp only checks listMode/renameMode/resampleMode/decimateMode; update
the animOp computation used with SentryReporter::addBreadcrumb to include
simplifyMode and analyzeMode (e.g., check simplifyMode and analyzeMode in the
ternary chain before falling back to mergeMode) so animOp correctly becomes
"simplify" or "analyze" when those flags are set, keeping the existing mergeMode
branch and the mergeFiles suffix logic unchanged.
---
Nitpick comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 596-735: Add unit tests that cover quaternion antipodal handling
and scale-only simplification: create two new TEST_F cases (e.g.,
SimplifyAnimationAntipodalRotation and SimplifyAnimationScaleOnly) modeled after
the existing SimplifyAnimation* tests that build a skeleton, create an
animation/track, and populate keyframes; for the rotation test use keyframes
whose rotations are q and -q (setRotation with Ogre::Quaternion and its
negation) across multiple times and assert AnimationMerger::simplifyAnimation
collapses redundant rotation keys while preserving endpoints; for the scale test
use tracks that vary only in scale (setScale with small noise) and use
AnimationMerger::SimplifyTolerances + analyzeRedundantKeyframes and
simplifyAnimation to assert behavior under tight vs loose tolerances; reference
the existing test helpers and methods used above
(AnimationMerger::simplifyAnimation, AnimationMerger::analyzeRedundantKeyframes,
AnimationMerger::SimplifyTolerances, track->createNodeKeyFrame, getNodeKeyFrame,
setRotation, setScale) so the new tests exercise the antipodal slerp and
scale-tolerance branches.
In `@src/AnimationMerger.cpp`:
- Around line 547-573: Cppcheck warns about possibly uninitialized members in
struct TrackData; to silence this and make future edits safer, add default
member initializers to TrackData (e.g., initialize handle, associatedNode,
useShortestPath and keys) so every field is always initialized when the struct
is constructed before being filled in the loop that iterates
srcAnim->_getNodeTrackList(), where td is populated and later moved into tracks;
update the TrackData definition accordingly so the warning is eliminated without
changing the logic around simplifyTrackKeys(td.keys, tol) or
tracks.push_back(std::move(td)).
In `@src/CLIPipeline.cpp`:
- Around line 1497-1501: The success message uses pctTotal (computed from
totalRedundant) but prints totalRemoved, causing inconsistent output if
analyzeRedundantKeyframes and simplifyAnimation diverge; update the code so the
percent is computed from the actual removed count (totalRemoved/totalOriginal)
before writing the success line (or recompute a new pctRemoved variable) and use
that percent in the cliWrite call; refer to pctTotal, totalRedundant,
totalRemoved, totalOriginal, analyzeRedundantKeyframes and simplifyAnimation to
locate where to recompute and print the percentage.
- Around line 254-258: The help text and option parsing in CLIPipeline currently
treat --tolerance T as a single knob that sets both simplifyTranslationTol and
simplifyScaleTol (symbols: simplifyTranslationTol, simplifyScaleTol, and
AnimationMerger::SimplifyTolerances), but the usage strings do not document this
coupling; update the CLI to either (a) make the coupling explicit by changing
the usage/help strings (the usage lines around the anim help and the analyze
help where "--tolerance T" appears) to something like "--tolerance T
Translation/scale tolerance (world units)" or (b) split the option into
--translation-tolerance and --scale-tolerance and wire those new flags into the
existing parsing logic so they populate simplifyTranslationTol and
simplifyScaleTol separately (ensure the former single --tolerance remains
supported for backward compatibility by mapping it to both if used); adjust
parsing code that currently assigns both simplifyTranslationTol and
simplifyScaleTol from --tolerance and update AnimationMerger::SimplifyTolerances
callers accordingly.
In `@src/MCPServer_test.cpp`:
- Around line 3988-4020: The test ToolsListIncludesSimplifyAndAnalyzeAnimation
currently checks inputSchema properties only for simplify_animation; extend the
same schema assertions to also find the tool with name "analyze_animation" and
assert its inputSchema->properties contains "preset", "tolerance", and
"rotation_tolerance_deg". Locate the loop over tools (variable tools / const
QJsonObject t) that checks t.value("name") for "simplify_animation" and
duplicate or generalize that logic so it runs for "analyze_animation" as well,
adding EXPECT_TRUE checks for the same properties on the analyze_animation
schema.
In `@src/MCPServer.cpp`:
- Around line 2258-2290: Extract the duplicated entity/animation resolution
logic into a private helper (e.g., resolveEntityAndAnimations) that accepts the
incoming args and SceneManager pointer and returns a struct or tuple containing
Ogre::Entity* entity, Ogre::SkeletonPtr skeleton, std::vector<std::string>
animNames and an optional error QVariant/JSON; replace the duplicated blocks in
toolSimplifyAnimation and toolAnalyzeAnimation (and later toolResampleAnimation)
to call resolveEntityAndAnimations, check the returned error and early-return if
present, and otherwise use the returned entity/skeleton/animNames for the rest
of each tool.
- Around line 2292-2302: The loop currently calls
AnimationMerger::analyzeRedundantKeyframes for each animation (doubling work
because it internally runs simplifyTrackKeys) and then calls
AnimationMerger::simplifyAnimation; instead, compute totalOriginal in O(N) by
iterating the animation's track list and summing key counts from
skel->getAnimation(name) (or equivalent track container) and remove the
analyzeRedundantKeyframes call, leaving only
AnimationMerger::simplifyAnimation(skel.get(), name, tol) to perform the actual
simplification while accumulating totalRemoved and the precomputed
totalOriginal.
In `@src/PropertiesPanelController.cpp`:
- Around line 616-635: This function tolerancesForPreset duplicates the
preset->SimplifyTolerances table; instead add and use a single shared helper on
AnimationMerger (e.g. a public static method like
AnimationMerger::simplifyTolerancesForPreset or similar) that returns
AnimationMerger::SimplifyTolerances for a preset string, move the mapping logic
into that new AnimationMerger method, and replace this local tolerancesForPreset
implementation to call the new shared helper (update any callers such as this
file and MCPServer.cpp / CLI to use the same AnimationMerger helper).
🪄 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: 837e5b78-9b65-4ede-8d7d-e42a4604a651
📒 Files selected for processing (19)
CMakeLists.txtqml/PropertiesPanel.qmlqml/ThemedComboBox.qmlsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/MaterialEditorQML_qml_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/ScanConfig.cppsrc/ScanConfig.hsrc/ScanEngine.cppsrc/ScanEngine.hsrc/ScanEngine_test.cppsrc/qml_resources.qrc
Correctness: - CLIPipeline: Sentry breadcrumb no longer mislabels --simplify/--analyze as "merge" (animOp ternary now covers all modes). - CLIPipeline: simplify success line computes pct from totalRemoved (not the pre-pass analyze count) so it stays consistent if the simplifier and analyzer ever drift. - CLIPipeline: --analyze projection skipped when --animation filters to one clip — previously scaled the whole-file size by a per-clip pct, overstating savings. - ScanEngine: redundant_keyframes_pct now treats node keyframes atomically. Assimp stores T/R/S as three separate streams, but AnimationMerger collapses them into one per-time record; counting the three streams independently was inflating the scan rule's % vs what --simplify actually removes (Mixamo Rumba: 77.9% → 41.9%, matching the simplifier's 42.1%). - ScanEngine: drop const_cast on AssetInfo. Redundancy totals are now filled at inspectAsset time using default tolerances; evaluateRules re-runs with config tolerances purely to decide whether to emit a finding. - MCPServer simplify_animation: drop the redundant analyze-then-simplify call (analyzeRedundantKeyframes runs the full simplifier pass internally). Count totalOriginal directly from the track list. Sharing: - New AnimationMerger::tolerancesForPreset(name, *outOk) is the single source of truth for the conservative/balanced/aggressive table. PropertiesPanelController, MCPServer and CLIPipeline all forward to it — bumping a preset value updates every surface. UI: - Inspector simplify-tooltip no longer runs analyzeRedundantKeyframes in a live binding (~25ms per evaluation on Mixamo clips). Cached on first hover, invalidated when preset changes or after a simplify. - Scissors button hidden for non-skeletal entities since simplifyAnimation requires node tracks. Tests: - New AnimationMerger antipodal-rotation test (q vs -q on alternating keys must collapse). - New scale-only-track test — tight tolerance keeps wobble, loose collapses it. - New TolerancesForPresetMapping standalone test pins the preset table values. - MCP tools/list test now asserts analyze_animation also exposes preset/tolerance/rotation_tolerance_deg. Misc: - TrackData struct in simplifyAnimation now has default member initializers (silences cppcheck uninitStructMember). - CLI usage/help clarifies --tolerance sets translation+scale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MCP surface changed in this PR — added simplify_animation and analyze_animation tools. Per the project's MCP versioning rule, SERVER_VERSION (separate from app version) bumps when the interface expands so capability negotiation stays unambiguous. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 28, 2026
This was referenced May 5, 2026
This was referenced May 13, 2026
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
Adds a new operation that walks each animation track and removes any keyframe whose value matches the lerp/slerp of its neighbors within tolerance. First/last keys and sharp pose changes are preserved, so Mixamo-style baked clips shed 40-60% of keys without visible drift.
Surfaces:
AnimationMerger::simplifyAnimation+analyzeRedundantKeyframes) — antipodal-aware quaternion slerp, configurable per-axis tolerances.qtmesh anim file.fbx --simplify/--analyze, with--preset {conservative|balanced|aggressive}plus per-axis--toleranceand--rotation-tolerance-degoverrides.redundant_keyframes_pct: warns when projected savings exceed a threshold; message format:\"X% redundant keyframes (N/M). Simplify it to save ~Y. Original size: A, projected size: B\".simplify_animation+analyze_animation— preset + per-axis overrides, optionalentity_name/animation_name.Defaults
The new default is the Balanced preset: 1mm translation, 0.5° rotation, 1mm scale. Visually indistinguishable on meter-scale character rigs, drops 40-60% of keys on Mixamo clips. CLI / MCP / UI share the same preset definitions.
Verified on Mixamo
Rumba Dancing.fbx(1.89 MB):Other changes
ThemedComboBoxswitched fromMaterialEditorQMLtoThemeManagerso it works in any panel; aliased into the/PropertiesPanelresource prefix so the inspector can use it without cross-module imports. QML loading test fixture now registersThemeManagerso existing Material editor tests keep passing.Test plan
qtmesh anim file.fbx --analyze --preset aggressiveqtmesh scan ./assets --config qtmesh.ymlwithredundant_keyframes_pct: 30🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Version