get/set animation node positions#1206
Open
Thaina wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughTwo new static methods, ChangesAnimator State Position Endpoints
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Thaina
force-pushed
the
animation-node-graph-position-MCP
branch
from
4d8cf5d to
95b9c2f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs`:
- Around line 434-443: The GetStatePositions method currently collects and
returns all nodes in a single response without pagination, which violates coding
guidelines for endpoints that could return large datasets. Modify the method to
accept page_size and cursor parameters in the method signature, implement
pagination logic to slice the nodes list collected by CollectPositions based on
these parameters, and update the return object to include a next_cursor field
that indicates the position for the next page of results (or null if this is the
final page). This ensures large AnimatorControllers do not create oversized
payloads.
- Around line 474-481: The foreach loop iterating through positions assumes
every token item is a JObject that supports property indexing, but malformed
entries (nulls, primitives, or incompatible types) will throw exceptions. Before
accessing token["name"], token["x"], and token["y"], add a guard clause to
verify that token is a JObject instance; if not, skip that iteration using
continue or collect the error appropriately. This prevents crashes on malformed
position data while maintaining the existing logic for valid entries.
- Around line 473-503: The SetStatePositions method currently matches states by
name only, which is ambiguous since duplicate names are valid in Animator graphs
and could cause unintended updates across layers. Replace the name-only key in
the want dictionary with a unique state identifier that combines the layer
index, state-machine path, and state name (for example
"layer0/RootStateMachine/StateName"). Update the contract to expect this unique
identifier format instead of just "name", and modify the ApplyPositions method
(and any other matching logic around lines 507-521) to construct and match
against this same unique identifier when traversing the state machine hierarchy,
so that only the intended state is updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10199cd6-3ddb-40ef-aa2e-d236839cdc09
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/Animation/ControllerCreate.csMCPForUnity/Editor/Tools/Animation/ManageAnimation.csServer/src/services/tools/manage_animation.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs (2)
499-506:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
positionsitems before property indexing to avoid runtime exceptions.On Line 501–506, the loop indexes
token["..."]without verifying object shape. Non-object entries inpositionscan throw and convert client input errors into internal failures. Add anif (token is not JObject item) continue/collect-errorguard before field access.Minimal fix
foreach (var token in positions) { - string name = token["name"]?.ToString(); + if (token is not JObject item) + continue; + + string name = item["name"]?.ToString(); if (string.IsNullOrEmpty(name)) continue; - float x = token["x"]?.ToObject<float>() ?? 0f; - float y = token["y"]?.ToObject<float>() ?? 0f; - int? layer = token["layer"]?.ToObject<int>() ?? defaultLayer; + float x = item["x"]?.ToObject<float>() ?? 0f; + float y = item["y"]?.ToObject<float>() ?? 0f; + int? layer = item["layer"]?.ToObject<int>() ?? defaultLayer; want[$"{(layer.HasValue ? layer.Value.ToString() : "*")}:{name}"] = new Vector2(x, y); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs` around lines 499 - 506, In the foreach loop that iterates over positions, add a guard clause at the beginning to verify that each token is a JObject before attempting to access its properties using the indexer (e.g., token["name"]). If the token is not a JObject, use continue to skip it or handle the error appropriately. This prevents runtime exceptions when non-object entries exist in the positions collection and converts potential client input errors into graceful handling rather than internal failures.
497-507:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftState key is still ambiguous for duplicate names within the same layer.
On Line 497 and Line 541, matching is keyed by
{layer}:{name}(or wildcard), which is not unique across sub-state-machines in that layer. A single request can still move multiple unintended nodes when names repeat. The read/write contract needs a unique identifier (e.g., layer + state-machine path + state name) in bothGetStatePositionsoutput andSetStatePositionsmatching.Suggested direction
- // Returns [{ name, x, y, layer }] + // Returns [{ id, name, x, y, layer, path }] - want[$"{(layer.HasValue ? layer.Value.ToString() : "*")}:{name}"] = new Vector2(x, y); + // Expect `id` from GetStatePositions; use that as the only write key. + want[id] = new Vector2(x, y); - string scopedKey = $"{layer}:{name}"; - string anyKey = $"*:{name}"; + string scopedKey = BuildStateId(layer, stateMachinePath, name);Also applies to: 539-545
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs` around lines 497 - 507, The state positioning key format using only layer and name is ambiguous when duplicate state names exist in different sub-state-machines within the same layer. Update the key structure to include the state-machine path in addition to layer and name to ensure uniqueness. In the section around line 497-507 where the want dictionary is built, modify the key format from {layer}:{name} to include the full state-machine path (e.g., {layer}:{stateMachinePath}:{name}). Apply the same key format change in the matching logic around lines 539-545 to ensure consistency between how positions are stored and retrieved. This requires coordinating the GetStatePositions output format with the SetStatePositions matching logic to use the same unique identifier scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@MCPForUnity/Editor/Tools/Animation/ControllerCreate.cs`:
- Around line 499-506: In the foreach loop that iterates over positions, add a
guard clause at the beginning to verify that each token is a JObject before
attempting to access its properties using the indexer (e.g., token["name"]). If
the token is not a JObject, use continue to skip it or handle the error
appropriately. This prevents runtime exceptions when non-object entries exist in
the positions collection and converts potential client input errors into
graceful handling rather than internal failures.
- Around line 497-507: The state positioning key format using only layer and
name is ambiguous when duplicate state names exist in different
sub-state-machines within the same layer. Update the key structure to include
the state-machine path in addition to layer and name to ensure uniqueness. In
the section around line 497-507 where the want dictionary is built, modify the
key format from {layer}:{name} to include the full state-machine path (e.g.,
{layer}:{stateMachinePath}:{name}). Apply the same key format change in the
matching logic around lines 539-545 to ensure consistency between how positions
are stored and retrieved. This requires coordinating the GetStatePositions
output format with the SetStatePositions matching logic to use the same unique
identifier scheme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2f12fc9-d9c0-495a-9b3f-ef909f5fbfe1
📒 Files selected for processing (2)
MCPForUnity/Editor/Tools/Animation/ControllerCreate.csServer/tests/test_manage_animation.py
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
adding
get_state_positionsandset_state_positionsto the mcp managed animation controller so that AI can modified the node graph position in addition to create and set its transitionType of Change
Changes Made
Compatibility / Package Source
Packages/packages-lock.json: 4d8cf5dTesting/Screenshots/Recordings
cd Server && uv run pytest tests/ -v)Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)Related Issues
Additional Notes
Summary by CodeRabbit