Autoformat code #297
Autoformat code #297
Conversation
WalkthroughAdds skip-marking logic to a JUnit XML post-processor, significant refactors to Unity editor tooling (ManageGameObject, GameObjectSerializer, MCP bridge), and an optional error field to telemetry. Reorganizes a complex CI workflow (claude-nl-suite) around licensing gates. Broad formatting cleanups across C#, Python, and tests, plus minor workflow whitespace fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Job
participant MS as mark_skipped.py
participant FS as Filesystem
participant XML as JUnit XML
CI->>MS: Invoke main(path)
MS->>FS: Read XML file
MS->>XML: Parse testsuites
loop Each testsuite
loop Each testcase
MS->>XML: Inspect failure/error text
alt matches skip pattern
MS->>XML: Remove failure/error
MS->>XML: Add <skipped reason="pattern">
end
end
MS->>XML: Recompute counts (tests, failures, errors, skipped)
end
alt Any change
MS->>FS: Write updated XML
MS-->>CI: Exit 0 (updated)
else No change
MS-->>CI: Exit 0 (no-op)
end
sequenceDiagram
autonumber
participant Client as MCP Client
participant Bridge as MCPForUnityBridge
participant MG as ManageGameObject
participant GS as GameObjectSerializer
Client->>Bridge: Command (e.g., set_component_property)
Bridge->>Bridge: Validate/deserialize JSON
Bridge->>MG: Execute request (includes includeNonPublicSerialized)
alt Prefab target
MG->>MG: Redirect/deny per action
else Scene object
MG->>MG: Resolve object/components/properties
MG->>GS: Serialize GameObject/Component data
GS-->>MG: Plain object payload
end
MG-->>Bridge: Result or structured error
Bridge-->>Client: Framed response (status, data/error)
sequenceDiagram
autonumber
participant GH as GitHub Actions
participant WF as claude-nl-suite.yml
participant Lic as Licensing Gate
participant UB as Unity Bridge
participant NL as NL/T Tests
GH->>WF: workflow_dispatch / push
WF->>Lic: Decide ULF vs EBL
alt ULF available
Lic-->>WF: Stage ULF
else EBL
Lic-->>WF: Activate via Entitlement
else Fail
Lic-->>WF: Missing entitlement -> fail job
end
WF->>UB: Warm-up and start persistent bridge
WF->>NL: Run NL pass
WF->>NL: Run T pass (+ retry/backfill if needed)
WF->>GH: Publish JUnit + artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_manage_script_uri.py (1)
1-41: Fix import order to avoidModuleNotFoundError.By importing
tools.manage_scripton Line 1 you now load the module before inserting the Unity server path intosys.path. In a fresh test environment that path is absent, so the import will fail and the entire test module will abort. Move the import until after thesys.path.insert(...)call (e.g., viaimportlib.import_module) so the dynamic path setup still works.-import tools.manage_script as manage_script # type: ignore +import importlib @@ sys.path.insert(0, str(SRC)) +manage_script = importlib.import_module("tools.manage_script") # type: ignoreUnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
698-725: Fix invalid cast: JArray isn’t IEnumerable; causes false negativesCasting dynamic args (JArray) to IEnumerable can throw at runtime and makes IsCursorConfigured always return false on errors. Use non-generic IEnumerable or JArray directly.
Apply this diff:
- var args = unity.args; - if (args == null) return false; - // Prefer exact extraction of the --directory value and compare normalized paths - string[] strArgs = ((System.Collections.Generic.IEnumerable<object>)args) - .Select(x => x?.ToString() ?? string.Empty) - .ToArray(); + var argsToken = unity.args; + if (argsToken == null) return false; + // Prefer exact extraction of the --directory value and compare normalized paths + // Handle both JArray and generic IEnumerable safely + string[] strArgs; + if (argsToken is JArray jarr) + { + strArgs = jarr.Select(t => t?.ToString() ?? string.Empty).ToArray(); + } + else if (argsToken is System.Collections.IEnumerable en) + { + strArgs = en.Cast<object>().Select(x => x?.ToString() ?? string.Empty).ToArray(); + } + else + { + return false; + }(Requires
using Newtonsoft.Json.Linq;which this file already has.)🧹 Nitpick comments (13)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
212-222: Consider consolidating the duplicated IsDebugEnabled pattern.The
IsDebugEnabled()helper appears in multiple files with identical implementations (PortManager.cs, MCPForUnityBridge.cs, McpLog.cs). Consider extracting this to a shared utility class to reduce duplication and improve maintainability.For example, create a shared helper:
// In a new DebugHelper.cs or add to an existing utility class public static class DebugPrefs { public static bool IsDebugEnabled() { try { return UnityEditor.EditorPrefs.GetBool("MCPForUnity.DebugLogs", false); } catch { return false; } } }Then replace all instances with calls to this shared method.
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (5)
2040-2059: Remove unreachable commented code.The fallback logic at lines 2056-2058 is unreachable because all paths either return or throw. Since the code is commented and dead, consider removing it to reduce clutter.
Apply this diff to remove the unreachable commented code:
throw; } - // If ToObject succeeded, it would have returned. If it threw, we wouldn't reach here. - // This fallback logic is likely unreachable if ToObject covers all cases or throws on failure. - // Debug.LogWarning($"Conversion failed for token to {targetType.FullName}. Token: {token.ToString(Formatting.None)}"); - // return targetType.IsValueType ? Activator.CreateInstance(targetType) : null; }
950-1021: Reverse iteration pattern prevents collection modification issues.The defensive approach of copying components to a
List, nulling the original array reference, and iterating backward (lines 963-1003) avoids potential collection modification exceptions that can occur when reflection triggers property getters that modify the component array. TheInsert(0, data)maintains original order in the final result.This pattern is robust but adds O(n) overhead for the list copy and O(n²) for insertions. Consider using a deque or reversing once at the end if performance becomes a concern with large component counts.
Alternative approach to avoid O(n²) insertions:
// After the reverse loop, reverse the list once componentData.Reverse();This would allow appending (
componentData.Add(data)) during the loop, then reversing once at the end for O(n) total complexity.
1886-1920: Material shader property handling is comprehensive but could be simplified.The cascading array-size checks (4, 3, 2 elements) with multiple
try-catchblocks for each type conversion are thorough but verbose. The logic correctly handles Color (RGB/RGBA), Vector2/3/4, and Texture assignments.Consider extracting this into a helper method to reduce duplication and improve readability, especially since similar patterns exist elsewhere.
private static bool TrySetMaterialProperty(Material material, string propName, JToken value, JsonSerializer serializer) { if (value is JArray jArray) { return TrySetMaterialVector(material, propName, jArray, serializer); } else if (value.Type == JTokenType.Float || value.Type == JTokenType.Integer) { try { material.SetFloat(propName, value.ToObject<float>(serializer)); return true; } catch { } } // ... etc return false; }
2156-2258: New FindObjectByInstruction method enables UnityEngineObjectConverter integration.This public method provides a centralized lookup mechanism for Unity objects based on JSON find instructions, supporting:
- Asset database lookups by path/GUID/name
- Scene object searches by ID/name/path
- Component resolution on found GameObjects
The method correctly prioritizes asset lookups for asset types (Materials, Textures, etc.) before falling back to scene searches. Ambiguous asset matches appropriately return null with a warning.
Consider adding a cache for repeated lookups of the same instruction to avoid redundant searches:
private static readonly Dictionary<string, WeakReference<UnityEngine.Object>> _findCache = new();This would benefit workflows that repeatedly reference the same objects across multiple commands.
882-882: Remove trailing empty lines for consistency.Multiple trailing empty lines throughout the file should be removed to match the project's formatting style.
Apply this pattern at lines 882, 1184, 1233, 2271, 2277, 2281, 2460, and 2485:
- } - + }Also applies to: 1184-1184, 1233-1233, 2271-2271, 2277-2277, 2281-2281, 2460-2460, 2485-2485
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (6)
862-888: Avoid repeated ResolveClaude()/FindUvPath() in OnGUI pathOnGUI can run many times per second. Re-resolving CLI paths multiple times per draw adds I/O overhead. Resolve once and reuse locals; also reuse uvPathEarly later (hideConfigInfo).
Apply these minimal edits:
@@ - // Special pre-check for Claude Code: if CLI missing, reflect in status UI + // Special pre-check for Claude Code: cache CLI/UV resolution for this draw if (mcpClient.mcpType == McpTypes.ClaudeCode) { - string claudeCheck = ExecPath.ResolveClaude(); + string claudePath = ExecPath.ResolveClaude(); + string claudeCheck = claudePath; if (string.IsNullOrEmpty(claudeCheck)) { mcpClient.configStatus = "Claude Not Found"; mcpClient.status = McpStatus.NotConfigured; } } @@ - bool uvRequired = mcpClient.mcpType != McpTypes.ClaudeCode; - bool uvMissingEarly = false; - if (uvRequired) - { - string uvPathEarly = FindUvPath(); - if (string.IsNullOrEmpty(uvPathEarly)) - { - uvMissingEarly = true; - mcpClient.configStatus = "uv Not Found"; - mcpClient.status = McpStatus.NotConfigured; - } - } + bool uvRequired = mcpClient.mcpType != McpTypes.ClaudeCode; + string uvPathEarly = uvRequired ? FindUvPath() : null; + bool uvMissingEarly = uvRequired && string.IsNullOrEmpty(uvPathEarly); + if (uvMissingEarly) + { + mcpClient.configStatus = "uv Not Found"; + mcpClient.status = McpStatus.NotConfigured; + } @@ - if (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(ExecPath.ResolveClaude())) + if (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(claudeCheck)) @@ - bool claudeAvailable = !string.IsNullOrEmpty(ExecPath.ResolveClaude()); + bool claudeAvailable = !string.IsNullOrEmpty(claudeCheck); @@ - string resolvedClaude = ExecPath.ResolveClaude(); + string resolvedClaude = claudeCheck; @@ - bool hideConfigInfo = - (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(ExecPath.ResolveClaude())) - || ((mcpClient.mcpType != McpTypes.ClaudeCode) && string.IsNullOrEmpty(FindUvPath())); + bool hideConfigInfo = + (mcpClient.mcpType == McpTypes.ClaudeCode && string.IsNullOrEmpty(claudeCheck)) + || ((mcpClient.mcpType != McpTypes.ClaudeCode) && string.IsNullOrEmpty(uvPathEarly));This keeps behavior identical while cutting redundant probes.
Also applies to: 903-919, 922-959, 971-1021, 1072-1084
1759-1766: Use validated uv path consistently (avoid ExecPath.ResolveUv fallback to "uv")Registration should use the same uv discovery/validation as configuration. Falling back to a bare "uv" can fail or pick the wrong binary on systems with multiple versions.
Apply this diff:
- string uvPath = ExecPath.ResolveUv() ?? "uv"; + string uvPath = FindUvPath(); + if (string.IsNullOrEmpty(uvPath)) + { + UnityEngine.Debug.LogError("MCP for Unity: UV package manager not found. Install uv and retry."); + return; + }This aligns with ServerInstaller.FindUvPath() semantics used elsewhere.
1815-1842: DRY the Claude server-name list (used in get/remove loops)The candidate name arrays are duplicated, which risks drift. Centralize once and reuse for both “get” and “remove” passes.
Example:
- string[] candidateNamesForGet = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; + string[] candidateNamesForGet = PossibleClaudeServerNames(); @@ - string[] possibleNames = { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" }; + string[] possibleNames = PossibleClaudeServerNames();Add a private helper:
private static string[] PossibleClaudeServerNames() => new[] { "UnityMCP", "unityMCP", "unity-mcp", "UnityMcpServer" };
1418-1421: Avoid magic “Configured successfully” sentinel across code pathsMultiple methods compare exact return text to set status. Promote to a private const or switch to a Result enum to prevent fragile string coupling.
For a light touch:
+ private const string ConfigOk = "Configured successfully"; @@ - if (result == "Configured successfully") + if (result == ConfigOk) { mcpClient.SetStatus(McpStatus.Configured); } @@ - if (!changed) - { - return "Configured successfully"; - } + if (!changed) return ConfigOk; @@ - return "Configured successfully"; + return ConfigOk;Repeat for ConfigureCodexClient.
Also applies to: 1511-1513, 1253-1254
173-221: Layout constants: consider EditorPrefs-backed overrides for widths/heightsFixed panel sizes can clip on low-DPI or unusually long localized strings. Expose optional EditorPrefs (or a hidden “Advanced Layout” foldout) to tweak colWidth/topPanelHeight/bottomPanelHeight.
No behavior change; improves accessibility across setups.
1901-1979: Claude config parsing: add tolerant checks for array/object typesDynamic parsing can vary with user edits. Before accessing
claudeConfig.projects, guard that it’s an object; normalize case-insensitive lookups formcpServers. Current code is mostly robust, but a couple extra type checks reduce warning noise.Add:
if (claudeConfig?.projects is JObject projectsObj) { foreach (var prop in projectsObj.Properties()) { // use prop.Name/prop.Value safely } }Keeps log clean on malformed files. Based on learnings.
UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs (1)
528-528: Remove trailing empty line.Minor formatting inconsistency: the closing brace should not be followed by an empty line.
} -} - +}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.github/scripts/mark_skipped.py(5 hunks).github/workflows/bump-version.yml(1 hunks).github/workflows/claude-nl-suite.yml(1 hunks).github/workflows/github-repo-stats.yml(0 hunks).github/workflows/unity-tests.yml(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs(8 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs(10 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(18 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs(11 hunks)UnityMcpBridge/Editor/AssemblyInfo.cs(1 hunks)UnityMcpBridge/Editor/Data/DefaultServerConfig.cs(0 hunks)UnityMcpBridge/Editor/External/Tommy.cs(45 hunks)UnityMcpBridge/Editor/Helpers/ExecPath.cs(1 hunks)UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs(13 hunks)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs(0 hunks)UnityMcpBridge/Editor/Helpers/McpLog.cs(0 hunks)UnityMcpBridge/Editor/Helpers/PackageDetector.cs(0 hunks)UnityMcpBridge/Editor/Helpers/PackageInstaller.cs(2 hunks)UnityMcpBridge/Editor/Helpers/PortManager.cs(4 hunks)UnityMcpBridge/Editor/Helpers/Response.cs(0 hunks)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs(3 hunks)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs(0 hunks)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs(11 hunks)UnityMcpBridge/Editor/Helpers/Vector3Helper.cs(0 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(5 hunks)UnityMcpBridge/Editor/Tools/CommandRegistry.cs(0 hunks)UnityMcpBridge/Editor/Tools/ManageGameObject.cs(20 hunks)UnityMcpBridge/Editor/Tools/ManageScene.cs(0 hunks)UnityMcpBridge/Editor/Tools/ManageScript.cs(0 hunks)UnityMcpBridge/Editor/Tools/ManageShader.cs(1 hunks)UnityMcpBridge/Editor/Tools/ReadConsole.cs(3 hunks)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs(36 hunks)UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs(4 hunks)UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs(3 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(9 hunks)mcp_source.py(4 hunks)test_unity_socket_framing.py(5 hunks)tests/conftest.py(0 hunks)tests/test_edit_normalization_and_noop.py(5 hunks)tests/test_edit_strict_and_warnings.py(4 hunks)tests/test_find_in_file_minimal.py(3 hunks)tests/test_get_sha.py(2 hunks)tests/test_improved_anchor_matching.py(5 hunks)tests/test_logging_stdout.py(1 hunks)tests/test_manage_script_uri.py(4 hunks)tests/test_read_console_truncate.py(5 hunks)tests/test_read_resource_minimal.py(3 hunks)tests/test_resources_api.py(4 hunks)tests/test_script_tools.py(8 hunks)tests/test_telemetry_endpoint_validation.py(1 hunks)tests/test_telemetry_queue_worker.py(2 hunks)tests/test_telemetry_subaction.py(1 hunks)tests/test_transport_framing.py(3 hunks)tests/test_validate_script_summary.py(3 hunks)tools/stress_mcp.py(10 hunks)💤 Files with no reviewable changes (14)
- UnityMcpBridge/Editor/Helpers/Response.cs
- UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs
- TestProjects/UnityMCPTests/Assets/Scripts/Hello.cs
- .github/workflows/github-repo-stats.yml
- UnityMcpBridge/Editor/Helpers/PackageDetector.cs
- UnityMcpBridge/Editor/Helpers/Vector3Helper.cs
- UnityMcpBridge/Editor/Helpers/McpLog.cs
- TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs
- UnityMcpBridge/Editor/Tools/ManageScript.cs
- UnityMcpBridge/Editor/Tools/ManageScene.cs
- tests/conftest.py
- UnityMcpBridge/Editor/Tools/CommandRegistry.cs
- UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs
- UnityMcpBridge/Editor/Data/DefaultServerConfig.cs
🧰 Additional context used
🧬 Code graph analysis (22)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs (2)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (3)
ComponentResolver(2286-2546)TryResolve(2296-2332)Type(2265-2279)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (14)
Test(32-41)Test(43-49)Test(51-57)Test(59-65)Test(67-73)Test(75-82)Test(84-92)Test(94-100)Test(102-109)Test(111-118)Test(120-133)Test(135-147)Test(149-158)Test(160-168)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageScript.cs (1)
ManageScript(52-2552)tests/test_edit_normalization_and_noop.py (2)
tests/test_edit_strict_and_warnings.py (5)
_load(30-34)DummyMCP(40-45)tool(43-45)deco(44-44)setup_tools(48-51)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (1)
register_manage_script_edits_tools(309-968)tests/test_read_resource_minimal.py (3)
tests/test_find_in_file_minimal.py (1)
resource_tools(26-29)tests/test_resources_api.py (1)
resource_tools(38-41)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
register_resource_tools(136-396)read_resource(194-350)UnityMcpBridge/Editor/Helpers/PackageInstaller.cs (1)
UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
ServerInstaller(12-743)EnsureServerInstalled(22-110)UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (3)
UnityMcpBridge/Editor/Helpers/PortManager.cs (1)
IsDebugEnabled(19-23)UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
IsDebugEnabled(63-66)UnityMcpBridge/Editor/Helpers/McpLog.cs (1)
IsDebugEnabled(10-13)tests/test_resources_api.py (2)
tests/test_find_in_file_minimal.py (3)
resource_tools(26-29)tool(18-22)deco(19-21)tests/test_read_resource_minimal.py (3)
resource_tools(43-46)tool(35-39)deco(36-38)tests/test_script_tools.py (5)
tests/test_edit_normalization_and_noop.py (2)
_Dummy(17-18)DummyMCP(42-47)tests/test_improved_anchor_matching.py (2)
_Dummy(21-22)load_module(34-38)tests/test_manage_script_uri.py (2)
_Dummy(30-31)DummyMCP(46-54)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
register_manage_script_tools(22-565)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
register_manage_asset_tools(13-86)tests/test_read_console_truncate.py (2)
tests/test_get_sha.py (2)
_load_module(30-34)DummyMCP(41-49)UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py (2)
register_read_console_tools(12-90)read_console(17-90)tests/test_improved_anchor_matching.py (2)
tests/test_script_tools.py (1)
load_module(32-36)UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py (2)
_find_best_anchor_match(94-133)_apply_edits_locally(12-91)tests/test_telemetry_endpoint_validation.py (1)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (1)
TelemetryCollector(190-376)tests/test_validate_script_summary.py (2)
tests/test_get_sha.py (3)
_Dummy(17-18)_load_module(30-34)DummyMCP(41-49)tests/test_manage_script_uri.py (2)
_Dummy(30-31)DummyMCP(46-54)tests/test_manage_script_uri.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(456-513)tests/test_transport_framing.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
UnityConnection(32-371)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (15)
SetUp(14-30)Test(32-41)Test(43-49)Test(51-57)Test(59-65)Test(67-73)Test(75-82)Test(84-92)Test(94-100)Test(102-109)Test(111-118)Test(120-133)Test(135-147)Test(149-158)Test(160-168)UnityMcpBridge/Editor/Tools/ManageGameObject.cs (9)
GameObject(1268-1291)ManageGameObject(22-2280)HandleCommand(41-186)ComponentResolver(2286-2546)List(1296-1455)List(2347-2384)List(2407-2425)List(2430-2469)List(2477-2512)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs (2)
Test(11-19)Test(21-29)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs (1)
UnityMcpBridge/Editor/Tools/ManageGameObject.cs (6)
ComponentResolver(2286-2546)List(1296-1455)List(2347-2384)List(2407-2425)List(2430-2469)List(2477-2512)tests/test_get_sha.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py (1)
manage_script(456-513)UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (7)
UnityMcpBridge/Editor/Helpers/CodexConfigHelper.cs (4)
CodexConfigHelper(17-242)IsCodexConfigured(19-41)BuildCodexServerBlock(43-49)TryParseCodexServer(105-145)UnityMcpBridge/Editor/Helpers/ExecPath.cs (2)
ExecPath(11-277)ResolveClaude(16-89)UnityMcpBridge/Editor/Helpers/ServerInstaller.cs (2)
FindUvPath(529-717)ServerInstaller(12-743)UnityMcpBridge/Editor/Helpers/McpConfigFileHelper.cs (2)
McpConfigFileHelper(14-185)WriteAtomicFile(100-150)UnityMcpBridge/Editor/Helpers/ConfigJsonBuilder.cs (2)
ConfigJsonBuilder(7-128)BuildManualConfigJson(9-29)UnityMcpBridge/Editor/Windows/ManualConfigEditorWindow.cs (2)
ManualConfigEditorWindow(9-295)ShowWindow(19-27)UnityMcpBridge/Editor/Helpers/ServerPathResolver.cs (2)
ServerPathResolver(8-148)TryFindEmbeddedServerSource(15-100)UnityMcpBridge/UnityMcpServer~/src/server.py (2)
UnityMcpBridge/UnityMcpServer~/src/telemetry.py (5)
record_telemetry(391-395)record_milestone(249-271)record_milestone(398-400)RecordType(37-46)MilestoneType(49-57)UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (1)
get_unity_connection(378-399)UnityMcpBridge/Editor/MCPForUnityBridge.cs (3)
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (3)
TelemetryHelper(12-223)RecordBridgeStartup(139-146)IsDebugEnabled(212-222)UnityMcpBridge/Editor/Helpers/McpLog.cs (5)
IsDebugEnabled(10-13)McpLog(6-30)Info(15-19)Warn(21-24)Error(26-29)UnityMcpBridge/Editor/Models/Command.cs (1)
Command(8-19)tests/test_find_in_file_minimal.py (2)
tests/test_read_resource_minimal.py (2)
resource_tools(43-46)DummyMCP(31-39)UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py (2)
register_resource_tools(136-396)find_in_file(354-396)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(507-526)🪛 Ruff (0.13.1)
tests/test_edit_normalization_and_noop.py
45-45: Unused method argument:
args(ARG002)
45-45: Unused method argument:
kwargs(ARG002)
46-46: Multiple statements on one line (semicolon)
(E702)
83-83: Unused function argument:
cmd(ARG001)
98-98: Unused function argument:
cmd(ARG001)
98-98: Unused function argument:
params(ARG001)
mcp_source.py
81-82: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_resources_api.py
30-30: Unused method argument:
args(ARG002)
30-30: Unused method argument:
kwargs(ARG002)
tests/test_improved_anchor_matching.py
161-161: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
tools/stress_mcp.py
308-309:
try-except-passdetected, consider logging the exception(S110)
308-308: Do not catch blind exception:
Exception(BLE001)
tests/test_transport_framing.py
188-189:
try-except-passdetected, consider logging the exception(S110)
188-188: Do not catch blind exception:
Exception(BLE001)
test_unity_socket_framing.py
32-32: Consider moving this statement to an
elseblock(TRY300)
33-33: Do not catch blind exception:
Exception(BLE001)
93-94: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (63)
TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef/CustomComponent.cs (1)
9-9: LGTM! Standard formatting improvements.The blank line between serialized fields and the trailing newline at end of file align with common C# formatting conventions.
Also applies to: 18-18
tests/test_edit_normalization_and_noop.py (1)
15-16: LGTM! Formatting changes align with autoformat objective.The changes are purely cosmetic—adding blank lines and splitting long function calls for readability. No logic or behavior is altered.
Also applies to: 17-18, 29-29, 36-36, 38-39, 44-44, 49-49, 72-73, 82-82, 88-89, 91-91, 97-97, 102-103, 111-112, 115-115, 125-126, 128-129
UnityMcpBridge/Editor/Windows/VSCodeManualSetupWindow.cs (5)
16-16: LGTM! Formatting improvement.The blank line improves readability by visually separating the window configuration from the subsequent comment and object initialization.
23-23: LGTM! Formatting improvement.The blank line appropriately separates the
mcpClientinitialization from thewindow.Show()call.
87-87: LGTM! Formatting improvement.The blank line appropriately separates the prerequisites section from the configuration steps section.
105-105: LGTM! Formatting improvement.The blank line appropriately separates the configuration steps from the path location section.
123-123: LGTM! Formatting cleanup.The
elsekeyword formatting has been cleaned up, removing any extraneous whitespace.UnityMcpBridge/Editor/Helpers/PortManager.cs (4)
46-47: LGTM! Improved readability of complex condition.The multi-line formatting makes the compound boolean expression easier to read and maintain.
231-231: LGTM! Better visual separation.The blank line improves readability by separating variable initialization from the conditional logic.
264-264: LGTM! Consistent formatting improvement.The blank line provides consistent visual separation, matching the formatting in
LoadStoredPortmethod.
319-319: LGTM! Standard EOF formatting.The end-of-file newline follows standard formatting conventions.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
15-359: LGTM! Formatting improvements enhance readability.The changes are limited to blank line additions that improve visual separation between logical sections of test methods. No functional changes to test logic or assertions.
tests/test_telemetry_endpoint_validation.py (1)
4-63: LGTM! Formatting aligns with Python conventions.The changes wrap long
importlib.import_modulecalls across multiple lines for better readability and add blank lines to improve visual structure. No functional changes to test logic.UnityMcpBridge/Editor/Helpers/ExecPath.cs (1)
208-208: LGTM! Spacing normalized to standard C# conventions.The change normalizes the spacing around the
+=operator from two spaces to one, aligning with typical C# formatting standards. No functional change to the event handler subscription.UnityMcpBridge/Editor/Tools/ManageShader.cs (1)
342-342: LGTM!End-of-file newline added per standard formatting conventions.
tests/test_telemetry_subaction.py (1)
6-7: LGTM!Multiline formatting improves readability with no behavioral change.
UnityMcpBridge/Editor/AssemblyInfo.cs (1)
3-3: LGTM!Formatting adjustment with no semantic change to the InternalsVisibleTo declaration.
UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (1)
113-113: LGTM!Whitespace formatting adjustments with no functional changes.
Also applies to: 141-141, 266-266
tests/test_logging_stdout.py (1)
67-70: LGTM!Line continuation improves readability of long assertion messages without changing test logic.
UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs (1)
169-184: LGTM! Optional error parameter enhances telemetry.The addition of the optional
errorparameter toRecordToolExecutionallows capturing failure details without breaking existing call sites. The conditional assignment of the error field is handled correctly.UnityMcpBridge/UnityMcpServer~/src/server.py (2)
1-1: LGTM! Telemetry imports align with broader integration.The addition of telemetry imports (
record_telemetry,record_milestone,RecordType,MilestoneType) is consistent with the telemetry enhancements across the codebase, including the TelemetryHelper.cs changes.
25-50: Formatting adjustments improve readability.The multi-line formatting of log path construction, handler instantiation, and logger configuration improves code readability without changing behavior.
tests/test_validate_script_summary.py (1)
1-77: LGTM! Formatting adjustments maintain test functionality.The formatting changes (blank lines, multi-line call wrapping) improve readability without altering test behavior. The test scaffolding setup remains intact and correctly configured.
UnityMcpBridge/Editor/External/Tommy.cs (1)
1-2139: LGTM! Formatting-only changes to external TOML parser.All changes in this file are formatting adjustments (whitespace, line breaks, expression alignment) with no semantic or API modifications. The MIT-licensed Tommy TOML parser functionality remains unchanged.
.github/scripts/mark_skipped.py (3)
33-40: LGTM! Public function extraction improves reusability.Extracting
should_skip()as a public function enables reuse of the skip-pattern logic in other contexts. The pattern matching against environmental/permission failures is appropriate.
43-56: LGTM! Testsuite summary logic is correct.The
summarize_counts()function correctly tallies tests, failures, errors, and skipped cases for JUnit XML testsuite elements.
59-110: LGTM! Main orchestration logic handles XML updates correctly.The
main()function properly:
- Parses JUnit XML and handles parse errors gracefully
- Identifies failing tests matching skip patterns
- Converts failures to skipped entries with appropriate messages
- Recomputes suite tallies after processing
- Only writes changes when modifications occurred
The multi-line formatting for
first_match_textassignment (lines 85-86) and print statement (lines 105-106) improves readability.UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs (7)
122-122: LGTM: Backwards-compatible API enhancement.The new optional parameter
includeNonPublicSerializedFieldswith a default value oftruepreserves existing behavior while adding flexibility. The parameter is correctly threaded through the caching mechanism (line 230) and field filtering logic (lines 266-275).
247-250: LGTM: Correctly handles property overrides.The duplicate check using
Any()ensures only the most derived version of an overridden property is cached, which is the correct behavior for reflection-based serialization.
314-353: LGTM: Robust filtering for problematic Unity properties.The consolidated skip logic using a
skipPropertyflag is clean and maintainable. It correctly filters out:
- Obsolete component shortcuts (
rigidbody,camera, etc.)- Matrix properties prone to cycles
- Camera-specific problematic properties
This defensive approach prevents serialization crashes reported in Unity's API.
462-474: LGTM: Comprehensive JToken type handling.The additional cases for
Date,Guid,Uri,TimeSpan,Bytes, andUndefinedprovide robust coverage of Newtonsoft.Json token types. TreatingUndefinedasnullis a sensible default.
122-122: LGTM: Backward-compatible signature enhancement.Adding the default parameter
includeNonPublicSerializedFields = truepreserves existing behavior while enabling callers to exclude non-public serialized fields when needed. The default aligns with Unity's serialization model.
462-474: Enhanced JToken type coverage improves robustness.The addition of explicit handling for
Date,Guid,Uri,TimeSpan,Bytes, andUndefinedtoken types ensures more comprehensive serialization support. The fallback tojValue.Valuefor unlisted types provides graceful degradation.
247-250: Verify override handling for hidden and explicit-interface propertiesEnsure that properties using C#’s
newmodifier or explicit interface implementations (sameNamebut differentDeclaringType) are correctly skipped or retained by thepropertiesToCache.Any(p => p.Name == propInfo.Name)check. Add targeted test cases—e.g., a component with a hidden property or an explicit-interface property—to confirm only the intended (most-derived or interface) property is cached.UnityMcpBridge/Editor/MCPForUnityBridge.cs (12)
520-552: LGTM: Improved handshake error handling.The restructured handshake logic with explicit try-catch blocks and early returns on failure prevents cascading errors and improves debuggability. The guarded logging prevents exceptions in the error path from causing additional problems.
573-583: LGTM: Efficient ping handling optimization.Handling the
pingcommand before JSON deserialization is a smart optimization for health checks. This reduces latency and CPU overhead for frequent heartbeat/health check operations.
604-621: LGTM: Robust timeout and error response handling.The
Task.WhenAnypattern correctly implements timeout detection without blocking indefinitely. Structured error responses with explicitstatusanderrorfields enable clients to distinguish timeouts from other failures and recover gracefully.
627-652: LGTM: Comprehensive serialization error handling.The guarded serialization path with try-catch blocks, detailed logging, and timing diagnostics significantly improves debuggability and prevents crashes from malformed responses. The IoInfo logging pattern is consistent and helpful for troubleshooting network issues.
810-826: LGTM: Reduced lock contention via snapshot pattern.Moving heartbeat processing outside the lock and using a snapshot of the command queue reduces contention on
lockObj. This improves responsiveness and prevents the heartbeat from being delayed by long-running command processing.
836-879: LGTM: Comprehensive command validation.The layered validation (empty check → ping handling → JSON validation → deserialization) prevents crashes from malformed input and provides clear, structured error responses. This defensive approach significantly improves robustness.
93-105: LGTM: Telemetry integration enhances observability.The addition of
TelemetryHelper.RecordBridgeStartup()andRecordBridgeConnection()calls provides valuable instrumentation for monitoring bridge lifecycle events. Error messages are appropriately truncated (200 chars) to prevent excessive logging.
520-552: Improved error handling in client connection setup.The refactored handshake logic now properly wraps connection setup, Nagle disable, and handshake write in separate try-catch blocks. This provides clearer diagnostics and ensures the client handler exits cleanly on failure. The timeout for handshake write (
FrameIOTimeoutMs) prevents indefinite hangs.
605-611: Structured timeout response improves client error handling.Returning a structured JSON error (
status: "error") when command processing times out allows clients to distinguish timeouts from other failures and implement appropriate retry logic. The timeout duration is exposed in the message for debugging.
628-652: Comprehensive serialization error logging aids diagnostics.The added error logging around response serialization (
IoInfocalls) captures sequence numbers, lengths, and durations for both successful writes and failures. This provides valuable forensics for diagnosing protocol-level issues without impacting the happy path.
810-826: Snapshot-based command processing reduces lock contention.Moving the queue snapshot outside the processing loop (lines 820-825) minimizes the time spent holding
lockObj, reducing contention when commands arrive at high frequency. TheListcopy is cheap and avoids holding the lock during potentially long-running command execution.
836-879: Robust command validation prevents crashes from malformed input.The multi-layered validation (empty check, ping special-case, JSON validation via
IsValidJson, null-command check) ensures that malformed or unexpected input doesn't crash the bridge. Each failure path returns a structured error, maintaining protocol consistency.UnityMcpBridge/Editor/Tools/ManageGameObject.cs (6)
25-37: LGTM: Efficient shared serializer pattern.Creating a single
InputSerializerinstance with all necessary converters avoids repeated allocations and ensures consistent serialization behavior across all command processing operations.
1696-1763: LGTM: Robust property setting with shared serializer.The refactored
SetPropertycorrectly uses the sharedinputSerializerfor conversions, provides detailed error messages with formatted token values, and handles properties, public fields, and[SerializeField]private fields. This covers the common Unity patterns comprehensively.
2430-2512: LGTM: Pragmatic property suggestion system.The
GetAIPropertySuggestionsmethod uses a sensible rule-based approach with caching. The combination of exact matching, word containment, and Levenshtein distance provides useful suggestions without requiring external AI services. The threshold (Math.Max(2, cleanedInput.Length / 4)) at line 2502 balances sensitivity and precision well.
950-1021: Clarify reverse iteration in GetComponentsFromTarget
ManageGameObject.cs (lines 972–986) uses a reverse for-loop andInsert(0)to rebuild the list, but no internal comments or references explain why. Unless this addresses a specific Unity serialization or ordering quirk, please confirm its necessity or simplify by iterating forward and usingList.Add()instead.
30-38: LGTM: Vector2Converter addition completes serializer setup.Adding
Vector2ConvertertoInputSerializerensures consistent handling of Vector2 properties alongside the existing Vector3, Quaternion, and other Unity type converters.
2040-2058: ConvertJTokenToType exceptions are already handled: BothSetProperty(lines 1680–1770) andSetNestedProperty(lines 1770–1870) wrap all calls in atry/catch(Exception), logging errors and returning failure, so no unhandled exceptions will escape..github/workflows/claude-nl-suite.yml (8)
1-69: LGTM: Workflow setup and MCP installation.The secrets detection, Python environment setup, and MCP server installation logic with fallback paths are well-structured and properly handle different project layouts.
161-220: LGTM: Unity warm-up and persistent bridge startup.The warm-up step ensures the Unity Library is imported before the bridge starts, and the persistent container is properly configured with necessary mounts (workspace, status dir, Unity config/local as read-only). The conditional license handling is correct.
222-275: LGTM: Robust Unity bridge health check.The wait logic is comprehensive with multiple signal sources (status JSON, log markers, container state) and a grace period to allow licensing to settle. Sensitive data is properly redacted in error logs.
278-366: LGTM: MCP configuration and report setup.The MCP client configuration, tool permissions, report directory structure, and bridge status verification are all properly implemented. The bootstrap placeholder in the JUnit skeleton is correctly designed to be replaced later.
460-493: Verify consistency of T coverage assertions.The pre-retry coverage check (lines 426-427) allows fragments in either
reports/orreports/_staging/, and the post-promotion assertion (lines 485-487) also accepts staged fragments. However, the post-retry assertion (lines 467) only checksreports/and fails if any fragment is missing there, without checking_staging/. This inconsistency could cause false failures if the retry writes to staging instead of directly to reports.Consider whether the post-retry assertion should also check
_staging/to align with the other coverage checks.
495-636: LGTM: Test name canonicalization and backfill logic.The canonicalization script properly normalizes test names using filename-derived IDs as the source of truth, preventing misclassification. The backfill step ensures missing tests are represented with failure placeholders for complete reporting.
638-967: LGTM: JUnit assembly, reporting, and cleanup.The JUnit assembly logic robustly handles fragments (including malformed XML), enforces one testcase per fragment, and repairs names as needed. Markdown generation, artifact uploading, and cleanup (Unity stop, license return) are all properly implemented.
150-157: Drop the undefined‐output concern. Referencingsteps.ulf.outputs.okwhen theulfstep is skipped simply expands to an empty string, which evaluates as “false” in both the Bash[[ … == "true" ]]checks and the downstreamULF_OKenv var logic (§ steps context, missing keys → empty string). (docs.github.com)Likely an incorrect or invalid review comment.
tests/test_read_resource_minimal.py (1)
1-73: LGTM: Formatting improvements.The import reorganization (moving
register_resource_toolsto the top), additional blank lines for readability, and multi-line function call formatting are all consistent with best practices. No logic changes detected.tests/test_resources_api.py (1)
1-87: LGTM: Test fixture and formatting improvements.The addition of
register_resource_toolsimport and fixture call aligns with the pattern in other test files, establishing consistent resource tool registration. The multi-line formatting improves readability.Note: The static analysis warning about unused
*args, **kwargsin thetoolmethod (line 30) is a false positive—these parameters are required for the decorator pattern to accept arbitrary arguments from callers.tests/test_find_in_file_minimal.py (1)
1-50: LGTM: Resource tools fixture and formatting.The new
resource_toolsfixture (lines 25-29) establishes consistent test infrastructure matching the pattern in other test files. The import reorganization and multi-line formatting improve code clarity. No logic changes detected.
| @@ -1,3 +1,4 @@ | |||
| from unity_connection import UnityConnection | |||
There was a problem hiding this comment.
Restore deferred UnityConnection import to keep dynamic path discovery working
Importing UnityConnection before we modify sys.path defeats the dynamic lookup just below; in clean test environments the module won’t be importable yet, so this now raises ModuleNotFoundError. Move the import back to after the sys.path.insert to keep the tests runnable.
-from unity_connection import UnityConnection
import sys
import json
...
sys.path.insert(0, str(SRC))
-from unity_connection import UnityConnection
+from unity_connection import UnityConnection📝 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.
| from unity_connection import UnityConnection | |
| # tests/test_transport_framing.py | |
| import sys | |
| import json | |
| ... | |
| # ensure our src directory is on the import path before loading UnityConnection | |
| sys.path.insert(0, str(SRC)) | |
| from unity_connection import UnityConnection | |
| ... |
🤖 Prompt for AI Agents
In tests/test_transport_framing.py around lines 1 to 1, the file currently
imports UnityConnection before the test modifies sys.path which breaks dynamic
path discovery and causes ModuleNotFoundError in clean test environments; move
the "from unity_connection import UnityConnection" statement down so it occurs
after the sys.path.insert that adds the project/test path, ensuring the import
happens only once the path is adjusted and leaving dynamic module lookup intact.
| if (commandText.Trim() == "ping") | ||
| { | ||
| // Direct response to ping without going through JSON parsing | ||
| byte[] pingResponseBytes = System.Text.Encoding.UTF8.GetBytes( | ||
| /*lang=json,strict*/ | ||
| "{\"status\":\"success\",\"result\":{\"message\":\"pong\"}}" | ||
| ); | ||
| await WriteFrameAsync(stream, pingResponseBytes); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Optimize ping fast-path to avoid redundant JSON parsing.
The special-case handling for bare "ping" commands bypasses JSON deserialization entirely, improving latency for health checks. Consider documenting this optimization in the protocol specification, as it creates an expectation that ping must be sent as raw text (not JSON).
🌐 Web query:
Does the unity-mcp protocol specification document the expected format for ping commands?
💡 Result:
Yes. The MCP specification documents ping as a JSON‑RPC 2.0 request with method "ping" (no params) and an expected empty-result response (e.g., request {"jsonrpc":"2.0","id":"...","method":"ping"} and response {"jsonrpc":"2.0","id":"...","result":{}}). [1][2]
Sources:
[1] MCP Ping specification (message format & behavior).
[2] MCP protocol message-format reference (JSON‑RPC ping example).
Ensure ping fast-path conforms to JSON-RPC spec
The MCP protocol spec defines ping as a JSON-RPC 2.0 request with method "ping" and an empty-result response; the current fast-path bypasses JSON parsing for bare "ping" text, deviating from spec. Either adjust the fast-path to accept proper JSON-RPC ping messages or update the protocol specification to reflect this behavior.
| /// <summary> | ||
| /// Finds a specific UnityEngine.Object based on a find instruction JObject. | ||
| /// Primarily used by UnityEngineObjectConverter during deserialization. | ||
| /// </summary> | ||
| // Made public static so UnityEngineObjectConverter can call it. Moved from ConvertJTokenToType. | ||
| public static UnityEngine.Object FindObjectByInstruction(JObject instruction, Type targetType) | ||
| { | ||
| string findTerm = instruction["find"]?.ToString(); | ||
| string method = instruction["method"]?.ToString()?.ToLower(); | ||
| string componentName = instruction["component"]?.ToString(); // Specific component to get | ||
|
|
||
| if (string.IsNullOrEmpty(findTerm)) | ||
| { | ||
| Debug.LogWarning("Find instruction missing 'find' term."); | ||
| return null; | ||
| } | ||
|
|
||
| // Use a flexible default search method if none provided | ||
| string searchMethodToUse = string.IsNullOrEmpty(method) ? "by_id_or_name_or_path" : method; | ||
|
|
||
| // If the target is an asset (Material, Texture, ScriptableObject etc.) try AssetDatabase first | ||
| if (typeof(Material).IsAssignableFrom(targetType) || | ||
| typeof(Texture).IsAssignableFrom(targetType) || | ||
| typeof(ScriptableObject).IsAssignableFrom(targetType) || | ||
| targetType.FullName.StartsWith("UnityEngine.U2D") || // Sprites etc. | ||
| typeof(AudioClip).IsAssignableFrom(targetType) || | ||
| typeof(AnimationClip).IsAssignableFrom(targetType) || | ||
| typeof(Font).IsAssignableFrom(targetType) || | ||
| typeof(Shader).IsAssignableFrom(targetType) || | ||
| typeof(ComputeShader).IsAssignableFrom(targetType) || | ||
| typeof(GameObject).IsAssignableFrom(targetType) && findTerm.StartsWith("Assets/")) // Prefab check | ||
| { | ||
| // Try loading directly by path/GUID first | ||
| UnityEngine.Object asset = AssetDatabase.LoadAssetAtPath(findTerm, targetType); | ||
| if (asset != null) return asset; | ||
| asset = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(findTerm); // Try generic if type specific failed | ||
| if (asset != null && targetType.IsAssignableFrom(asset.GetType())) return asset; | ||
|
|
||
|
|
||
| // If direct path failed, try finding by name/type using FindAssets | ||
| string searchFilter = $"t:{targetType.Name} {System.IO.Path.GetFileNameWithoutExtension(findTerm)}"; // Search by type and name | ||
| string[] guids = AssetDatabase.FindAssets(searchFilter); | ||
|
|
||
| if (guids.Length == 1) | ||
| { | ||
| asset = AssetDatabase.LoadAssetAtPath(AssetDatabase.GUIDToAssetPath(guids[0]), targetType); | ||
| if (asset != null) return asset; | ||
| } | ||
| else if (guids.Length > 1) | ||
| { | ||
| Debug.LogWarning($"[FindObjectByInstruction] Ambiguous asset find: Found {guids.Length} assets matching filter '{searchFilter}'. Provide a full path or unique name."); | ||
| // Optionally return the first one? Or null? Returning null is safer. | ||
| return null; | ||
| } | ||
| // If still not found, fall through to scene search (though unlikely for assets) | ||
| } | ||
|
|
||
|
|
||
| // --- Scene Object Search --- | ||
| // Find the GameObject using the internal finder | ||
| GameObject foundGo = FindObjectInternal(new JValue(findTerm), searchMethodToUse); | ||
|
|
||
| if (foundGo == null) | ||
| { | ||
| // Don't warn yet, could still be an asset not found above | ||
| // Debug.LogWarning($"Could not find GameObject using instruction: {instruction}"); | ||
| return null; | ||
| } | ||
|
|
||
| // Now, get the target object/component from the found GameObject | ||
| if (targetType == typeof(GameObject)) | ||
| { | ||
| return foundGo; // We were looking for a GameObject | ||
| } | ||
| else if (typeof(Component).IsAssignableFrom(targetType)) | ||
| { | ||
| Type componentToGetType = targetType; | ||
| if (!string.IsNullOrEmpty(componentName)) | ||
| { | ||
| Type specificCompType = FindType(componentName); | ||
| if (specificCompType != null && typeof(Component).IsAssignableFrom(specificCompType)) | ||
| { | ||
| componentToGetType = specificCompType; | ||
| } | ||
| else | ||
| { | ||
| Debug.LogWarning($"Could not find component type '{componentName}' specified in find instruction. Falling back to target type '{targetType.Name}'."); | ||
| } | ||
| } | ||
|
|
||
| Component foundComp = foundGo.GetComponent(componentToGetType); | ||
| if (foundComp == null) | ||
| { | ||
| Debug.LogWarning($"Found GameObject '{foundGo.name}' but could not find component of type '{componentToGetType.Name}'."); | ||
| } | ||
| return foundComp; | ||
| } | ||
| else | ||
| { | ||
| Debug.LogWarning($"Find instruction handling not implemented for target type: {targetType.Name}"); | ||
| return null; | ||
| } | ||
| } | ||
| if (asset != null) return asset; | ||
| asset = AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(findTerm); // Try generic if type specific failed | ||
| if (asset != null && targetType.IsAssignableFrom(asset.GetType())) return asset; | ||
|
|
||
|
|
||
| // If direct path failed, try finding by name/type using FindAssets | ||
| string searchFilter = $"t:{targetType.Name} {System.IO.Path.GetFileNameWithoutExtension(findTerm)}"; // Search by type and name | ||
| string[] guids = AssetDatabase.FindAssets(searchFilter); | ||
|
|
||
| if (guids.Length == 1) | ||
| { | ||
| asset = AssetDatabase.LoadAssetAtPath(AssetDatabase.GUIDToAssetPath(guids[0]), targetType); | ||
| if (asset != null) return asset; | ||
| } | ||
| else if (guids.Length > 1) | ||
| { | ||
| Debug.LogWarning($"[FindObjectByInstruction] Ambiguous asset find: Found {guids.Length} assets matching filter '{searchFilter}'. Provide a full path or unique name."); | ||
| // Optionally return the first one? Or null? Returning null is safer. | ||
| return null; | ||
| } | ||
| // If still not found, fall through to scene search (though unlikely for assets) | ||
| } | ||
|
|
||
|
|
||
| // --- Scene Object Search --- | ||
| // Find the GameObject using the internal finder | ||
| GameObject foundGo = FindObjectInternal(new JValue(findTerm), searchMethodToUse); | ||
|
|
||
| if (foundGo == null) | ||
| { | ||
| // Don't warn yet, could still be an asset not found above | ||
| // Debug.LogWarning($"Could not find GameObject using instruction: {instruction}"); | ||
| return null; | ||
| } | ||
|
|
||
| // Now, get the target object/component from the found GameObject | ||
| if (targetType == typeof(GameObject)) | ||
| { | ||
| return foundGo; // We were looking for a GameObject | ||
| } | ||
| else if (typeof(Component).IsAssignableFrom(targetType)) | ||
| { | ||
| Type componentToGetType = targetType; | ||
| if (!string.IsNullOrEmpty(componentName)) | ||
| { | ||
| Type specificCompType = FindType(componentName); | ||
| if (specificCompType != null && typeof(Component).IsAssignableFrom(specificCompType)) | ||
| { | ||
| componentToGetType = specificCompType; | ||
| } | ||
| else | ||
| { | ||
| Debug.LogWarning($"Could not find component type '{componentName}' specified in find instruction. Falling back to target type '{targetType.Name}'."); | ||
| } | ||
| } | ||
|
|
||
| Component foundComp = foundGo.GetComponent(componentToGetType); | ||
| if (foundComp == null) | ||
| { | ||
| Debug.LogWarning($"Found GameObject '{foundGo.name}' but could not find component of type '{componentToGetType.Name}'."); | ||
| } | ||
| return foundComp; | ||
| } | ||
| else | ||
| { | ||
| Debug.LogWarning($"Find instruction handling not implemented for target type: {targetType.Name}"); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM: Well-structured object finder for deserializer.
The new FindObjectByInstruction method provides a comprehensive search strategy (assets first via AssetDatabase, then scene objects via FindObjectInternal) with good error handling for ambiguous results. The method correctly extracts components when needed.
Verify that UnityEngineObjectConverter correctly calls this method:
🏁 Script executed:
#!/bin/bash
# Find calls to FindObjectByInstruction
rg "FindObjectByInstruction" --type cs -C 3Length of output: 1541
🏁 Script executed:
#!/bin/bash
# Locate UnityEngineObjectConverter class and check if it invokes FindObjectByInstruction
rg -n "class UnityEngineObjectConverter" -C3 --type cs
rg -n "ConvertJTokenToType" -C3 --type csLength of output: 9999
🏁 Script executed:
#!/bin/bash
# Check if FindObjectByInstruction is referenced in the UnityEngineObjectConverter
rg -n "FindObjectByInstruction" -C3 --type cs UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs
# Locate and inspect the ReadJson implementation to see how objects are deserialized
rg -n "override .*ReadJson" -C3 --type cs UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.csLength of output: 2714
Integrate FindObjectByInstruction into UnityEngineObjectConverter.ReadJson
The new FindObjectByInstruction method is defined but never invoked by UnityEngineObjectConverter. Update the ReadJson implementation in UnityMcpBridge/Runtime/Serialization/UnityTypeConverters.cs (around line 223) to call ManageGameObject.FindObjectByInstruction(jo, objectType) when deserializing UnityEngine.Object.
|
It's just a whitespace change to avoid PRs being riddled with whitespace changes, making them harder to read. PEP8 and MS C# formatting should be standard in any case |
Summary by CodeRabbit
New Features
Improvements
Telemetry