[feature] Animation and AnimController#696
[feature] Animation and AnimController#696Scriptwonder wants to merge 5 commits intoCoplayDev:betafrom
Conversation
📝 WalkthroughWalkthroughAdds a comprehensive animation management feature: Unity Editor C# tools for Animator/AnimationClip/AnimatorController operations, a ManageAnimation dispatcher, Python CLI and server tooling to invoke it, extensive Unity and Python tests, plus manifest and docs updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "Python CLI"
participant Server as "Server/Service"
participant Dispatcher as "ManageAnimation (C# dispatcher)"
participant Tools as "Animator/Clip/Controller Tools"
participant Unity as "UnityEditor / AssetDatabase"
User->>CLI: animation animator play --target Cube --state-name Walk
CLI->>Server: manage_animation(action="animator_play", target="Cube", ...)
Server->>Dispatcher: HandleCommand({action:"animator_play", target:"Cube", ...})
Dispatcher->>Dispatcher: NormalizeParams (snake_case → camelCase, aliases)
Dispatcher->>Tools: AnimatorControl.Play({target:"Cube", stateName:"Walk"})
Tools->>Unity: Resolve object (ObjectResolver) & find Animator
alt Animator found
Tools->>Unity: Undo.RecordObject(animator) and animator.Play("Walk")
Tools->>Unity: EditorUtility.SetDirty / AssetDatabase.SaveAssets
Tools-->>Dispatcher: {success:true, ...}
else Animator not found
Tools-->>Dispatcher: {success:false, error:"Animator not found"}
end
Dispatcher-->>Server: result
Server-->>CLI: formatted result
CLI-->>User: display success / error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Reviewer's GuideAdds a full animation management stack: a new manage_animation MCP tool, Unity editor-side handlers for Animator/AnimationClip/AnimatorController operations (including presets, blend trees, layers, and events), a rich animation CLI command group, and comprehensive tests and docs wiring. Sequence diagram for creating an AnimationClip preset via CLI and manage_animationsequenceDiagram
actor UserDev
participant CLI as animation_CLI
participant PyTool as manage_animation_tool
participant Transport as Unity_transport
participant CsTool as ManageAnimation_HandleCommand
participant ClipPresets as ClipPresets_CreatePreset
UserDev->>CLI: unity-mcp animation clip create-preset "Bounce.anim" bounce
CLI->>PyTool: run_command("manage_animation", action=clip_create_preset,...)
PyTool->>Transport: send_with_unity_instance("manage_animation", params_dict)
Transport->>CsTool: manage_animation + params
CsTool->>CsTool: NormalizeParams
CsTool->>ClipPresets: HandleClipAction("create_preset")
ClipPresets->>ClipPresets: Create AnimationClip with preset curves
ClipPresets->>CsTool: { success, data, message }
CsTool->>Transport: result JSON
Transport->>PyTool: result JSON
PyTool->>CLI: result dict
CLI->>UserDev: formatted output + print_success
Class diagram for ManageAnimation and animation handler classesclassDiagram
class ManageAnimation {
<<static>>
-ParamAliases : Dictionary~string,string~
+HandleCommand(params : JObject) object
-NormalizeParams(source : JObject) JObject
-ExtractProperties(source : JObject) JObject
-NormalizeKey(key : string, allowAliases : bool) string
-NormalizeToken(token : JToken) JToken
-HandleAnimatorAction(params : JObject, action : string) object
-HandleControllerAction(params : JObject, action : string) object
-HandleClipAction(params : JObject, action : string) object
}
class AnimatorControl {
<<static>>
+Play(params : JObject) object
+Crossfade(params : JObject) object
+SetParameter(params : JObject) object
+SetSpeed(params : JObject) object
+SetEnabled(params : JObject) object
}
class AnimatorRead {
<<static>>
+GetInfo(params : JObject) object
+GetParameter(params : JObject) object
}
class ClipCreate {
<<static>>
+Create(params : JObject) object
+GetInfo(params : JObject) object
+AddCurve(params : JObject) object
+SetCurve(params : JObject) object
+SetVectorCurve(params : JObject) object
+Assign(params : JObject) object
+AddEvent(params : JObject) object
+RemoveEvent(params : JObject) object
-SetOrAddCurve(params : JObject, append : bool) object
-SetupLegacyClip(clip : AnimationClip) void
-ParseKeyframes(keysToken : JToken) Keyframe[]
-ResolveType(typeName : string) Type
-CreateFoldersRecursive(folderPath : string) void
}
class ClipPresets {
<<static>>
-ValidPresets : string[]
+CreatePreset(params : JObject) object
-ApplyBounce(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyRotate(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyPulse(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyFade(clip : AnimationClip, duration : float) void
-ApplyShake(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyHover(clip : AnimationClip, duration : float, amplitude : float) void
-ApplySpin(clip : AnimationClip, duration : float, amplitude : float) void
-ApplySway(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyBob(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyWiggle(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyBlink(clip : AnimationClip, duration : float) void
-ApplySlideIn(clip : AnimationClip, duration : float, amplitude : float) void
-ApplyElastic(clip : AnimationClip, duration : float, amplitude : float) void
-CreateFoldersRecursive(folderPath : string) void
}
class ControllerCreate {
<<static>>
+Create(params : JObject) object
+AddState(params : JObject) object
+AddTransition(params : JObject) object
+AddParameter(params : JObject) object
+GetInfo(params : JObject) object
+AssignToGameObject(params : JObject) object
-LoadController(params : JObject) AnimatorController
-ControllerNotFoundError(params : JObject) object
-CreateFoldersRecursive(folderPath : string) void
}
class ControllerLayers {
<<static>>
+AddLayer(params : JObject) object
+RemoveLayer(params : JObject) object
+SetLayerWeight(params : JObject) object
}
class ControllerBlendTrees {
<<static>>
+CreateBlendTree1D(params : JObject) object
+CreateBlendTree2D(params : JObject) object
+AddBlendTreeChild(params : JObject) object
}
class ObjectResolver {
<<static>>
+ResolveGameObject(target : JToken, searchMethod : string) GameObject
}
class AssetPathUtility {
<<static>>
+SanitizeAssetPath(path : string) string
}
class StringCaseUtility {
<<static>>
+ToCamelCase(value : string) string
}
ManageAnimation --> AnimatorControl : uses
ManageAnimation --> AnimatorRead : uses
ManageAnimation --> ClipCreate : uses
ManageAnimation --> ClipPresets : uses
ManageAnimation --> ControllerCreate : uses
ManageAnimation --> ControllerLayers : uses
ManageAnimation --> ControllerBlendTrees : uses
AnimatorControl --> ObjectResolver : resolves GameObject
AnimatorRead --> ObjectResolver : resolves GameObject
ClipCreate --> ObjectResolver : resolves GameObject
ClipCreate --> AssetPathUtility : sanitizes paths
ClipPresets --> AssetPathUtility : sanitizes paths
ControllerCreate --> AssetPathUtility : sanitizes paths
ControllerLayers --> AssetPathUtility : sanitizes paths
ControllerBlendTrees --> AssetPathUtility : sanitizes paths
ManageAnimation --> StringCaseUtility : normalizes keys
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The various animation helpers (e.g.
ClipCreate,ClipPresets,ControllerCreate) all reimplement folder-creation and asset-path handling; consider extractingCreateFoldersRecursive/path sanitization into a shared utility to avoid duplication and keep behavior consistent. - In
ClipPresetsthe preset writers useclip.SetCurvewhileClipCreateexplicitly switched toAnimationUtility.SetEditorCurveto avoid marking clips as legacy; it would be good to standardize on one approach (likelySetEditorCurve) to prevent subtle differences in how preset clips behave with Mecanim/BlendTrees.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The various animation helpers (e.g. `ClipCreate`, `ClipPresets`, `ControllerCreate`) all reimplement folder-creation and asset-path handling; consider extracting `CreateFoldersRecursive`/path sanitization into a shared utility to avoid duplication and keep behavior consistent.
- In `ClipPresets` the preset writers use `clip.SetCurve` while `ClipCreate` explicitly switched to `AnimationUtility.SetEditorCurve` to avoid marking clips as legacy; it would be good to standardize on one approach (likely `SetEditorCurve`) to prevent subtle differences in how preset clips behave with Mecanim/BlendTrees.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/Animation/ClipPresets.cs:128` </location>
<code_context>
+ new Keyframe(half + half * 0.5f, amplitude),
+ new Keyframe(duration, 0f)
+ );
+ clip.SetCurve("", typeof(Transform), "localPosition.y", curve);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using clip.SetCurve here can mark clips as legacy and conflict with the non-legacy behavior used elsewhere.
In `ClipCreate.SetOrAddCurve/SetVectorCurve` you switched to `AnimationUtility.SetEditorCurve` specifically to keep clips non-legacy for Mecanim BlendTrees, but here (e.g., in `ApplyBounce`, `ApplyPulse`, etc.) you still use `clip.SetCurve`, which can set the legacy flag on preset-generated clips.
To avoid reintroducing that issue and to keep behavior consistent, use `AnimationUtility.SetEditorCurve` here as well, reusing the same binding construction pattern from `ClipCreate`.
</issue_to_address>
### Comment 2
<location> `MCPForUnity/Editor/Tools/Animation/ManageAnimation.cs:191-193` </location>
<code_context>
+
+ return new { success = false, message = $"Unknown action: {action}. Actions must be prefixed with: animator_, controller_, or clip_" };
+ }
+ catch (Exception ex)
+ {
+ return new { success = false, message = ex.Message, stackTrace = ex.StackTrace };
+ }
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Returning raw stack traces in tool responses can leak internal details and is noisy for normal usage.
The catch block in `HandleCommand` returns `ex.Message` and `ex.StackTrace` directly, which can expose internal paths and implementation details to tool callers and end users, and makes errors unnecessarily noisy. Consider returning a sanitized error message (possibly with a short error code/type) while logging the full exception and stack trace to the Unity console instead.
Suggested implementation:
```csharp
catch (Exception ex)
{
// Log full exception details to the Unity console for debugging
Debug.LogError($"[ManageAnimation] Error handling command '{action}': {ex}");
// Return a sanitized error payload to callers
return new
{
success = false,
message = "An unexpected error occurred while handling the animation command.",
errorType = ex.GetType().Name
};
}
```
If `Debug` is not already available in this file, ensure you have `using UnityEngine;` at the top of the file so `Debug.LogError` resolves correctly.
</issue_to_address>
### Comment 3
<location> `TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAnimationTests.cs:47-56` </location>
<code_context>
+ Assert.IsTrue(settings.loopTime, "Should be looping");
+ }
+
+ [Test]
+ public void ClipCreatePreset_AllPresetsCreateSuccessfully()
+ {
+ string[] presets = { "bounce", "rotate", "pulse", "fade", "shake", "hover", "spin" };
</code_context>
<issue_to_address>
**suggestion (testing):** Extend preset-creation tests to cover all supported preset names
`ClipPresets.ValidPresets` includes additional presets (sway, bob, wiggle, blink, slide_in, elastic, etc.) that aren’t covered here. To keep tests in sync with the implementation, consider deriving the list from `ValidPresets` or explicitly enumerating all of its values so every preset is verified to parse and create a valid `AnimationClip`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| [Test] | ||
| public void HandleCommand_MissingAction_ReturnsError() | ||
| { | ||
| var paramsObj = new JObject(); | ||
| var result = ToJObject(ManageAnimation.HandleCommand(paramsObj)); | ||
| Assert.IsFalse(result.Value<bool>("success")); | ||
| Assert.That(result["message"].ToString(), Does.Contain("Action is required")); | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
suggestion (testing): Extend preset-creation tests to cover all supported preset names
ClipPresets.ValidPresets includes additional presets (sway, bob, wiggle, blink, slide_in, elastic, etc.) that aren’t covered here. To keep tests in sync with the implementation, consider deriving the list from ValidPresets or explicitly enumerating all of its values so every preset is verified to parse and create a valid AnimationClip.
There was a problem hiding this comment.
Pull request overview
Adds first-class Unity animation support to Unity MCP by introducing a new manage_animation tool end-to-end (Unity Editor implementation + server tool wrapper + CLI surface + tests) and updating manifests/docs accordingly.
Changes:
- Introduces
manage_animationtool with Animator control, AnimationClip creation/editing, and AnimatorController CRUD (including layers, transitions, blend trees). - Adds a comprehensive Unity EditMode test suite for
ManageAnimationplus Python-side tests for the server tool + CLI parameter building. - Updates
manifest.jsonand README docs to include the new tool (and aligns tool/resource lists).
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| manifest.json | Registers manage_animation tool and updates tool metadata/description. |
| README.md | Updates documented tool/resource lists to include manage_animation (and other existing tools/resources). |
| docs/i18n/README-zh.md | Same doc list updates for Chinese README. |
| Server/uv.lock | Bumps server package version in lockfile to 9.4.0. |
| Server/src/services/tools/manage_animation.py | Adds server-side MCP tool wrapper/validation for manage_animation. |
| Server/src/cli/commands/animation.py | Adds full CLI command tree for animator/clip/controller + raw escape hatch. |
| Server/tests/test_manage_animation.py | Adds Python tests for action lists, tool validation, and CLI param building. |
| MCPForUnity/Editor/Tools/Animation.meta | Unity folder meta for new Animation tools folder. |
| MCPForUnity/Editor/Tools/Animation/ManageAnimation.cs | Unity-side command dispatcher + param normalization/aliasing for animation actions. |
| MCPForUnity/Editor/Tools/Animation/ManageAnimation.cs.meta | Unity meta for ManageAnimation.cs. |
| MCPForUnity/Editor/Tools/Animation/AnimatorRead.cs | Implements animator info + parameter querying. |
| MCPForUnity/Editor/Tools/Animation/AnimatorRead.cs.meta | Unity meta for AnimatorRead.cs. |
| MCPForUnity/Editor/Tools/Animation/AnimatorControl.cs | Implements animator play/crossfade/set parameter/speed/enabled. |
| MCPForUnity/Editor/Tools/Animation/AnimatorControl.cs.meta | Unity meta for AnimatorControl.cs. |
| MCPForUnity/Editor/Tools/Animation/ClipCreate.cs | Implements clip create/info/curve edits/vector curves/assign/events. |
| MCPForUnity/Editor/Tools/Animation/ClipCreate.cs.meta | Unity meta for ClipCreate.cs. |
| MCPForUnity/Editor/Tools/Animation/ClipPresets.cs | Implements preset-based clip generation (bounce/rotate/etc). |
| MCPForUnity/Editor/Tools/Animation/ClipPresets.cs.meta | Unity meta for ClipPresets.cs. |
| MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs | Implements controller create/state/transition/parameter/info/assign. |
| MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs.meta | Unity meta for ControllerCreate.cs. |
| MCPForUnity/Editor/Tools/Animation/ControllerLayers.cs | Implements controller layer add/remove/weight operations. |
| MCPForUnity/Editor/Tools/Animation/ControllerLayers.cs.meta | Unity meta for ControllerLayers.cs. |
| MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs | Implements blend tree creation + child insertion. |
| MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs.meta | Unity meta for ControllerBlendTrees.cs. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAnimationTests.cs | Adds Unity EditMode tests covering dispatch, clips, controllers, presets, params normalization. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAnimationTests.cs.meta | Unity meta for ManageAnimationTests.cs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| curve.keys[0].outTangent = 360f * amplitude / duration; | ||
| var keys = curve.keys; | ||
| keys[1].inTangent = 360f * amplitude / duration; | ||
| curve.keys = keys; |
There was a problem hiding this comment.
Setting tangents via curve.keys[0].outTangent = ... won’t persist because AnimationCurve.keys returns a copy of the keyframe array. As written, only the second key’s tangent is reliably applied. Update tangents by modifying the keys array (both indices) and assigning it back to curve.keys. Also guard against duration == 0 to avoid division by zero when computing linear tangents.
| curve.keys[0].outTangent = 360f * amplitude / duration; | |
| var keys = curve.keys; | |
| keys[1].inTangent = 360f * amplitude / duration; | |
| curve.keys = keys; | |
| if (duration != 0f) | |
| { | |
| float slope = 360f * amplitude / duration; | |
| var keys = curve.keys; | |
| if (keys.Length >= 2) | |
| { | |
| keys[0].outTangent = slope; | |
| keys[1].inTangent = slope; | |
| curve.keys = keys; | |
| } | |
| } |
| value = animator.GetBool(paramName); | ||
| break; | ||
| case AnimatorControllerParameterType.Trigger: | ||
| value = animator.GetBool(paramName); |
There was a problem hiding this comment.
Trigger parameters can’t be read via Animator.GetBool; Unity doesn’t provide a getter for trigger state. Returning GetBool here is misleading and may be incorrect. Consider returning value = null (or omitting it) for triggers, or returning an explicit message that trigger values aren’t queryable at runtime.
| value = animator.GetBool(paramName); | |
| // Trigger parameters cannot be read at runtime; Unity does not provide a getter. | |
| value = null; |
| return new { success = false, message = $"Layer index {layerIndex} out of range (0-{layers.Length - 1})" }; | ||
|
|
||
| var stateMachine = layers[layerIndex].stateMachine; | ||
|
|
There was a problem hiding this comment.
CreateBlendTree1D adds a new state unconditionally via stateMachine.AddState(stateName) without checking for an existing state with the same name. This can silently create duplicate states, which is inconsistent with ControllerCreate.AddState (which rejects duplicates) and makes controllers harder to reason about. Add a duplicate-name check and return an error (or reuse the existing state) before creating the new state.
| // Prevent creating duplicate states with the same name in this layer | |
| var existingState = Array.Find(stateMachine.states, s => s.state != null && s.state.name == stateName); | |
| if (existingState.state != null) | |
| return new { success = false, message = $"State '{stateName}' already exists in layer {layerIndex}" }; |
| var state = rootStateMachine.AddState(stateName); | ||
|
|
||
| // Optionally assign a clip | ||
| string clipPath = @params["clipPath"]?.ToString(); | ||
| if (!string.IsNullOrEmpty(clipPath)) | ||
| { | ||
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | ||
| if (clipPath != null) | ||
| { | ||
| var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | ||
| if (clip != null) | ||
| state.motion = clip; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
When clipPath is provided, invalid paths or missing clips are silently ignored and the state is still created successfully (with hasMotion = false). Since the caller explicitly requested a motion assignment, it would be clearer to return an error if clipPath is invalid or the clip can’t be loaded.
| var state = rootStateMachine.AddState(stateName); | |
| // Optionally assign a clip | |
| string clipPath = @params["clipPath"]?.ToString(); | |
| if (!string.IsNullOrEmpty(clipPath)) | |
| { | |
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | |
| if (clipPath != null) | |
| { | |
| var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | |
| if (clip != null) | |
| state.motion = clip; | |
| } | |
| } | |
| // Validate optional clipPath before creating the state | |
| string clipPath = @params["clipPath"]?.ToString(); | |
| AnimationClip clip = null; | |
| if (!string.IsNullOrEmpty(clipPath)) | |
| { | |
| string originalClipPath = clipPath; | |
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | |
| if (clipPath == null) | |
| { | |
| return new { success = false, message = $"Invalid clip asset path '{originalClipPath}'" }; | |
| } | |
| clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | |
| if (clip == null) | |
| { | |
| return new { success = false, message = $"AnimationClip not found at '{clipPath}'" }; | |
| } | |
| } | |
| var state = rootStateMachine.AddState(stateName); | |
| // Assign validated clip if provided | |
| if (clip != null) | |
| { | |
| state.motion = clip; | |
| } |
| @animator.command("set-enabled") | ||
| @click.argument("target") | ||
| @click.argument("enabled", type=bool) | ||
| @click.option("--search-method", type=SEARCH_METHOD_CHOICE_BASIC, default=None) |
There was a problem hiding this comment.
enabled is declared as a Click argument with type=bool, which will treat any non-empty string (including "false") as True because it calls Python's bool() for conversion. This makes set-enabled ... false behave incorrectly. Use type=click.Choice(["true","false"], case_sensitive=False) (and map to a bool), or change to a flag-style option like --enable/--disable instead of a boolean positional argument.
| { | ||
| string controllerPath = $"{TempRoot}/DuplicateController.controller"; | ||
|
|
||
| var controller = AnimatorController.CreateAnimatorControllerAtPath(controllerPath); |
There was a problem hiding this comment.
This assignment to controller is useless, since its value is never read.
| var controller = AnimatorController.CreateAnimatorControllerAtPath(controllerPath); | |
| AnimatorController.CreateAnimatorControllerAtPath(controllerPath); |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAnimationTests.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@manifest.json`:
- Around line 100-103: The manifest currently adds a tool entry named
"manage_texture" which is unrelated to this "Animation First" PR; either remove
the "manage_texture" entry from the manifest (the JSON object with "name":
"manage_texture" and its "description") so the PR only contains animation
changes, or update the PR title/description to explicitly include procedural
texture management and rename the tool or description to reflect shared scope;
locate the manifest JSON object with "name": "manage_texture" and either delete
it or update the PR metadata and the "description" field to match the broadened
scope.
In `@MCPForUnity/Editor/Tools/Animation/AnimatorControl.cs`:
- Around line 90-127: The SetParameter handling in AnimatorControl.cs silently
uses defaults when valueToken is null; update the SetParameter logic in both
Play and Edit branches (the switch handling in the block guarded by
Application.isPlaying and the corresponding edit-mode branch) to validate that
valueToken is present for non-"trigger" paramType and return a failure object
like { success = false, message = "Missing 'value' for parameter '<paramName>'"
} instead of applying 0/false; adjust the float/int/bool cases in the switch
(references: animator.SetFloat, animator.SetInteger, animator.SetBool and the
paramType switch) to check valueToken != null prior to ToObject conversion and
only proceed when present.
In `@MCPForUnity/Editor/Tools/Animation/ClipCreate.cs`:
- Around line 346-387: The clip is being converted to legacy by SetupLegacyClip
which mutates the shared AnimationClip; detect that change and append a
user-visible warning to the returned message. Concretely, before each call to
SetupLegacyClip (the branch that checks GetComponent<UnityEngine.Animation>()
and the branch that adds the Animation component via
Undo.AddComponent<UnityEngine.Animation>()), capture the original legacy state
(e.g. var wasLegacy = clip.legacy), then call SetupLegacyClip(clip) and if
wasLegacy == false append a concise warning like " Warning: clip was converted
to legacy and will not be usable in Mecanim/BlendTrees." to the message returned
by those anonymous result objects so the user is informed of the side effect.
In `@MCPForUnity/Editor/Tools/Animation/ClipPresets.cs`:
- Around line 131-144: ApplyRotate currently writes to curve.keys[0].outTangent
directly which is a no-op because curve.keys returns a copy; fix by grabbing a
local Keyframe[] keys = curve.keys, set keys[0].outTangent and keys[1].inTangent
to 360f * amplitude / duration, then reassign curve.keys = keys before calling
clip.SetCurve (keep the existing curve creation and property names in
ApplyRotate).
In `@MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs`:
- Around line 97-104: The switch on blendTypeStr currently maps unknown values
silently to BlendTreeType.SimpleDirectional2D; update the logic in
ControllerBlendTrees (the blendTypeStr/BlendTreeType switch) to detect unmapped
inputs and return or throw an explicit error instead of defaulting—e.g., after
computing blendTypeStr, perform a validation step that checks if the switch
matched (or use a non-default branch) and if not return a failure result or
throw with a message like "Unknown blendType 'X'. Valid: simpledirectional2d,
freeformdirectional2d, freeformcartesian2d" so callers can handle invalid input
rather than masking it.
- Around line 167-170: After calling
AssetPathUtility.SanitizeAssetPath(clipPath) in the block around
AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath), add a null guard for
clipPath and return the same anonymous failure object with a clear message (e.g.
"Invalid or sanitized asset path") if clipPath is null; this prevents calling
LoadAssetAtPath with a null/empty path and avoids the confusing "AnimationClip
not found at ''" message. Ensure the check sits immediately after the
SanitizeAssetPath call and before LoadAssetAtPath, referencing clipPath,
AssetPathUtility.SanitizeAssetPath, and the existing return payload used in this
method.
In `@MCPForUnity/Editor/Tools/Animation/ManageAnimation.cs`:
- Around line 190-194: The catch block in ManageAnimation.cs currently returns
ex.StackTrace to clients; remove the stackTrace from the returned anonymous
object and only return success = false and message = ex.Message (or a sanitized
message). Instead, log the full exception/stack trace server-side using
UnityEngine.Debug.LogException(ex) or your existing logger inside the catch
(Exception ex) block so internal details are recorded but not exposed to MCP
clients; update the return value from the catch to omit stackTrace.
In `@Server/tests/test_manage_animation.py`:
- Around line 525-536: The test test_clip_create_preset_all_presets_accepted
currently patches cli.commands.animation.run_command as mock_run but never uses
it (Ruff F841); either remove the unused mock or assert the expected call
arguments to strengthen the test—when keeping the patch, use
mock_run.assert_called_once_with(...) or assert mock_run.call_args_list to
verify the parameters built by runner.invoke(animation, ["clip",
"create-preset", f"Assets/Anim/{preset}.anim", preset]) for each preset,
otherwise remove the with patch("cli.commands.animation.run_command",
return_value=mock_success) as mock_run block and just patch
return_value=mock_success without assigning to mock_run.
- Around line 347-459: The manage_animation tool function signature
(manage_animation) is missing a controller_path parameter so controllerPath gets
dropped; update the function signature to accept controller_path, add it to the
construction of params_dict (alongside action, target, search_method, clip_path,
properties), and map it to the top-level key "controllerPath" (consistent with
_TOP_LEVEL_KEYS and existing controller_* commands) so the CLI passes
controllerPath through to the tool.
🧹 Nitpick comments (12)
Server/tests/test_manage_animation.py (2)
97-148: Tests useasyncio.run()to invoke the async tool — considerpytest-asynciofor consistency.Each validation test re-imports
manage_animationinside the method body and usesasyncio.run(). This works but is somewhat unconventional for pytest. If the project already usespytest-asyncio, marking these asasyncwith@pytest.mark.asynciowould be more idiomatic. Also, the repeated local import could be hoisted to the class or module level.
159-255: CLI tests don't assertrunner.invokeexit code before checking mock params.If a CLI command fails (e.g., due to a missing argument or Click error),
runner.invokereturns a non-zero exit code butmock_runmay never be called, leading to confusingAssertionErroronmock_run.assert_called_once()rather than surfacing the actual CLI error. Addingassert result.exit_code == 0, result.outputas a guard (liketest_clip_create_preset_all_presets_accepteddoes) would make failures much easier to diagnose.MCPForUnity/Editor/Tools/Animation/AnimatorRead.cs (1)
11-90: Consider usingToolParamsfor parameter validation consistency.This file (and the other new C# tool files in this PR) accesses
JObjectparameters directly with@params["target"]?.ToString(),@params["searchMethod"]?.ToString(), etc. The project convention documented inCLAUDE.mdrecommends usingToolParamsfor consistent parameter validation in C# Editor tools (with helpers likeRequireString(),GetInt(), etc.). While the manual approach works, adoptingToolParamswould reduce boilerplate and align with the rest of the codebase.This applies across all new C# files in this PR (AnimatorRead, AnimatorControl, ClipPresets, ControllerBlendTrees, ControllerLayers, ManageAnimation).
Based on learnings: "Use
ToolParamsfor consistent parameter validation in C# Editor tools with methods likeGetInt(),RequireString()" (from CLAUDE.md).Server/src/services/tools/manage_animation.py (1)
90-100: RedundantNonefilter on Line 100.Lines 91–98 already guard each insertion with
if ... is not None, so the dictionary comprehension on Line 100 will never actually filter anything out. The"action"key set on Line 90 is always non-None.Suggested simplification
params_dict: dict[str, Any] = {"action": action_normalized} if properties is not None: params_dict["properties"] = properties if target is not None: params_dict["target"] = target if search_method is not None: params_dict["searchMethod"] = search_method if clip_path is not None: params_dict["clipPath"] = clip_path - params_dict = {k: v for k, v in params_dict.items() if v is not None} - result = await send_with_unity_instance(MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs (1)
12-24: Controller loading boilerplate is duplicated across all three methods.
ControllerCreate.csalready has aLoadControllerhelper (Lines 424–435) that performs the samecontrollerPathextraction, sanitization, andAssetDatabase.LoadAssetAtPathcall. Consider reusing it (or extracting a shared helper) to reduce duplication. The same pattern also appears inControllerLayers.cs.Also applies to: 73-85, 145-157
MCPForUnity/Editor/Tools/Animation/ControllerLayers.cs (1)
64-131: Duplicated layer-resolution logic betweenRemoveLayerandSetLayerWeight.Both methods contain nearly identical code for resolving a layer by
layerIndexorlayerName(Lines 78–112 and 147–177). Consider extracting a small helper likeResolveLayerIndex(AnimatorController controller, JObject@params)that returns a(int index, string name, string error)tuple to DRY this up.Also applies to: 133-200
MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs (2)
192-205: Unknown condition mode silently defaults toGreater.If a caller provides an unrecognized
modestring (e.g., a typo like"greter"), it silently falls back toAnimatorConditionMode.Greaterwith no warning. This could lead to hard-to-debug behavior. Consider returning an error or at least logging a warning for unrecognized modes.Proposed fix
- default: mode = AnimatorConditionMode.Greater; break; + default: + return new { success = false, message = $"Unknown condition mode '{modeStr}'. Valid: greater, less, equals, notequal/not_equal, if/true, ifnot/if_not/false" };
444-456: DuplicateCreateFoldersRecursiveacross multiple files.This exact helper is duplicated in
ClipCreate.cs(line 614) andClipPresets.cs(line 317). Consider extracting it to a shared utility (e.g.,AssetPathUtility) to reduce duplication.MCPForUnity/Editor/Tools/Animation/ClipCreate.cs (1)
470-500: Assembly scan fallback iterates all loaded assemblies.The
ResolveTypefallback (lines 490-497) searches every loaded assembly. This is fine for editor tooling called occasionally, but if this gets called in a tight loop (e.g., per-curve in a batch operation), it could become a performance bottleneck. For now this is acceptable — just noting for awareness.Server/src/cli/commands/animation.py (1)
895-933:animation_rawis a good escape hatch — consider documenting thepropertiesflattening behavior.Users passing
--paramsJSON may include keys that overlap with_TOP_LEVEL_KEYS(e.g.,"target"inside the JSON). At line 931,request_params.update(parsed)would overwrite the explicitly-providedtargetargument if the JSON also contains"target". This could be surprising.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageAnimationTests.cs (2)
1003-1022: Incomplete preset coverage — only 7 of 13 presets tested.
ClipCreatePreset_AllPresetsCreateSuccessfullytestsbounce, rotate, pulse, fade, shake, hover, spinbut the CLI (and presumably the C# side) supports 6 more:sway, bob, wiggle, blink, slide_in, elastic. Consider adding them to the array for full coverage.Proposed fix
- string[] presets = { "bounce", "rotate", "pulse", "fade", "shake", "hover", "spin" }; + string[] presets = { "bounce", "rotate", "pulse", "fade", "shake", "hover", "spin", "sway", "bob", "wiggle", "blink", "slide_in", "elastic" };
896-974: No test coverage forclip_add_eventandclip_remove_eventactions.
ClipCreate.AddEventandClipCreate.RemoveEventare part of the public API and exposed via CLI, but this test suite has no tests for them. Consider adding at least basic positive and negative path tests (add an event, verify it exists, remove by index, remove by function name).Would you like me to generate test methods for
clip_add_eventandclip_remove_event, or open a new issue to track this?
| JToken valueToken = @params["value"]; | ||
|
|
||
| // In Edit mode, runtime Animator.SetFloat/SetInteger/SetBool are no-ops because | ||
| // the Animator graph isn't active. Instead, modify the controller asset's default | ||
| // parameter values so changes actually persist. | ||
| bool isPlaying = Application.isPlaying; | ||
|
|
||
| if (isPlaying) | ||
| { | ||
| Undo.RecordObject(animator, $"Set Animator Parameter {paramName}"); | ||
|
|
||
| switch (paramType) | ||
| { | ||
| case "float": | ||
| float fVal = valueToken?.ToObject<float>() ?? 0f; | ||
| animator.SetFloat(paramName, fVal); | ||
| return new { success = true, message = $"Set float '{paramName}' = {fVal}" }; | ||
|
|
||
| case "int": | ||
| case "integer": | ||
| int iVal = valueToken?.ToObject<int>() ?? 0; | ||
| animator.SetInteger(paramName, iVal); | ||
| return new { success = true, message = $"Set int '{paramName}' = {iVal}" }; | ||
|
|
||
| case "bool": | ||
| case "boolean": | ||
| bool bVal = valueToken?.ToObject<bool>() ?? false; | ||
| animator.SetBool(paramName, bVal); | ||
| return new { success = true, message = $"Set bool '{paramName}' = {bVal}" }; | ||
|
|
||
| case "trigger": | ||
| animator.SetTrigger(paramName); | ||
| return new { success = true, message = $"Set trigger '{paramName}'" }; | ||
|
|
||
| default: | ||
| return new { success = false, message = $"Unknown parameter type: {paramType}. Valid: float, int, bool, trigger" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
SetParameter silently defaults to zero/false when value is missing.
On Lines 104, 110, and 116 (and the corresponding edit-mode lines 154, 163, 172), if valueToken is null, the parameter is set to 0f, 0, or false respectively — with no indication to the caller that their value was missing. This could cause unintentional parameter resets. Consider requiring value for non-trigger types and returning an error when it's absent.
Example guard for the Play mode float case
case "float":
- float fVal = valueToken?.ToObject<float>() ?? 0f;
+ if (valueToken == null)
+ return new { success = false, message = "'value' is required for float parameters" };
+ float fVal = valueToken.ToObject<float>();
animator.SetFloat(paramName, fVal);🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/AnimatorControl.cs` around lines 90 - 127,
The SetParameter handling in AnimatorControl.cs silently uses defaults when
valueToken is null; update the SetParameter logic in both Play and Edit branches
(the switch handling in the block guarded by Application.isPlaying and the
corresponding edit-mode branch) to validate that valueToken is present for
non-"trigger" paramType and return a failure object like { success = false,
message = "Missing 'value' for parameter '<paramName>'" } instead of applying
0/false; adjust the float/int/bool cases in the switch (references:
animator.SetFloat, animator.SetInteger, animator.SetBool and the paramType
switch) to check valueToken != null prior to ToObject conversion and only
proceed when present.
| string blendTypeStr = @params["blendType"]?.ToString()?.ToLowerInvariant() ?? "simpledirectional2d"; | ||
|
|
||
| BlendTreeType blendType = blendTypeStr switch | ||
| { | ||
| "freeformdirectional2d" => BlendTreeType.FreeformDirectional2D, | ||
| "freeformcartesian2d" => BlendTreeType.FreeformCartesian2D, | ||
| _ => BlendTreeType.SimpleDirectional2D | ||
| }; |
There was a problem hiding this comment.
Silent fallback to SimpleDirectional2D for unrecognized blend types.
If the user passes an invalid blendType string, the switch expression silently falls through to SimpleDirectional2D (Line 103). This could mask user errors. Consider returning an error for unrecognized types instead.
Suggested fix
BlendTreeType blendType = blendTypeStr switch
{
+ "simpledirectional2d" => BlendTreeType.SimpleDirectional2D,
"freeformdirectional2d" => BlendTreeType.FreeformDirectional2D,
"freeformcartesian2d" => BlendTreeType.FreeformCartesian2D,
- _ => BlendTreeType.SimpleDirectional2D
+ _ => BlendTreeType.SimpleDirectional2D // or return error
};Alternatively, return an error on unrecognized input:
if (blendType not matched)
return new { success = false, message = $"Unknown blendType '{blendTypeStr}'. Valid: simpledirectional2d, freeformdirectional2d, freeformcartesian2d" };🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs` around lines 97 -
104, The switch on blendTypeStr currently maps unknown values silently to
BlendTreeType.SimpleDirectional2D; update the logic in ControllerBlendTrees (the
blendTypeStr/BlendTreeType switch) to detect unmapped inputs and return or throw
an explicit error instead of defaulting—e.g., after computing blendTypeStr,
perform a validation step that checks if the switch matched (or use a
non-default branch) and if not return a failure result or throw with a message
like "Unknown blendType 'X'. Valid: simpledirectional2d, freeformdirectional2d,
freeformcartesian2d" so callers can handle invalid input rather than masking it.
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | ||
| var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | ||
| if (clip == null) | ||
| return new { success = false, message = $"AnimationClip not found at '{clipPath}'" }; |
There was a problem hiding this comment.
Missing null check after SanitizeAssetPath for clipPath.
AssetPathUtility.SanitizeAssetPath returns null for paths containing ".." (path traversal). If clipPath becomes null after sanitization, LoadAssetAtPath returns null and the error message on Line 170 will confusingly say "AnimationClip not found at ''". The other two methods in this file (and throughout the PR) correctly check for null after sanitization.
🐛 Add null guard
clipPath = AssetPathUtility.SanitizeAssetPath(clipPath);
+ if (clipPath == null)
+ return new { success = false, message = "Invalid clip asset path" };
var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | |
| var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | |
| if (clip == null) | |
| return new { success = false, message = $"AnimationClip not found at '{clipPath}'" }; | |
| clipPath = AssetPathUtility.SanitizeAssetPath(clipPath); | |
| if (clipPath == null) | |
| return new { success = false, message = "Invalid clip asset path" }; | |
| var clip = AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath); | |
| if (clip == null) | |
| return new { success = false, message = $"AnimationClip not found at '{clipPath}'" }; |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/ControllerBlendTrees.cs` around lines 167
- 170, After calling AssetPathUtility.SanitizeAssetPath(clipPath) in the block
around AssetDatabase.LoadAssetAtPath<AnimationClip>(clipPath), add a null guard
for clipPath and return the same anonymous failure object with a clear message
(e.g. "Invalid or sanitized asset path") if clipPath is null; this prevents
calling LoadAssetAtPath with a null/empty path and avoids the confusing
"AnimationClip not found at ''" message. Ensure the check sits immediately after
the SanitizeAssetPath call and before LoadAssetAtPath, referencing clipPath,
AssetPathUtility.SanitizeAssetPath, and the existing return payload used in this
method.
| def test_clip_create_preset_all_presets_accepted(self, runner, mock_config, mock_success): | ||
| """Verify all preset names are accepted by the CLI.""" | ||
| presets = ["bounce", "rotate", "pulse", "fade", "shake", "hover", "spin", | ||
| "sway", "bob", "wiggle", "blink", "slide_in", "elastic"] | ||
| for preset in presets: | ||
| with patch("cli.commands.animation.get_config", return_value=mock_config): | ||
| with patch("cli.commands.animation.run_command", return_value=mock_success) as mock_run: | ||
| result = runner.invoke(animation, [ | ||
| "clip", "create-preset", f"Assets/Anim/{preset}.anim", preset, | ||
| ]) | ||
| assert result.exit_code == 0, f"Preset '{preset}' failed: {result.output}" | ||
|
|
There was a problem hiding this comment.
mock_run is assigned but never used in the loop.
The test only checks exit_code but never verifies the params built by run_command. The mock_run variable is unused (also flagged by Ruff F841). Either remove it or add a param assertion to strengthen the test.
Proposed fix (remove unused variable)
for preset in presets:
with patch("cli.commands.animation.get_config", return_value=mock_config):
- with patch("cli.commands.animation.run_command", return_value=mock_success) as mock_run:
+ with patch("cli.commands.animation.run_command", return_value=mock_success):
result = runner.invoke(animation, [
"clip", "create-preset", f"Assets/Anim/{preset}.anim", preset,
])
assert result.exit_code == 0, f"Preset '{preset}' failed: {result.output}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_clip_create_preset_all_presets_accepted(self, runner, mock_config, mock_success): | |
| """Verify all preset names are accepted by the CLI.""" | |
| presets = ["bounce", "rotate", "pulse", "fade", "shake", "hover", "spin", | |
| "sway", "bob", "wiggle", "blink", "slide_in", "elastic"] | |
| for preset in presets: | |
| with patch("cli.commands.animation.get_config", return_value=mock_config): | |
| with patch("cli.commands.animation.run_command", return_value=mock_success) as mock_run: | |
| result = runner.invoke(animation, [ | |
| "clip", "create-preset", f"Assets/Anim/{preset}.anim", preset, | |
| ]) | |
| assert result.exit_code == 0, f"Preset '{preset}' failed: {result.output}" | |
| def test_clip_create_preset_all_presets_accepted(self, runner, mock_config, mock_success): | |
| """Verify all preset names are accepted by the CLI.""" | |
| presets = ["bounce", "rotate", "pulse", "fade", "shake", "hover", "spin", | |
| "sway", "bob", "wiggle", "blink", "slide_in", "elastic"] | |
| for preset in presets: | |
| with patch("cli.commands.animation.get_config", return_value=mock_config): | |
| with patch("cli.commands.animation.run_command", return_value=mock_success): | |
| result = runner.invoke(animation, [ | |
| "clip", "create-preset", f"Assets/Anim/{preset}.anim", preset, | |
| ]) | |
| assert result.exit_code == 0, f"Preset '{preset}' failed: {result.output}" |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 531-531: Local variable mock_run is assigned to but never used
Remove assignment to unused variable mock_run
(F841)
🤖 Prompt for AI Agents
In `@Server/tests/test_manage_animation.py` around lines 525 - 536, The test
test_clip_create_preset_all_presets_accepted currently patches
cli.commands.animation.run_command as mock_run but never uses it (Ruff F841);
either remove the unused mock or assert the expected call arguments to
strengthen the test—when keeping the patch, use
mock_run.assert_called_once_with(...) or assert mock_run.call_args_list to
verify the parameters built by runner.invoke(animation, ["clip",
"create-preset", f"Assets/Anim/{preset}.anim", preset]) for each preset,
otherwise remove the with patch("cli.commands.animation.run_command",
return_value=mock_success) as mock_run block and just patch
return_value=mock_success without assigning to mock_run.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Tools/Animation/ClipCreate.cs`:
- Around line 428-467: In ParseKeyframes, currently any array item that is
neither a JArray pair nor a JObject is silently ignored; update the foreach to
detect that else case (when item is not JArray or JObject) and emit a clear
warning or throw an error so callers notice malformed input — e.g., use
UnityEngine.Debug.LogWarningFormat (or throw a JsonException) referencing the
keysToken item and index, include the offending token content and context
(function ParseKeyframes, parameter keysToken) and keep existing behavior for
valid items; ensure the warning path still returns the keyframes parsed so far
or change the method to propagate the error based on caller expectations.
- Around line 569-576: The error message when validating eventIndex is confusing
for empty event lists; modify the logic in ClipCreate.cs around the eventIndex
validation (the block referencing eventIndex and events.RemoveAt) to first check
if events.Count == 0 and return a clear failure (e.g., "No events to remove" or
similar) before doing the index bounds check, otherwise compute and display a
non-negative upper bound for the range when constructing the out-of-range
message (use events.Count - 1 only when events.Count > 0).
In `@MCPForUnity/Editor/Tools/Animation/ClipPresets.cs`:
- Around line 31-32: The code currently reads duration from params into the
local variable duration and then uses it as a divisor in multiple methods
(ApplyRotate, ApplySpin, ApplySlideIn), which will produce Infinity/NaN when
duration is zero or negative; fix by validating and normalizing duration
immediately after parsing (e.g., if duration <= 0f then set duration = 1f or
Mathf.Epsilon and optionally log/warn), so all downstream uses in ApplyRotate,
ApplySpin, ApplySlideIn (and any other Apply* methods) operate on a safe,
positive duration value.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/Animation/ClipPresets.cs (1)
354-366:CreateFoldersRecursiveis duplicated in three files.This identical method exists in
ClipPresets.cs,ClipCreate.cs, andControllerCreate.cs. Consider extracting it intoAssetPathUtilityalongside the existing path helpers.
| private static Keyframe[] ParseKeyframes(JToken keysToken) | ||
| { | ||
| if (keysToken is JArray array && array.Count > 0) | ||
| { | ||
| var keyframes = new List<Keyframe>(); | ||
|
|
||
| foreach (var item in array) | ||
| { | ||
| if (item is JArray pair && pair.Count >= 2) | ||
| { | ||
| // Shorthand: [time, value] | ||
| float time = pair[0].ToObject<float>(); | ||
| float value = pair[1].ToObject<float>(); | ||
| keyframes.Add(new Keyframe(time, value)); | ||
| } | ||
| else if (item is JObject obj) | ||
| { | ||
| // Full form: {"time":0, "value":0, "inTangent":0, "outTangent":0} | ||
| float time = obj["time"]?.ToObject<float>() ?? 0f; | ||
| float value = obj["value"]?.ToObject<float>() ?? 0f; | ||
|
|
||
| var kf = new Keyframe(time, value); | ||
| if (obj["inTangent"] != null) | ||
| kf.inTangent = obj["inTangent"].ToObject<float>(); | ||
| if (obj["outTangent"] != null) | ||
| kf.outTangent = obj["outTangent"].ToObject<float>(); | ||
| if (obj["inWeight"] != null) | ||
| kf.inWeight = obj["inWeight"].ToObject<float>(); | ||
| if (obj["outWeight"] != null) | ||
| kf.outWeight = obj["outWeight"].ToObject<float>(); | ||
|
|
||
| keyframes.Add(kf); | ||
| } | ||
| } | ||
|
|
||
| return keyframes.ToArray(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Unrecognized keyframe formats are silently dropped.
If a JSON array item is neither a JArray pair nor a JObject, it is skipped without any indication. This can cause partial data loss — e.g., 5 keyframes sent but only 3 recognized — producing a subtly wrong animation with no error.
Consider returning an error (or at minimum a warning) when an item doesn't match either expected format, rather than silently discarding it.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/ClipCreate.cs` around lines 428 - 467, In
ParseKeyframes, currently any array item that is neither a JArray pair nor a
JObject is silently ignored; update the foreach to detect that else case (when
item is not JArray or JObject) and emit a clear warning or throw an error so
callers notice malformed input — e.g., use UnityEngine.Debug.LogWarningFormat
(or throw a JsonException) referencing the keysToken item and index, include the
offending token content and context (function ParseKeyframes, parameter
keysToken) and keep existing behavior for valid items; ensure the warning path
still returns the keyframes parsed so far or change the method to propagate the
error based on caller expectations.
| int? eventIndex = @params["eventIndex"]?.ToObject<int?>(); | ||
| if (eventIndex.HasValue) | ||
| { | ||
| if (eventIndex.Value < 0 || eventIndex.Value >= events.Count) | ||
| return new { success = false, message = $"Event index {eventIndex.Value} out of range (0-{events.Count - 1})" }; | ||
|
|
||
| events.RemoveAt(eventIndex.Value); | ||
| } |
There was a problem hiding this comment.
Edge case: error message is confusing when the clip has zero events.
When events.Count == 0, the message reads "Event index X out of range (0--1)". Consider handling the empty-events case before checking the index:
Proposed fix
+ if (events.Count == 0)
+ return new { success = false, message = "Clip has no events to remove" };
+
int? eventIndex = `@params`["eventIndex"]?.ToObject<int?>();
if (eventIndex.HasValue)
{
if (eventIndex.Value < 0 || eventIndex.Value >= events.Count)
return new { success = false, message = $"Event index {eventIndex.Value} out of range (0-{events.Count - 1})" };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int? eventIndex = @params["eventIndex"]?.ToObject<int?>(); | |
| if (eventIndex.HasValue) | |
| { | |
| if (eventIndex.Value < 0 || eventIndex.Value >= events.Count) | |
| return new { success = false, message = $"Event index {eventIndex.Value} out of range (0-{events.Count - 1})" }; | |
| events.RemoveAt(eventIndex.Value); | |
| } | |
| if (events.Count == 0) | |
| return new { success = false, message = "Clip has no events to remove" }; | |
| int? eventIndex = `@params`["eventIndex"]?.ToObject<int?>(); | |
| if (eventIndex.HasValue) | |
| { | |
| if (eventIndex.Value < 0 || eventIndex.Value >= events.Count) | |
| return new { success = false, message = $"Event index {eventIndex.Value} out of range (0-{events.Count - 1})" }; | |
| events.RemoveAt(eventIndex.Value); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/ClipCreate.cs` around lines 569 - 576, The
error message when validating eventIndex is confusing for empty event lists;
modify the logic in ClipCreate.cs around the eventIndex validation (the block
referencing eventIndex and events.RemoveAt) to first check if events.Count == 0
and return a clear failure (e.g., "No events to remove" or similar) before doing
the index bounds check, otherwise compute and display a non-negative upper bound
for the range when constructing the out-of-range message (use events.Count - 1
only when events.Count > 0).
| float duration = @params["duration"]?.ToObject<float>() ?? 1f; | ||
| float amplitude = @params["amplitude"]?.ToObject<float>() ?? 1f; |
There was a problem hiding this comment.
Division by zero when duration is zero or negative.
Several Apply* methods divide by duration (e.g., ApplyRotate Line 162, ApplySpin Line 243, ApplySlideIn Line 319). A caller passing "duration": 0 will produce Infinity/NaN tangents or collapse all keyframes to t=0. Validate early:
Proposed fix
float duration = `@params`["duration"]?.ToObject<float>() ?? 1f;
float amplitude = `@params`["amplitude"]?.ToObject<float>() ?? 1f;
+if (duration <= 0f)
+ return new { success = false, message = "'duration' must be greater than 0" };
bool loop = `@params`["loop"]?.ToObject<bool>() ?? true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float duration = @params["duration"]?.ToObject<float>() ?? 1f; | |
| float amplitude = @params["amplitude"]?.ToObject<float>() ?? 1f; | |
| float duration = `@params`["duration"]?.ToObject<float>() ?? 1f; | |
| float amplitude = `@params`["amplitude"]?.ToObject<float>() ?? 1f; | |
| if (duration <= 0f) | |
| return new { success = false, message = "'duration' must be greater than 0" }; |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/Animation/ClipPresets.cs` around lines 31 - 32, The
code currently reads duration from params into the local variable duration and
then uses it as a divisor in multiple methods (ApplyRotate, ApplySpin,
ApplySlideIn), which will produce Infinity/NaN when duration is zero or
negative; fix by validating and normalizing duration immediately after parsing
(e.g., if duration <= 0f then set duration = 1f or Mathf.Epsilon and optionally
log/warn), so all downstream uses in ApplyRotate, ApplySpin, ApplySlideIn (and
any other Apply* methods) operate on a safe, positive duration value.
|
Haha, Coplay is launching it's version of dedicated animation tools as well soon. Kind of cool they're both happening! |
2 participants
Description
Provide a new feature that adds animation and an animation controller to the scene, while changing all the specs to make simple, working animations, such as looping, circling, and bouncing. No physics yet, pure point and joint-based simple animations. With no humanoid animation tested, I think the current one could:
Type of Change
Save your change type
Changes Made
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
I do expect testing results/feedback from the community, as its use case for me is a little simple.
Summary by Sourcery
Add a full manage_animation toolchain for controlling Unity Animator/AnimatorController/AnimationClips via MCP and CLI, including presets and blend trees.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests