fix(animation): resolve the Animator on child objects, not just the exact target - #1337
Open
BurakErdemci wants to merge 2 commits into
Open
fix(animation): resolve the Animator on child objects, not just the exact target#1337BurakErdemci wants to merge 2 commits into
BurakErdemci wants to merge 2 commits into
Conversation
…xact target Imported models keep their Animator on the model root, which is normally a child of the GameObject a caller names. The animator_* actions looked the component up with GetComponent<Animator>() on the target alone, so reads and controls both failed with "No Animator component" on the most common rig setup. Route the seven read/control lookups through a shared AnimatorResolver.Find that falls back to a descendant search. Inactive descendants are included because a disabled rig is still readable. Ambiguity is reported, never guessed. Unity's descendant search is depth-first, so a wrapper holding several rigs returns the first branch however deep - not the nearest rig, and not what a caller would predict. When more than one descendant carries an Animator the call fails and names the candidates instead of silently mutating one of them. Every successful response names the object that actually changed, including the CLI's own success line for animator_play, which echoed the requested target and so contradicted the result it had just printed. This matters most for animator_set_parameter, which in Edit mode writes the shared AnimatorController asset, so the caller has to be able to see which Animator answered. Actions that ADD an Animator (controller_assign) deliberately keep the exact-target lookup: making them search descendants would silently retarget the component they create. A test locks that boundary in. animator_get_info additionally reports animatorGameObject. The existing gameObject field keeps naming the resolved target.
…he target The usage guide still stated the old precondition, so a user reading it would retarget a command to the model child that the resolver now handles on its own.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughAnimator tools now resolve Animators on targets or unique descendants, including inactive descendants. Ambiguous or missing targets return structured errors. Responses identify resolved objects. CLI output, documentation, and tests cover the new behavior. ChangesAnimator resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change resolves Animator lookup for child and inactive rig objects while refusing ambiguous targets and reporting the object actually affected. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLI
participant AnimatorControl
participant AnimatorResolver
participant UnityAnimator
CLI->>AnimatorControl: Send animation command
AnimatorControl->>AnimatorResolver: Resolve target Animator
AnimatorResolver->>UnityAnimator: Check target and descendants
UnityAnimator-->>AnimatorResolver: Return unique match or candidates
AnimatorResolver-->>AnimatorControl: Return Animator or structured error
AnimatorControl-->>CLI: Return operation result and resolved-object message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
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.
Description
manage_animation'sanimator_*actions resolved the Animator withGetComponent<Animator>()on the target alone. Imported models keep their Animator on themodel root, which is normally a child of the GameObject a user names, so on the most common
rig setup every read and control call failed with
No Animator component on 'X'even thoughthe Animator was right there.
The fix is not just "search children". A naive fallback trades a loud failure for a silent
wrong-target mutation — the same class of problem, made quieter. So this PR also makes an
ambiguous target an error, and makes retargeting visible in the response.
Type of Change
Changes Made
New
AnimatorResolver(MCPForUnity/Editor/Tools/Animation/AnimatorResolver.cs) —prefers an Animator on the exact target, otherwise looks at descendants, inactive ones
included because a disabled rig is still readable. The seven read/control lookups
(
AnimatorRead.GetInfo,AnimatorRead.GetParameter, andAnimatorControl.{Play, Crossfade, SetParameter, SetSpeed, SetEnabled}) all route through it.Ambiguity is reported, never guessed. Unity's descendant search is depth-first, so a
wrapper holding several rigs returns the first branch however deep — not the nearest rig,
and not what a caller would predict. I measured this on 6000.4.4f1: with Animators on
Root/A/ADeepandRoot/B, the winner isADeep. When more than one descendant carries anAnimator the call now fails and names the candidates, and mutates nothing.
Every successful response names the object that actually changed. A retargeted call used
to report success on the wrapper while a descendant was what moved. This includes the CLI's
own success line for
animator_play, which echoed the requested target and so contradictedthe result it had just printed. It matters most for
animator_set_parameter, which in Editmode writes the shared
AnimatorControllerasset, so the caller needs to see which Animatoranswered. Where the tool returns no message at all, the CLI fallback now names no object
rather than guessing the target.
animator_get_infoadditionally reportsanimatorGameObject. The existinggameObjectfield keeps naming the resolved target, so nothing that reads it changes meaning.
controller_assigndeliberately keeps the exact-target lookup. It ADDS an Animator whennone is found; making it search descendants would silently retarget the component it creates.
A test locks that boundary in.
The CLI usage guide still stated the old precondition, so it is updated too.
Compatibility / Package Source
file:(TestProjects/UnityMCPTests→file:../../../MCPForUnity)Packages/packages-lock.json: n/a (localfile:source)Testing/Screenshots/Recordings
cd Server && uv run pytest tests/ -v) — 1376 passed, 3 skippedNew tests:
AnimatorResolverTests.cs(new)ManageAnimationTests.cs[TestCase]set covering all seven entry points for ambiguity refusal (asserting no rig is mutated), plus child resolution, inactive-child resolution, control parity, response attribution forset_speed/set_enabled/Edit-modeset_parameter, and thecontroller_assignboundarytest_manage_animation.pyI checked that these tests can actually fail, by mutating the resolver three ways and
re-running the animation suite each time:
AllRefusecases and both ambiguity testsset_speed,set_enabled, Edit-modeset_parameterReverting the CLI change turns both new Python tests red.
Two of the 17 animation test cases survived every mutation, both on purpose. The
controller_assigntest guards behaviour this PR deliberately does not change, and is thereto catch a future edit that pulls it across the boundary.
AnimatorGetInfo_TargetAndDescendants HaveAnimators_UsesTheTargetpins exact-target precedence, which none of the three mutationsbreaks. I am not counting either as proof of this change.
Worth noting for whoever reads the assertions: the ambiguity cases assert both candidate names
appear in the message, not merely that the call failed.
animator_get_parameterandanimator_set_parameterstill fail under the silent-match mutation — on a later parametercheck — so a bare
success == falseassertion would have passed there.On the 66 skipped: that number is unchanged from before this branch, and none of them are
animation tests. They are
[Explicit]process/port tests (38), missingrender-pipeline packages (18), domain-reload stress tests meant to be run by hand (4), and
tests with side effects such as writing real client configs or stopping the server (6).
Documentation Updates
No tool, action, or parameter was added, and
uv run python ../tools/generate_docs_reference.py --checkreports the generated reference isup to date. The one documentation change is narrative:
Server/src/cli/CLI_USAGE_GUIDE.mdsaid the target must carry the Animator.
Related Issues
None found — I searched the open issues and PRs and did not find this reported.
Additional Notes
On the Unity version. I could only test locally on 6000.4.4f1, which is not in
tools/unity-versions.json; none of the four matrix versions are installed on this machine.tools/check-unity-versions.shskips all four and still exits 0, so please don't read a localgreen there as coverage — CI is the real check for the matrix. The change uses no
version-conditional code and no
#if UNITY_*blocks.Two things I found and deliberately did not change. Both are pre-existing and neither is
made worse per call by this PR, but this PR does make the first reachable from more targets,
so they seem worth your judgement rather than my silence:
ClipCreate.Assignalso looks upGetComponent<Animator>(), and I left it on theexact-target side because it can add a component. Looking closer, that check is really a
branch selector: with an Animator it returns non-mutating Mecanim guidance; without one
it adds a legacy
Animationcomponent and converts the clip asset to legacy. So afterthis PR,
clip_assignonWrappertakes the legacy branch whileanimator_get_infoonthe same
Wrapperreports the child's Animator — the two disagree about whetherWrapperhas an Animator. I did not change it because putting a rig under a wrapper and deliberately
animating the wrapper with a legacy clip looks like a legitimate setup the resolver would
break. Happy to follow up either way.
Edit-mode
animator_set_parameterwrites the default onto the sharedAnimatorControllerasset, which affects every Animator using that controller, and the response does not say
so. This is unchanged from
beta— I checked the same lines are there — and a directtarget has always had the same effect, so this PR only broadens which target names reach
it. Disclosing the controller in that message would be a small, separate improvement.
Unrelated, noticed while checking the CLI's success lines.
clip create,clip create-presetandcontroller createprint a secondary success line using the caller'sraw path, while the tool sanitizes it and appends
.anim/.controller. Soanimation clip create Assets/WalkcreatesAssets/Walk.animand then printsCreated clip at Assets/Walk. Untouched here — mentioning it in case it is useful.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests