Improve animation merge: prefix base, clean Mixamo, deduplicate - #220
Merged
Conversation
Three improvements to animation name handling during merge:
1. Base entity animations now get prefixed with their node name,
same as source animations. All animations follow the same
{node}_{animation} pattern after merge.
2. Mixamo noise cleanup: removes "mixamo.com" segments from
pipe-separated names before slugifying (e.g.
"Armature|mixamo.com|Layer0" → "armature_layer0").
3. Smart deduplication: when prefix == animation name, avoids
redundant "idle_idle" — produces just "idle". When multiple
animations produce the same final name, appends _2, _3, etc.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAnimations are renamed and deduplicated in a two-phase merge: source animations are cleaned/slugified and deduplicated against a seeded set from the base, copied into the base, then all animations are post-renamed for consistent cleaning/prefixing and final deduplication; stale entity animation states are removed before refresh. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Merger as AnimationMerger
participant Base as BaseSkeleton
participant Source as SourceSkeleton
participant Entity
Caller->>Merger: request merge(source, base)
Merger->>Base: collect existing animation names (seed set)
Merger->>Merger: pre-rename source animations (clean, build names, dedupe vs seed)
Merger->>Merger: apply two-pass temp renames to avoid collisions
Merger->>Merger: _mergeSkeletonAnimations() (copy animations into Base)
Merger->>Merger: post-rename all Base animations (clean/prefix + final dedupe)
Merger->>Entity: remove stale animation states (entity vs skeleton)
Merger->>Entity: refreshAvailableAnimationState()
Merger-->>Caller: merge complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
When renaming base animations, the new name for one animation could equal the old name of another (e.g. "jump" → "test_jump" fails if "test_jump" already exists). Fixed with two-pass rename: first all animations to unique temp names, then temp to final. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: 080fd60774
ℹ️ 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
+152
to
+153
| for (const auto& [oldName, newName] : baseRenames) | ||
| renameAnimation(baseSkel.get(), oldName, newName); |
There was a problem hiding this comment.
The function now renames all base animations before it verifies that any source animation can actually be merged, so error paths still mutate the scene. For example, if every source shares the same skeleton resource (srcSkel.get() == baseSkel.get()) or a later source is incompatible, mergeAnimations returns nullptr but the base animation names have already been changed, which is a surprising side effect for a failed operation and can break callers that retry after failure.
Useful? React with 👍 / 👎.
Comment on lines
+148
to
+149
| QString desired = buildAnimName(baseRawName, QString::fromStdString(origName)); | ||
| QString finalName = deduplicateName(desired, existingNames); |
There was a problem hiding this comment.
Each invocation re-applies the base prefix to existing animation names, so previously renamed entries are renamed again (e.g., basenode_idle becomes basenode_basenode_idle on the next merge call). This causes unstable animation identifiers across repeated merges, which can break downstream references and makes names grow indefinitely instead of staying in a consistent {node}_{animation} form.
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: 2
🧹 Nitpick comments (1)
src/AnimationMerger_test.cpp (1)
269-307: Deduplication test does not currently hit the_2collision branch.At Line 272, the test description says suffixing is validated, but the asserted names (
idle,idle2_idle,idle3_idle) are all unique. Consider forcing two inputs to resolve to the same slugged final name and then asserting_2.💡 Example adjustment
- auto* nodeB = sceneMgr->getRootSceneNode()->createChildSceneNode("idle2"); + auto* nodeB = sceneMgr->getRootSceneNode()->createChildSceneNode("Idle 2"); ... - auto* nodeC = sceneMgr->getRootSceneNode()->createChildSceneNode("idle3"); + auto* nodeC = sceneMgr->getRootSceneNode()->createChildSceneNode("idle_2"); ... - EXPECT_TRUE(skel->hasAnimation("idle2_idle")); - EXPECT_TRUE(skel->hasAnimation("idle3_idle")); + EXPECT_TRUE(skel->hasAnimation("idle_2_idle")); + EXPECT_TRUE(skel->hasAnimation("idle_2_idle_2"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger_test.cpp` around lines 269 - 307, The test never triggers the collision branch because each source yields a unique final name; update the setup so two sources generate the same slugged name (e.g., attach entityB and entityC to the same scene node name "idle2" instead of creating separate "idle2" and "idle3" nodes) so AnimationMerger::mergeAnimations will have to deduplicate and produce a "_2" suffix; adjust the EXPECT_TRUE checks to assert the presence of "idle2_idle" and "idle2_idle_2" (and keep the base "idle") and keep references to createChildSceneNode, attachObject, and the mergeAnimations call to locate the change.
🤖 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/AnimationMerger.cpp`:
- Around line 42-46: The current concatenation returns "_<anim>" when slugPrefix
is empty; update the logic that builds the combined slug (use the slugPrefix and
slugAnim variables) so that if slugPrefix is empty you return slugAnim (or
slugAnim when not empty), otherwise return slugPrefix + "_" + slugAnim; keep the
existing check for slugAnim == slugPrefix and slugAnim.isEmpty() and add a
branch for slugPrefix.isEmpty() to avoid producing a leading underscore.
- Around line 135-154: The code mutates base skeleton animation names (using
baseSkel, renameAnimation, buildAnimName, deduplicateName and existingNames)
before verifying merge viability; defer or make non-destructive changes: compute
and store the intended baseRenames mapping (origName -> finalName) but do not
call renameAnimation until after the merge viability check completes
successfully (i.e., after the logic that may early-return/fail), or
alternatively operate on a temporary clone of baseSkel and apply renameAnimation
to the real baseSkel only when the merge is confirmed; ensure all references to
baseEntity/parentNode use the same raw name source when computing the mapping.
---
Nitpick comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 269-307: The test never triggers the collision branch because each
source yields a unique final name; update the setup so two sources generate the
same slugged name (e.g., attach entityB and entityC to the same scene node name
"idle2" instead of creating separate "idle2" and "idle3" nodes) so
AnimationMerger::mergeAnimations will have to deduplicate and produce a "_2"
suffix; adjust the EXPECT_TRUE checks to assert the presence of "idle2_idle" and
"idle2_idle_2" (and keep the base "idle") and keep references to
createChildSceneNode, attachObject, and the mergeAnimations call to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fe2f82c-7d73-4cfc-bfad-9fd06e8b6312
📒 Files selected for processing (2)
src/AnimationMerger.cppsrc/AnimationMerger_test.cpp
Base animations that already start with the base node's slug are kept as-is (only Mixamo noise cleaned). This prevents "test_jump" becoming "test_test_jump" on subsequent merges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/AnimationMerger.cpp (2)
40-45:⚠️ Potential issue | 🟡 MinorAvoid
_walkwhen the prefix slug is empty.
buildAnimName("", "walk")still returns_walk. ReturnslugAnimwhenslugPrefixis empty so unnamed prefixes do not generate malformed names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 40 - 45, The current logic in buildAnimName returns slugPrefix + "_" + slugAnim even when slugPrefix is empty, producing names like "_walk"; update buildAnimName so that if slugPrefix.isEmpty() it returns slugAnim, otherwise if slugAnim.isEmpty() or slugAnim==slugPrefix return slugPrefix, else return slugPrefix + "_" + slugAnim. Reference slugPrefix, slugAnim and function buildAnimName to implement this branching.
135-168:⚠️ Potential issue | 🔴 CriticalDo not mutate skeletons until the merge is known to succeed.
This can still return
nullptrafter renaming the base skeleton, and after partially processing earlier sources. Pre-scan merge candidates and compatibility first so failure stays side-effect free.💡 Suggested direction
+ QList<Ogre::Entity*> mergeCandidates; + for (Ogre::Entity* srcEntity : sourceEntities) + { + if (!srcEntity || srcEntity == baseEntity || !srcEntity->hasSkeleton()) + continue; + + Ogre::SkeletonPtr srcSkel = srcEntity->getMesh()->getSkeleton(); + if (!srcSkel || srcSkel.get() == baseSkel.get()) + continue; + + if (!areSkeletonsCompatible(baseSkel, srcSkel)) { + errorMsg = QString("Skeleton of '%1' is incompatible with base skeleton") + .arg(srcEntity->getName().c_str()); + return nullptr; + } + + mergeCandidates.append(srcEntity); + } + + if (mergeCandidates.isEmpty()) { + errorMsg = "No animations were merged (no valid source entities found)"; + return nullptr; + } + // rename/merge only after viability is confirmedAlso applies to: 188-192, 227-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 135 - 168, Currently renameAnimation is called on baseSkel (via the baseRenames/tempToFinal logic) before the overall merge is validated, causing irreversible mutations if later steps fail; change the flow to pre-scan and validate all merge candidates and compatibility first (collect all animation rename mappings using buildAnimName and deduplicateName into baseRenames/tempToFinal without calling renameAnimation), run the merge feasibility checks (the same checks referenced for the other occurrences at the regions noted), and only if validation succeeds iterate tempToFinal and call renameAnimation on baseSkel; apply the same pre-scan-then-mutate pattern to the other similar blocks around the other occurrences (lines referenced 188-192 and 227-230) so no skeleton is modified unless the merge is guaranteed to proceed.
🤖 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/AnimationMerger.cpp`:
- Around line 157-163: The temporary-name scheme can collide with existing
animations; when creating tempName in the pass that fills tempToFinal (use
baseRenames, tempToFinal, renameAnimation, baseSkel), probe
baseSkel->hasAnimation(tempName) in a loop and mutate the candidate (e.g., add a
numeric suffix or increment an inner counter) until you find one that does not
exist, then call renameAnimation with that unique tempName and append the
mapping to tempToFinal; ensure the probing logic updates tempName per iteration
so every generated "__merge_temp_*" is guaranteed unused before renaming.
---
Duplicate comments:
In `@src/AnimationMerger.cpp`:
- Around line 40-45: The current logic in buildAnimName returns slugPrefix + "_"
+ slugAnim even when slugPrefix is empty, producing names like "_walk"; update
buildAnimName so that if slugPrefix.isEmpty() it returns slugAnim, otherwise if
slugAnim.isEmpty() or slugAnim==slugPrefix return slugPrefix, else return
slugPrefix + "_" + slugAnim. Reference slugPrefix, slugAnim and function
buildAnimName to implement this branching.
- Around line 135-168: Currently renameAnimation is called on baseSkel (via the
baseRenames/tempToFinal logic) before the overall merge is validated, causing
irreversible mutations if later steps fail; change the flow to pre-scan and
validate all merge candidates and compatibility first (collect all animation
rename mappings using buildAnimName and deduplicateName into
baseRenames/tempToFinal without calling renameAnimation), run the merge
feasibility checks (the same checks referenced for the other occurrences at the
regions noted), and only if validation succeeds iterate tempToFinal and call
renameAnimation on baseSkel; apply the same pre-scan-then-mutate pattern to the
other similar blocks around the other occurrences (lines referenced 188-192 and
227-230) so no skeleton is modified unless the merge is guaranteed to proceed.
The base-rename-during-merge approach caused duplications because Ogre's _mergeSkeletonAnimations interacts badly with mid-flight skeleton modifications. New approach: 1. Merge source animations (prefixed) into base skeleton as before 2. AFTER all merges complete, post-process ALL animations in one pass: - Base animations get prefixed with their node name - Already-prefixed animations (from previous merges) kept as-is - Mixamo noise cleaned from all names - Deduplication with _2, _3 suffixes 3. Two-pass rename (old→temp→final) avoids name collisions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Only prepend node/file name when the animation name is generic (e.g. "mixamo.com" → becomes node name). Meaningful names like "jump", "idle" are kept as-is with just Mixamo noise cleaned. - Fix dedup: strip existing _N suffix before incrementing, so repeated merges produce jump, jump_2, jump_3 (not jump_2_2_2) - Remove stale animation states after rename (was causing 9 instead of 5 animations in the UI) - Remove debug fprintf statements Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New feature: improved animation merge naming. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AnimationMerger.cpp (1)
164-169:⚠️ Potential issue | 🟠 MajorGuard against shared source skeleton resources.
The loop at line 164 only skips a source when it shares the base skeleton (line 168). If two selected sources reference the same non-base mesh/skeleton resource, the first iteration renames that skeleton's animations in-place and merges them into the base. When the second iteration processes the same skeleton pointer, the animations have already been renamed; attempting to rename them again (lines 197–201) causes the skeleton to be mutated a second time with incorrect animation names, corrupting the animation set.
Track seen
Skeleton*pointers to process each unique skeleton only once, or rename on isolated clones before merge.Also applies to: 200–203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 164 - 169, The loop currently skips only when srcSkel equals baseSkel but needs to also skip duplicate source skeletons; add a guard that tracks processed skeleton pointers (e.g., a std::unordered_set<Ogre::Skeleton*> seenSkels) and before renaming/merging check if srcSkel.get() is already in seenSkels, skip if so, otherwise insert it and proceed; apply the same duplicate-pointer check around the animation rename/merge logic that operates on srcSkel (the rename block referenced in the review) so each unique Skeleton* is processed exactly once (alternatively, clone srcSkel before in-place renames if you prefer cloning instead of pointer deduplication).
♻️ Duplicate comments (3)
src/AnimationMerger.cpp (3)
156-176:⚠️ Potential issue | 🔴 CriticalPre-scan all sources before the first rename/merge.
If one source merges successfully and a later source fails the compatibility check at Line 171, this returns
nullptrafter earliersrcSkels have already been renamed andbaseSkelhas already been mutated. Please do a read-only validation pass oversourceEntitiesfirst, then start renaming/merging once failure is no longer possible.Also applies to: 189-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 156 - 176, Pre-scan sourceEntities in a read-only validation pass before doing any renaming/merging: iterate over each srcEntity (skipping baseEntity), verify srcEntity is non-null, hasSkeleton(), obtain srcSkel via srcEntity->getMesh()->getSkeleton(), and call areSkeletonsCompatible(baseSkel, srcSkel); if any check fails set errorMsg (use the same message text referencing srcEntity->getName()) and return nullptr without mutating baseSkel or renaming skeletons; only after this validation loop succeeds run the existing second loop that performs the renames/merges (the code around the current blocks that reference baseEntity, baseSkel, and areSkeletonsCompatible). This same two-phase pattern should also be applied to the later block mentioned (the code covering the other duplicate region).
189-201:⚠️ Potential issue | 🔴 CriticalSource renames still need the temp-name pass.
This is back to a one-pass
old -> finalrename. A source that contains bothmixamo.com|walkandwalkcan still collide here becauserenameAnimation()creates the destination before removing the source animation.💡 Suggested fix
- for (const auto& [oldName, newName] : renameList) - renameAnimation(srcSkel.get(), oldName, newName); + QList<std::pair<std::string, std::string>> tempToFinal; + for (int i = 0; i < renameList.size(); ++i) + { + std::string tempName = "__merge_temp_src_" + std::to_string(i); + while (srcSkel->hasAnimation(tempName)) + tempName += "_x"; + + renameAnimation(srcSkel.get(), renameList[i].first, tempName); + tempToFinal.append({tempName, renameList[i].second}); + } + + for (const auto& [tempName, finalName] : tempToFinal) + renameAnimation(srcSkel.get(), tempName, finalName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 189 - 201, The current single-pass renaming (building renameList and calling renameAnimation(old->final)) can collide when an original name equals another final name because renameAnimation creates the destination before removing the source; change to a two-pass approach: first generate a temp unique name for each original (using buildAnimName/deduplicateName logic but ensuring temp names are distinct and do not collide with existingNames or originals), apply renameAnimation(srcSkel.get(), orig, temp) for all entries, then in a second loop renameAnimation(srcSkel.get(), temp, final) to move temps to their desired final names; reference the existing symbols renameAnimation, buildAnimName, deduplicateName, existingNames, srcSkel and the renameList mapping when implementing the two-pass fix.
254-258:⚠️ Potential issue | 🟡 MinorMake the base temp names collision-proof.
__merge_temp_<i>is still assumed to be unused. If a skeleton already contains__merge_temp_0, the collision-avoidance pass collides with a real animation name on the first rename.💡 Suggested fix
for (int i = 0; i < renames.size(); ++i) { std::string tempName = "__merge_temp_" + std::to_string(i); + while (baseSkel->hasAnimation(tempName)) + tempName += "_x"; renameAnimation(baseSkel.get(), renames[i].first, tempName); tempToFinal.append({tempName, renames[i].second}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 254 - 258, Temp names like "__merge_temp_<i>" can collide with existing animations; update the loop that builds tempName (around renameAnimation, baseSkel, tempToFinal, renames) to generate collision-proof names by looping until a unique token is found: produce a candidate tempName (e.g., "__merge_temp_<i>_<nonce>"), and check that it does not already exist on baseSkel (via the skeleton's animation lookup) and is not already used in tempToFinal or in any renames targets; increment the nonce (or generate a UUID) until unique, then call renameAnimation(baseSkel.get(), original, tempName) and append {tempName, final} to tempToFinal. Ensure the uniqueness check covers both existing skeleton animations and previously reserved temp names.
🤖 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/AnimationMerger.cpp`:
- Around line 35-46: The buildAnimName function currently returns slugAnim
unprefixed for non-empty names, breaking callers that expect a stable prefix;
modify buildAnimName (which uses cleanMixamoNoise and slugify) so it always
includes the prefix when building the final name — e.g., compute slugPrefix =
slugify(prefix) and if slugAnim is empty return slugPrefix, otherwise return
slugPrefix + "_" + slugAnim (or equivalent stable concatenation), ensuring all
callers receive a consistently prefixed, slugified animation name to avoid
cross-entity collisions.
---
Outside diff comments:
In `@src/AnimationMerger.cpp`:
- Around line 164-169: The loop currently skips only when srcSkel equals
baseSkel but needs to also skip duplicate source skeletons; add a guard that
tracks processed skeleton pointers (e.g., a std::unordered_set<Ogre::Skeleton*>
seenSkels) and before renaming/merging check if srcSkel.get() is already in
seenSkels, skip if so, otherwise insert it and proceed; apply the same
duplicate-pointer check around the animation rename/merge logic that operates on
srcSkel (the rename block referenced in the review) so each unique Skeleton* is
processed exactly once (alternatively, clone srcSkel before in-place renames if
you prefer cloning instead of pointer deduplication).
---
Duplicate comments:
In `@src/AnimationMerger.cpp`:
- Around line 156-176: Pre-scan sourceEntities in a read-only validation pass
before doing any renaming/merging: iterate over each srcEntity (skipping
baseEntity), verify srcEntity is non-null, hasSkeleton(), obtain srcSkel via
srcEntity->getMesh()->getSkeleton(), and call areSkeletonsCompatible(baseSkel,
srcSkel); if any check fails set errorMsg (use the same message text referencing
srcEntity->getName()) and return nullptr without mutating baseSkel or renaming
skeletons; only after this validation loop succeeds run the existing second loop
that performs the renames/merges (the code around the current blocks that
reference baseEntity, baseSkel, and areSkeletonsCompatible). This same two-phase
pattern should also be applied to the later block mentioned (the code covering
the other duplicate region).
- Around line 189-201: The current single-pass renaming (building renameList and
calling renameAnimation(old->final)) can collide when an original name equals
another final name because renameAnimation creates the destination before
removing the source; change to a two-pass approach: first generate a temp unique
name for each original (using buildAnimName/deduplicateName logic but ensuring
temp names are distinct and do not collide with existingNames or originals),
apply renameAnimation(srcSkel.get(), orig, temp) for all entries, then in a
second loop renameAnimation(srcSkel.get(), temp, final) to move temps to their
desired final names; reference the existing symbols renameAnimation,
buildAnimName, deduplicateName, existingNames, srcSkel and the renameList
mapping when implementing the two-pass fix.
- Around line 254-258: Temp names like "__merge_temp_<i>" can collide with
existing animations; update the loop that builds tempName (around
renameAnimation, baseSkel, tempToFinal, renames) to generate collision-proof
names by looping until a unique token is found: produce a candidate tempName
(e.g., "__merge_temp_<i>_<nonce>"), and check that it does not already exist on
baseSkel (via the skeleton's animation lookup) and is not already used in
tempToFinal or in any renames targets; increment the nonce (or generate a UUID)
until unique, then call renameAnimation(baseSkel.get(), original, tempName) and
append {tempName, final} to tempToFinal. Ensure the uniqueness check covers both
existing skeleton animations and previously reserved temp names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5792dfea-8f57-4f8c-9520-7d13bbd61e1d
📒 Files selected for processing (2)
src/AnimationMerger.cppsrc/AnimationMerger_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AnimationMerger_test.cpp
…source rename - Guard against empty prefix in buildAnimName (returns slugAnim) - Temp names (__merge_temp_*) now probe hasAnimation() to avoid collisions with real animation names - Source skeleton renames use two-pass (old→temp→final) same as base post-processing, preventing old↔new name collisions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/AnimationMerger.cpp (2)
281-297: Consider order consistency withSkeletonTransform::renameAnimation.This code removes stale states before
refreshAvailableAnimationState(), whereasSkeletonTransform.cpp:193-196removes them after refresh with the comment "if the master skeleton re-created it." Here, since the skeleton already contains only final names after the two-pass rename, the order is correct—stale names won't be recreated. The logic is sound, but you may want to add a brief comment explaining why the order differs fromSkeletonTransform, to avoid future confusion.💡 Suggested comment addition
// Rebuild animation states from scratch. refreshAvailableAnimationState() only // adds new states but doesn't remove stale ones from renamed/removed animations. + // Note: We remove stale states first (unlike SkeletonTransform::renameAnimation) + // because the skeleton already contains only final names after two-pass renames. {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 281 - 297, The existing block in AnimationMerger.cpp removes stale animation states from baseEntity->getAllAnimationStates() before calling baseEntity->refreshAvailableAnimationState(), which differs from the post-refresh removal in SkeletonTransform::renameAnimation; leave the current order as-is (because the skeleton already contains final names after two-pass rename so stale states won't be recreated) but add a brief clarifying comment above this block referencing SkeletonTransform::renameAnimation and explaining why removal-before-refresh is intentional here to avoid future confusion; mention the functions getAllAnimationStates(), removeAnimationState(), and refreshAvailableAnimationState() in that comment for clarity.
248-259: Comments don't matchbuildAnimName()behavior.Lines 250 and 255 say base animations will be "prefix[ed]" and "needs prefix", but
buildAnimName()(lines 47-51) returns only the cleaned animation name for meaningful names—it doesn't prepend the node prefix. The comments should reflect the actual intent: base animations are cleaned/slugified, and only generic names (like empty ormixamo.comresidue) fall back to the prefix.💡 Suggested comment clarification
if (baseAnimNames.contains(origQName)) { - // This was a base animation — prefix it (unless already prefixed) + // This was a base animation — clean it; only generic names get prefix fallback QString origSlug = slugify(origQName); if (origSlug.startsWith(baseSlug + "_") || origSlug == baseSlug) desired = slugify(cleanMixamoNoise(origQName)); // already prefixed else - desired = buildAnimName(baseRawName, origQName); // needs prefix + desired = buildAnimName(baseRawName, origQName); // clean; prefix only if generic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 248 - 259, Update the misleading comments around the base-animation branch to match actual behavior: explain that when origQName is a base animation we generally clean/slugify it (using cleanMixamoNoise + slugify) and only when the cleaned name is generic/empty does buildAnimName(baseRawName, origQName) supply a node prefix/fallback; reference the conditional using baseAnimNames.contains(origQName), the variables origQName, baseSlug, baseRawName, and the helper functions buildAnimName, slugify, and cleanMixamoNoise so readers understand which cases produce a cleaned name versus a prefixed/fallback name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/AnimationMerger.cpp`:
- Around line 281-297: The existing block in AnimationMerger.cpp removes stale
animation states from baseEntity->getAllAnimationStates() before calling
baseEntity->refreshAvailableAnimationState(), which differs from the
post-refresh removal in SkeletonTransform::renameAnimation; leave the current
order as-is (because the skeleton already contains final names after two-pass
rename so stale states won't be recreated) but add a brief clarifying comment
above this block referencing SkeletonTransform::renameAnimation and explaining
why removal-before-refresh is intentional here to avoid future confusion;
mention the functions getAllAnimationStates(), removeAnimationState(), and
refreshAvailableAnimationState() in that comment for clarity.
- Around line 248-259: Update the misleading comments around the base-animation
branch to match actual behavior: explain that when origQName is a base animation
we generally clean/slugify it (using cleanMixamoNoise + slugify) and only when
the cleaned name is generic/empty does buildAnimName(baseRawName, origQName)
supply a node prefix/fallback; reference the conditional using
baseAnimNames.contains(origQName), the variables origQName, baseSlug,
baseRawName, and the helper functions buildAnimName, slugify, and
cleanMixamoNoise so readers understand which cases produce a cleaned name versus
a prefixed/fallback name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbb4dfb4-7004-4858-8c9d-596d30646098
📒 Files selected for processing (2)
CMakeLists.txtsrc/AnimationMerger.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
|
This was referenced Apr 1, 2026
4 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
Improves animation naming during merge so all animations follow a consistent pattern.
Changes
1. Base entity animations get prefixed
Previously only source animations were prefixed with their node name. Now base animations are also prefixed, so all merged animations follow the same
{node}_{animation}pattern.Before: base keeps
idle, sources getwalk_walk,run_runAfter: base gets
character_idle, sources getwalk_walk,run_run2. Mixamo noise cleanup
Removes
mixamo.comfrom pipe-separated animation names before slugifying:Armature|mixamo.com|Layer0→character_armature_layer0mixamo.com|walk→walkanim_walk3. Smart deduplication
idleinstead ofidle_idle_2,_3, etc.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores