Material tools: support direct shader property keys + add EditMode coverage #344
Conversation
- MCPMaterialTests.cs: Tests for material creation, assignment, and data reading - MCPParameterHandlingTests.cs: Tests for JSON parameter parsing issues - SphereMaterialWorkflowTests.cs: Tests for complete sphere material workflow These tests document the current issues with: - JSON parameter parsing in manage_asset and manage_gameobject tools - Material creation with properties - Material assignment to GameObjects - Material component data reading All tests currently fail (Red phase of TDD) and serve as specifications for what needs to be fixed in the MCP system.
- Removed redundant tests that verify working functionality (GameObjectSerializer, Unity APIs) - Kept focused tests that document the real issue: MCP tool parameter validation - Tests now clearly identify the root cause: JSON string parsing in MCP tools - Tests specify exactly what needs to be fixed: parameter type flexibility The issue is NOT in Unity APIs or serialization (which work fine), but in MCP tool parameter validation being too strict.
- Update _try_probe_unity_mcp to recognize Unity bridge welcome message - Unity bridge sends 'WELCOME UNITY-MCP' instead of JSON pong response - Maintains backward compatibility with JSON pong format - Fixes MCP server connection to Unity Editor
…ol schemas for JSON-string params
…; add EditMode tests
…t; return clear error
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis PR introduces JSON parameter coercion for MCP tool payloads across C# and Python implementations, enabling flexible property and component assignment. It adds direct material property binding via shader property aliases and updates connection validation for the port discovery mechanism. Changes
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant C_Tool as C# Tool<br/>(ManageAsset/ManageGameObject)
participant Unity as Unity Editor
Client->>C_Tool: Send command with<br/>properties/componentProperties<br/>as JSON string
activate C_Tool
C_Tool->>C_Tool: Coerce JSON string<br/>to dict/JObject
alt JSON Parse Success
C_Tool->>C_Tool: Apply property aliases<br/>(_Color → _BaseColor, etc.)
C_Tool->>C_Tool: Extract values<br/>(colors, floats, textures)
C_Tool->>Unity: Set shader properties<br/>on material/component
Unity-->>C_Tool: Success
else JSON Parse Failure
C_Tool->>C_Tool: Log warning,<br/>use defaults
end
deactivate C_Tool
C_Tool-->>Client: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple files with heterogeneous purposes (tool logic, tests, configuration), but follow consistent patterns for JSON coercion. The material property alias mapping adds moderate logic density. Test coverage is comprehensive but straightforward. Configuration updates and meta-file changes are mechanical. Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (5)
MCPForUnity/UnityMcpServer~/src/port_discovery.py (2)
68-68: Consider proper JSON parsing for the pong response check.The substring check
b'"message":"pong"' in datais fragile and assumes exact JSON formatting without spaces. If the Unity bridge formats its JSON response with spaces (e.g.,"message": "pong") or uses different serialization settings, this check will fail.Apply this diff to parse JSON properly:
try: s.sendall(b"ping") data = s.recv(512) - # Check for Unity bridge welcome message format - if data and (b"WELCOME UNITY-MCP" in data or b'"message":"pong"' in data): - return True + # Check for Unity bridge welcome message or JSON pong response + if data: + if b"WELCOME UNITY-MCP" in data: + return True + # Try parsing as JSON for pong response + try: + decoded = data.decode('utf-8') + parsed = json.loads(decoded) + if parsed.get('message') == 'pong': + return True + except (json.JSONDecodeError, UnicodeDecodeError): + pass except Exception: return False
59-60: Update documentation to reflect dual acceptance criteria.The docstring (line 59) and comment (line 67) reference only the "Unity bridge welcome message," but the implementation accepts either the welcome message or a JSON pong response.
Apply this diff to clarify the documentation:
@staticmethod def _try_probe_unity_mcp(port: int) -> bool: """Quickly check if a MCP for Unity listener is on this port. - Tries a short TCP connect, sends 'ping', expects Unity bridge welcome message. + Tries a short TCP connect, sends 'ping', expects either a Unity bridge welcome message or a JSON pong response. """ try: with socket.create_connection(("127.0.0.1", port), PortDiscovery.CONNECT_TIMEOUT) as s: s.settimeout(PortDiscovery.CONNECT_TIMEOUT) try: s.sendall(b"ping") data = s.recv(512) - # Check for Unity bridge welcome message format + # Check for Unity bridge welcome message or JSON pong response if data and (b"WELCOME UNITY-MCP" in data or b'"message":"pong"' in data): return TrueAlso applies to: 67-67
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
1017-1117: Excellent implementation of direct material property assignment!This feature adds significant flexibility while maintaining backward compatibility. Key strengths:
- Reserved keys filter (line 1022): Prevents conflicts with existing structured properties (
shader,color,float,texture)- Shader property aliasing (lines 1025-1044): Handles URP/Standard differences transparently (_Color↔_BaseColor, _MainTex↔_BaseMap, _Glossiness↔_Smoothness)
- Type inference: Correctly infers property types from JSON structure (array→color, number→float, string→texture)
- Defensive coding: Checks
mat.HasProperty()before setting, validates array elements are numeric (line 1053), usesMathf.Approximatelyfor float comparison (line 1087)- Error handling: Try-catch blocks with appropriate warnings
The TODO on line 1118 appropriately notes future enhancements (Vectors, Ints, Keywords, RenderQueue).
Optional enhancement: Consider adding support for boolean properties that map to keywords (e.g.,
_AlphaClip: true→EnableKeyword("_ALPHATEST_ON")), which is common in URP/HDRP shaders:+// Boolean: shader keyword (e.g., _AlphaClip) +if (v.Type == JTokenType.Boolean) +{ + bool b = v.ToObject<bool>(); + // Common keyword patterns + string keyword = shaderProp.ToUpperInvariant() + "_ON"; + if (b && !mat.IsKeywordEnabled(keyword)) + { + mat.EnableKeyword(keyword); + modified = true; + } + else if (!b && mat.IsKeywordEnabled(keyword)) + { + mat.DisableKeyword(keyword); + modified = true; + } + continue; +}This could be deferred to a future PR if keyword handling proves necessary for your use cases.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)
66-97: Consider using "message" field for error reporting.Line 88 accesses
result.Value<string>("error"), but based on the Response pattern used elsewhere, the error message is typically in the"message"field. Consider usingresult.Value<string>("message")for consistency.Apply this diff:
- Assert.IsTrue(result.Value<bool>("success"), result.Value<string>("error")); + Assert.IsTrue(result.Value<bool>("success"), result.Value<string>("message"));The same pattern appears in line 113 and 134.
99-140: Consider extracting material creation into a shared helper.Line 103 creates a test dependency by calling
CreateMaterial_WithObjectProperties_SucceedsAndSetsColor()from within another test. This tight coupling means test failures cascade and makes tests harder to run in isolation.Consider extracting material creation into a private helper method:
private void CreateTestMaterial() { var createParams = new JObject { ["action"] = "create", ["path"] = _matPath, ["assetType"] = "Material", ["properties"] = new JObject { ["shader"] = "Universal Render Pipeline/Lit", ["color"] = new JArray(0f, 0f, 1f, 1f) } }; var result = ToJObject(ManageAsset.HandleCommand(createParams)); Assert.IsTrue(result.Value<bool>("success"), result.Value<string>("message")); }Then call
CreateTestMaterial()from both tests instead of having test interdependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
MCPForUnity/UnityMcpServer~/src/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
MCPForUnity/Editor/Tools/ManageAsset.cs(2 hunks)MCPForUnity/Editor/Tools/ManageGameObject.cs(1 hunks)MCPForUnity/UnityMcpServer~/src/port_discovery.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(4 hunks)TestProjects/UnityMCPTests/Assets/Materials.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Packages/manifest.json(2 hunks)TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json(0 hunks)TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt(1 hunks)
💤 Files with no reviewable changes (1)
- TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json
🧰 Additional context used
🧬 Code graph analysis (5)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
UnityEngine(2177-2274)HandleCommand(42-202)Color(2124-2137)GameObject(1284-1307)MCPForUnity/Editor/Tools/ManageAsset.cs (3)
CreateFolder(295-348)HandleCommand(46-126)DeleteAsset(510-538)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs (1)
MCPForUnity/Editor/Tools/ManageAsset.cs (4)
CreateFolder(295-348)DeleteAsset(510-538)CreateAsset(162-293)HandleCommand(46-126)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
tests/test_helpers.py (1)
info(3-4)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
tests/test_helpers.py (1)
info(3-4)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
Color(2124-2137)
🪛 Ruff (0.14.1)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py
42-42: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (21)
TestProjects/UnityMCPTests/Packages/manifest.json (2)
4-4: Verify necessity of newcom.unity.ai.navigationdependency.The new dependency at line 4 was added as part of this PR, but the PR objectives focus on material property assignment and direct shader keys. Clarify whether this dependency is required by the new test or test infrastructure, or if it was added incidentally.
Is
com.unity.ai.navigation1.1.4 actually required by the new EditMode tests or feature implementation? If not, consider removing it to keep dependencies minimal.
13-13: Clarify reason for timeline version bump.Line 13 updates
com.unity.timelinefrom 1.6.5 to 1.7.5. The PR objectives do not mention timeline changes. Confirm whether this bump is necessary for the material property assignment feature or test infrastructure.Is the timeline version bump required by the new tests, or is it unrelated to this PR's scope? If unrelated, consider reverting to keep this PR focused.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (1)
137-142: Excellent version compatibility handling!The use of
Enum.TryParseto conditionally check for the Trae enum value is a solid approach for supporting multiple package versions. The test will gracefully skip when Trae is unavailable and run normally when it exists, preventing compilation issues across versions.TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1)
1-2: Unity version update looks good.The project has been upgraded to Unity 2022.3.6f1, which aligns with the test suite expansions mentioned in the PR objectives.
TestProjects/UnityMCPTests/Assets/Materials.meta (1)
2-2: LGTM!Standard Unity meta file with updated GUID. No concerns.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs.meta (1)
1-11: LGTM!Standard Unity meta file for the new MaterialDirectPropertiesTests test class.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs.meta (1)
1-11: LGTM!Standard Unity meta file for the new MCPToolParameterTests test class.
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
69-82: LGTM! Consistent input normalization.The JSON string coercion for
componentPropertiesfollows the same defensive pattern used inManageAsset.csandmanage_asset.py, enabling flexible client payloads while maintaining backward compatibility with direct JObject inputs.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs.meta (1)
1-11: LGTM!Standard Unity meta file for the new MaterialParameterToolTests test class.
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
37-44: LGTM! Consistent JSON coercion pattern.The input normalization for
propertiesenables flexible client payloads (JSON strings or dicts) and matches the pattern in the C# implementations. The broad exception catch is appropriate here for defensive input handling.Note: The static analysis warning about catching blind
Exceptionon line 42 is acceptable in this context, as the coercion is intentionally defensive and the error is logged appropriately.MCPForUnity/Editor/Tools/ManageAsset.cs (1)
66-79: LGTM! Consistent input normalization.The JSON string coercion for
propertiesmatches the pattern inManageGameObject.csandmanage_asset.py, providing a unified interface for flexible client payloads.MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (2)
1-1: LGTM: JSON coercion support added correctly.The addition of the
jsonimport and the updated type annotation forcomponent_propertiesto accept bothdictandstrenables flexible client compatibility while maintaining backward compatibility.Also applies to: 46-51
117-126: LGTM: JSON coercion logic is well-implemented.The coercion logic appropriately handles:
- String-to-dict conversion via JSON parsing
- Clear error messages for malformed JSON
- Type validation after coercion
- Informative logging for successful coercion
The placement before parameter dictionary construction ensures the coerced value is used consistently downstream.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs (3)
11-54: LGTM: Test lifecycle management is robust.The test class properly implements setup and teardown with:
- GUID-based unique asset paths to prevent test conflicts
- Defensive cleanup in both SetUp and TearDown
- Proper folder creation and AssetDatabase refresh calls
56-87: LGTM: Helper methods are well-designed.The helper methods provide clean test utilities:
TryDeleteAssetwith defensive cleanup (both AssetDatabase and file system)CreateSolidTextureAssetfor test fixture creationToJObjectfor safe result conversion
89-154: LGTM: Comprehensive test coverage for direct property keys and aliases.The test effectively validates:
- Creation with direct property keys (
_Color,_Glossiness)- Modification using aliased property names (
_BaseColor,_Smoothness)- Texture assignment via direct property keys
- Property persistence across creation and modification
- Shader property alias resolution (Standard ↔ URP)
The verification logic appropriately handles property name variations and uses proper float comparison with tolerance.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (3)
21-68: LGTM: Effective test for JSON properties coercion.The test properly validates the ManageAsset JSON string coercion pathway:
- Uses a JSON string for the
propertiesparameter (line 43)- Verifies successful material creation and property application
- Includes proper cleanup in a finally block
70-119: LGTM: Comprehensive test for componentProperties JSON coercion.The test validates:
- Material creation as a prerequisite
- GameObject creation via ManageGameObject
- JSON string coercion for
componentProperties(lines 99-106)- Material assignment through the coerced properties
The test properly exercises the full workflow with appropriate cleanup.
121-169: LGTM: Good integration test for JSON coercion across tools.This test validates the end-to-end workflow with JSON coercion in both ManageAsset and ManageGameObject, ensuring consistent behavior across the toolset. While similar to the previous test, it serves as a valuable integration test.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)
17-59: LGTM: Robust test lifecycle with defensive cleanup.The SetUp and TearDown methods implement thorough cleanup:
- GUID-based unique naming for test isolation
- Both AssetDatabase and file system cleanup in SetUp
- Proper disposal of GameObjects and assets in TearDown
142-163: LGTM: Important test for EditMode material serialization behavior.This test correctly validates that:
- Material serialization doesn't instantiate materials (critical for EditMode)
- Instance IDs remain unchanged before and after serialization
- Serialized data includes material references
The comprehensive check for material-related keys (sharedMaterial, material, sharedMaterials, or materials) ensures robustness across different serialization scenarios.
Note: This test also has the test dependency issue mentioned in the previous comment.
| { | ||
| "dependencies": { | ||
| "com.coplaydev.unity-mcp": "file:../../../MCPForUnity", | ||
| "com.coplaydev.unity-mcp": "file:/Users/davidsarno/unity-mcp/MCPForUnity", |
There was a problem hiding this comment.
Remove machine-specific absolute path before merging.
Line 3 uses an absolute path (file:/Users/davidsarno/unity-mcp/MCPForUnity) that is specific to a single developer's machine. This will break the build for all other developers and CI pipelines that clone the repository.
Revert to the relative path:
- "com.coplaydev.unity-mcp": "file:/Users/davidsarno/unity-mcp/MCPForUnity",
+ "com.coplaydev.unity-mcp": "file:../../../MCPForUnity",📝 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.
| "com.coplaydev.unity-mcp": "file:/Users/davidsarno/unity-mcp/MCPForUnity", | |
| "com.coplaydev.unity-mcp": "file:../../../MCPForUnity", |
🤖 Prompt for AI Agents
In TestProjects/UnityMCPTests/Packages/manifest.json around line 3, the
dependency uses a machine-specific absolute file path
("file:/Users/davidsarno/...") which will break other developers and CI; change
that entry to a relative file path (e.g., "file:../path/to/MCPForUnity" or the
repository-relative path used by the project) or use the package version from
the registry, ensuring the manifest contains a portable path before merging.
…st; keep upstream elsewhere
1 participant
Summary
Add flexible direct property assignment for materials:
_Color/_BaseColor,_EmissionColorvia[r,g,b,(a)]_Smoothness/_Glossiness,_Metallic,_OcclusionStrength,_Cutoff,_BumpScale,_Parallax,_AlphaClip,_Cull, etc._BaseMap/_MainTex,_BumpMap,_OcclusionMap,_MetallicGlossMap,_EmissionMapvia asset paths_Color↔_BaseColor,_MainTex↔_BaseMap,_Glossiness↔_Smoothness)shader,color,float,textureTests
MaterialDirectPropertiesTests:Why
Enables straightforward, LLM-friendly payloads for material creation/modification without requiring nested structures. Improves ergonomics for automation and external MCP clients.
Notes
propertiesfor material create/modifycomponentPropertiesCloses @#318
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes