fix: JSON material property handling + tests (manage_asset) #90 #349
Conversation
…ercion and end-to-end; update test project manifest and ProjectVersion
…ly; resolve _BaseMap/_MainTex automatically and apply when missing name
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…red texture (supports 'albedo'); tests: self-sufficient texture asset and _BaseColor/_Color guards; python: assert success in invalid JSON case
…valid-json case tolerant; ensure prefab modify test patches transport correctly
WalkthroughThis PR enhances material property handling in ManageAsset with case-insensitive, alias-aware shader property resolution and texture assignment, adds comprehensive unit and integration tests for JSON parsing and material operations across C# and Python, updates project infrastructure including a local package reference and Unity version downgrade, and improves test mocking strategies for async operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ManageAsset as ManageAsset.ApplyMaterialProperties
participant PropResolver as Property Name Resolver
participant TextureLoader as Texture Loader
participant Material as Material
Caller->>ManageAsset: Apply properties dict
loop For each property
ManageAsset->>PropResolver: Resolve key (case-insensitive, alias-aware)<br/>(e.g., "albedo" → "_BaseMap")
PropResolver-->>ManageAsset: Canonical shader property
alt Texture Property
ManageAsset->>TextureLoader: Resolve texture path<br/>from top-level or nested "texture"
TextureLoader-->>ManageAsset: Loaded texture or null
ManageAsset->>Material: Assign texture to<br/>resolved shader property
else Float/Color Property
ManageAsset->>Material: Assign value to<br/>resolved shader property
end
end
ManageAsset-->>Caller: Material updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve mixed complexity across heterogeneous file types: case-insensitive property resolution logic with alias mapping in ManageAsset requires careful reasoning; seven new C# test methods follow established patterns but cover diverse scenarios (JSON parsing, textures, floats, end-to-end); new Python test infrastructure is straightforward but spans multiple test files; test mocking updates involve subtle closure-level patching requiring focused attention. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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 (7)
.github/workflows/unity-tests.yml (1)
4-4: Good addition; consider small CI hardening (optional).Add:
- pull_request trigger to validate PRs,
- a concurrency group to cancel superseded runs,
- minimal permissions.
Example:
on: + pull_request: + branches: [main] workflow_dispatch: {} push: branches: [main] paths: - TestProjects/UnityMCPTests/** - MCPForUnity/Editor/** - .github/workflows/unity-tests.yml + +concurrency: + group: unity-tests-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: readtests/test_script_tools.py (1)
181-186: Use monkeypatch.setitem for globals to avoid leakage across tests.Direct assignment persists beyond the test. Use monkeypatch so it auto-reverts.
-# Patch both at the module and at the function closure location -monkeypatch.setattr(tools_manage_asset, - "async_send_command_with_retry", fake_async) -# Also patch the globals of the function object (handles dynamically loaded module alias) -manage_asset.__globals__["async_send_command_with_retry"] = fake_async +# Patch both at the module and at the function closure location +monkeypatch.setattr(tools_manage_asset, "async_send_command_with_retry", fake_async) +monkeypatch.setitem(manage_asset.__globals__, "async_send_command_with_retry", fake_async)MCPForUnity/Editor/Tools/ManageAsset.cs (3)
992-1031: Structured texture block is robust; minor enhancement suggestion.Case-insensitive keys and default to _BaseMap look good. Consider also accepting a plain string under "texture" (treat as path, defaulting name to _BaseMap) for symmetry with direct assignments.
-// Example: Set texture property (case-insensitive key and subkeys) +// Example: Set texture property (case-insensitive key and subkeys; string path supported) { - JObject texProps = null; + JObject texProps = null; + // Allow: "texture": "Assets/.." + var textureToken = properties.Properties() + .FirstOrDefault(p => string.Equals(p.Name, "texture", StringComparison.OrdinalIgnoreCase))?.Value; + if (textureToken != null && textureToken.Type == JTokenType.String) + { + texProps = new JObject { ["path"] = textureToken }; + } - var direct = properties.Property("texture"); + var direct = properties.Property("texture"); if (direct != null && direct.Value is JObject t0) texProps = t0; ... }
1041-1066: Alias resolver: add a few common shader aliases.Add HDRP/Standard texture aliases so friendly names resolve more often.
switch (lower) { case "_color": candidates = new[] { "_Color", "_BaseColor" }; break; case "_basecolor": candidates = new[] { "_BaseColor", "_Color" }; break; case "_maintex": candidates = new[] { "_MainTex", "_BaseMap" }; break; case "_basemap": candidates = new[] { "_BaseMap", "_MainTex" }; break; case "_glossiness": candidates = new[] { "_Glossiness", "_Smoothness" }; break; case "_smoothness": candidates = new[] { "_Smoothness", "_Glossiness" }; break; // Friendly names → shader property names case "metallic": candidates = new[] { "_Metallic" }; break; case "smoothness": candidates = new[] { "_Smoothness", "_Glossiness" }; break; case "albedo": candidates = new[] { "_BaseMap", "_MainTex" }; break; + case "maskmap": candidates = new[] { "_MaskMap", "_MetallicGlossMap" }; break; // HDRP/Standard packed map + case "metallicglossmap": candidates = new[] { "_MetallicGlossMap", "_MaskMap" }; break; // Standard/HDRP + case "occlusion": candidates = new[] { "_OcclusionMap" }; break; + case "normal": candidates = new[] { "_BumpMap", "_NormalMap" }; break; default: candidates = new[] { name }; break; }
1067-1137: Direct assignments: warn on missing textures and reuse aliasing in structured blocks.
- Emit a warning if a direct texture path fails to load (parity with structured block).
- Reuse ResolvePropertyName in structured "float"/"color" too so aliases like "smoothness"/"_BaseColor" work consistently.
// Float: single number if (v.Type == JTokenType.Float || v.Type == JTokenType.Integer) { - if (mat.HasProperty(shaderProp)) + if (mat.HasProperty(shaderProp)) { try { float f = v.ToObject<float>(); if (!Mathf.Approximately(mat.GetFloat(shaderProp), f)) { mat.SetFloat(shaderProp, f); modified = true; } } catch (Exception ex) { Debug.LogWarning($"Error setting float '{shaderProp}': {ex.Message}"); } } continue; } // Texture: string path if (v.Type == JTokenType.String) { string texPath = v.ToString(); if (!string.IsNullOrEmpty(texPath) && mat.HasProperty(shaderProp)) { var tex = AssetDatabase.LoadAssetAtPath<Texture>(AssetPathUtility.SanitizeAssetPath(texPath)); - if (tex != null && mat.GetTexture(shaderProp) != tex) + if (tex != null && mat.GetTexture(shaderProp) != tex) { mat.SetTexture(shaderProp, tex); modified = true; } + else if (tex == null) + { + Debug.LogWarning($"Texture not found at path: {texPath}"); + } } continue; }Additionally, near the structured blocks above:
- string propName = colorProps["name"]?.ToString() ?? "_Color"; + string propName = ResolvePropertyName(colorProps["name"]?.ToString() ?? "_Color"); - string propName = floatProps["name"]?.ToString(); + string propName = ResolvePropertyName(floatProps["name"]?.ToString());tests/test_json_parsing_simple.py (1)
14-20: Optional: Prefer explicitelsefor clarity.The
returnstatement at line 20 is only reached whenpropertiesis not a string. Per the static analysis hint, consider moving it into an explicitelseblock to make the control flow more apparent.Apply this diff:
if isinstance(properties, str): try: parsed = json.loads(properties) return parsed, "success" except json.JSONDecodeError as e: return properties, f"failed to parse: {e}" - return properties, "no_parsing_needed" + else: + return properties, "no_parsing_needed"TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (1)
466-697: LGTM! Comprehensive end-to-end validation of all property handling scenarios.This integration test thoroughly validates all 10 scenarios described in the PR objectives: JSON string parsing, friendly name aliases (
metallic,smoothness,albedo), structured blocks, direct texture assignments, GameObject material assignment, pipeline-specific properties, invalid JSON handling, shader switching, and mixed key types.The test design is solid:
- Uses unique GUID suffix to avoid collisions
- Tolerance-based color comparisons (line 665) handle pipeline variance
- Tests skip gracefully when optional textures are missing (lines 547-563, 566-585)
- Proper cleanup ensures no test pollution
Optional: Consider splitting into smaller focused tests.
At 230+ lines, this test is at the upper limit for maintainability. If a scenario fails, it can be harder to pinpoint which of the 10 sub-tests caused the issue. Consider extracting each scenario into a separate test method, sharing setup/cleanup via fixtures or helper methods. This would:
- Improve test isolation and failure diagnosis
- Allow running individual scenarios independently
- Make the test suite easier to maintain as scenarios evolve
However, as an integration test that validates the full flow, the current structure is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/unity-tests.yml(1 hunks)MCPForUnity/Editor/Tools/ManageAsset.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs(6 hunks)TestProjects/UnityMCPTests/Packages/manifest.json(1 hunks)TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt(1 hunks)tests/test_json_parsing_simple.py(1 hunks)tests/test_manage_asset_json_parsing.py(1 hunks)tests/test_script_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_script_tools.py (2)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(16-92)UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py (1)
manage_asset(15-83)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (3)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
GameObject(1284-1307)HandleCommand(42-202)Color(2124-2137)UnityEngine(2177-2274)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialDirectPropertiesTests.cs (3)
Test(89-154)JObject(84-87)Texture2D(72-82)MCPForUnity/Editor/Tools/ManageAsset.cs (4)
HandleCommand(46-127)DeleteAsset(511-539)CreateFolder(296-349)CreateAsset(163-294)
tests/test_manage_asset_json_parsing.py (1)
tests/test_helpers.py (2)
info(3-4)warning(6-7)
🪛 Ruff (0.14.1)
tests/test_json_parsing_simple.py
17-17: Consider moving this statement to an else block
(TRY300)
tests/test_manage_asset_json_parsing.py
22-22: Unused function argument: cmd
(ARG001)
22-22: Unused function argument: params
(ARG001)
22-22: Unused function argument: loop
(ARG001)
49-49: Unused function argument: cmd
(ARG001)
49-49: Unused function argument: params
(ARG001)
49-49: Unused function argument: loop
(ARG001)
72-72: Unused function argument: cmd
(ARG001)
72-72: Unused function argument: params
(ARG001)
72-72: Unused function argument: loop
(ARG001)
97-97: Unused function argument: cmd
(ARG001)
97-97: Unused function argument: params
(ARG001)
97-97: Unused function argument: loop
(ARG001)
127-127: Unused function argument: cmd
(ARG001)
127-127: Unused function argument: params
(ARG001)
🔇 Additional comments (13)
TestProjects/UnityMCPTests/ProjectSettings/ProjectVersion.txt (1)
1-2: Version aligns with CI matrix.EditorVersion 2021.3.45f2 matches unity-tests.yml matrix. No action.
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
91-92: LGTM: pass parsed properties to ModifyAsset.Keeps parsing centralized and avoids re-reading raw tokens.
TestProjects/UnityMCPTests/Packages/manifest.json (1)
3-3: Local package path verified and resolves correctly.The relative path
../../../MCPForUnityresolves to the MCPForUnity directory at the repository root, and the directory exists. No UPM import failures will occur from path resolution issues.tests/test_json_parsing_simple.py (1)
26-108: LGTM! Comprehensive test coverage.The test suite covers the key scenarios: valid JSON parsing, invalid JSON handling, dict/None passthrough, complex nested structures, empty objects, and malformed edge cases. The assertions are clear and appropriate.
tests/test_manage_asset_json_parsing.py (2)
14-112: LGTM! Mock signatures are intentionally complete.The tests properly verify JSON string coercion behavior and logging. The static analysis warnings about unused function arguments in the mock functions (lines 22, 49, 72, 97) are false positives—mocks need to match the expected signature of
async_send_command_with_retryeven if they don't use all parameters.
119-143: LGTM! Synchronous mock matches manage_gameobject's transport.The test correctly uses a synchronous mock for
send_command_with_retry, which aligns withmanage_gameobject's usage pattern. The static analysis warnings about unused arguments (line 127) are false positives for the same reason as above.TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs (7)
22-32: LGTM! Good cross-pipeline shader validation.The helper centralizes shader validation and accepts the common fallback shaders across URP, HDRP, and Built-in pipelines. This makes the tests resilient to pipeline-specific defaults.
65-67: LGTM! Shader verification strengthens existing tests.Adding shader assertions to existing tests ensures that material creation and modification operations preserve the expected shader. This catches regressions in shader handling.
Also applies to: 124-134, 185-189
200-238: LGTM! Clean test for JSON string material creation.The test validates JSON string parsing during material creation, uses unique GUIDs to avoid collisions, and has proper cleanup. The assertions verify both shader and color properties.
240-291: LGTM! Validates JSON parsing for modify operations.The test covers the modify path by creating a material and then modifying it via JSON string properties. The two-step verification ensures the modification was applied correctly.
293-325: LGTM! Proper validation of error handling.The test verifies that invalid JSON is handled gracefully without crashing. Using
LogAssert.Expectwith a regex pattern is appropriate for matching warning messages, and the flexible assertion accepts either success with defaults or graceful failure.
327-384: LGTM! Thorough float property validation with tolerance.The test validates the
metallic→_Metallicalias mapping for both create and modify operations. Using tolerance-based comparison (1e-3f) and checkingHasPropertybefore access are good practices for shader property tests.
386-464: LGTM! Flexible texture assignment test handles pipeline variance.The test validates texture assignment via both
_BaseMap(URP) and_MainTex(Standard) aliases. Creating the texture inline if missing makes the test self-sufficient. The dual property check (hasTexturelogic at lines 435-437 and 452-454) appropriately handles shader differences across pipelines.
1 participant
ManageAsset:metallic→_Metallic,smoothness↔_Smoothness/_Glossiness,albedo↔_BaseMap/_MainTex"texture"key regardless of casing"name"/"Name"and"path"/"Path"_BaseMap/_MainTexautomatically{"_BaseMap": "Assets/.../Tex.asset"}Tests
MCPToolParameterTests:_Metalliccreate/modify testMisc
TestProjects/UnityMCPTests/Packages/manifest.jsonProjectSettings/ProjectVersion.txtfor test projectResult
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores