Fix the bug where the execution method fails to find the game object due to the missing by_ prefix.
#294
Conversation
…ect due to the missing `by_` prefix.
WalkthroughRefactors ManageGameObject command handling: normalizes search methods, expands object discovery across all loaded scenes, adds includeNonPublicSerialized plumbing, routes prefab-related commands to ManageAsset where applicable, tightens/clarifies logging and error handling, and unifies JSON serialization usage for property conversion and nested property setting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant MGO as ManageGameObject
participant Finder as FindObjectsInternal
participant MA as ManageAsset
participant Unity as Unity Engine
Caller->>MGO: HandleCommand(request)
MGO->>MGO: Normalize searchMethod (by_id/by_name/by_path/...)
alt Search target in scenes
MGO->>Finder: Find across all loaded scenes
Finder-->>MGO: GameObject(s) or not found
end
alt Target is Prefab and action is modify/set_component_property
MGO->>MA: Forward to ManageAsset.HandleCommand(assetParams)
MA-->>MGO: Result
MGO-->>Caller: Response
else Target is Prefab with blocked action
MGO-->>Caller: Error directing to manage_asset
else Non-prefab or allowed action
MGO->>Unity: Get/Add/Remove components (respect includeNonPublicSerialized)
MGO->>Unity: Get/Set component property (nested supported)
Unity-->>MGO: Operation result or exception
MGO-->>Caller: Success/Error with detailed logging
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (2)
1445-1458: Avoid flooding logs on every search path scan.
Lines 1445-1458 now emitDebug.Logper scene and per root object every timeGetAllSceneObjectsruns. Because this helper sits under virtually every ManageGameObject action, a single automation session will spam hundreds of logs and slow the editor. Please remove or gate these statements behind an explicit debug flag before merging.- Debug.Log($"[ManageGameObject] Scanning scene '{scene.name}' - {rootObjects.Length} root objects"); @@ - Debug.Log($"[ManageGameObject] Scene '{scene.name}' root '{root.name}' contains {sceneObjects.Count} objects (includeInactive: {includeInactive})"); @@ - Debug.Log($"[ManageGameObject] Total objects found: {allObjects.Count}");
960-989: Preserve component order without the O(n²) inserts.
Lines 960-989 iterate backward andInsert(0, …)on every pass, which balloons to quadratic work as component counts grow. Iterate forward and append instead soget_componentsstays linear.- for (int i = componentCount - 1; i >= 0; i--) // Iterate backwards over the COPY + for (int i = 0; i < componentCount; i++) { Component c = componentsToIterate[i]; // Use the copy @@ - if (data != null) // Ensure GetComponentData didn't return null - { - componentData.Insert(0, data); // Insert at beginning to maintain original order in final list - } + if (data != null) // Ensure GetComponentData didn't return null + { + componentData.Add(data); + } @@ - componentData.Insert(0, new JObject( // Insert error marker at beginning + componentData.Add(new JObject( new JProperty("typeName", c.GetType().FullName + " (Serialization Error)"), new JProperty("instanceID", c.GetInstanceID()), new JProperty("error", ex.Message) ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs(25 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
UnityMcpBridge/Editor/Tools/ManageGameObject.cs
🧬 Code graph analysis (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (13)
Vector2Converter(36-56)QuaternionConverter(58-84)ColorConverter(86-112)RectConverter(114-140)BoundsConverter(142-161)UnityEngineObjectConverter(164-265)Color(102-111)Vector3(25-33)Vector2(48-55)Quaternion(74-83)Rect(130-139)Bounds(154-160)UnityEngine(222-264)UnityMcpBridge/Editor/Tools/ManageAsset.cs (1)
ConvertJTokenToType(1158-1235)UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs (1)
JToken(506-525)
🔇 Additional comments (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (1)
1233-1248: Search method normalization resolves the missingby_cases.
Line 1244 ensures plain"name","id", etc., are internally rewritten to the existingby_*switch arms, so the handler now accepts both forms and addresses the repro the PR calls out. Nicely scoped fix.
|
Hey, thanks for submitting? Can you clarify this:
Is it possible you can add a unit test to validate this bug and change? It doesn't have to be end-to-end, just a Unity test will suffice. Also, would you have tried these changes using a large project i.e one with many scenes? |
|
Oh I'm sorry, I didn't see this PR. I believe the one I just merged should fix this. Please try in latest version. |
|
I only fixed the name search issue though, looks like you have some additional functionality that may still be good, if you want to test it with latest main changes. |
|
After updating to the latest version, I haven’t encountered this issue recently. Thank you! |
|
Good work @dsarno! |
3 participants
When I use Chinese prompts to execute an MCP task in VS Code, and search for a GameObject by name with the input:
{ "action": "find", "search_method": "name", "search_term": "ChessGame" }In the C# code, the
switchmatchesby_name, and using other search methods will cause type issues.Summary by CodeRabbit