feat: Add manage_material tool for dedicated material manipulation
#440
Conversation
- Add ManageMaterial tool for creating and modifying materials - Add MaterialOps helper for material property operations - Add comprehensive test suite for material management - Add string parameter parsing support for material properties - Update related tools (ManageGameObject, manage_asset, etc.) - Add test materials and scenes for material testing
- Move and logic from to - Update to delegate to - Update to use enhanced for creation and property setting - Add texture path loading support to
…n manage_gameobject modify action
- Fix Python server tools (redundant imports, exception handling, string formatting) - Clean up documentation and error reports - Improve ManageMaterial.cs (overwrite checks, error handling) - Enhance MaterialOps.cs (robustness, logging, dead code removal) - Update tests (assertions, unused imports) - Fix manifest.json relative path - Remove temporary test artifacts and manual setup scripts
…cp into feat/manage-material-tool * 'feat/manage-material-tool' of github.com:dsarno/unity-mcp: Remove unnecessary SceneTemplateSettings.json
WalkthroughAdds a MaterialOps utility for applying JSON-defined material properties, a ManageMaterial editor tool, server-side manage_material and centralized JSON parsing support, refactors ManageAsset/ManageGameObject to use MaterialOps, and adds extensive Unity editor tests for material workflows and parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant Unity
Note over Client,Server: Client requests tool action with properties (string or object)
Client->>Server: POST /tool (action, properties, ...)
Server->>Server: parse_json_payload(properties) => normalized params
Server->>Unity: send command (action + normalized params) [async/retry]
Unity->>Unity: ManageMaterial.HandleCommand parses params
Unity->>Unity: MaterialOps.ApplyProperties / TrySetShaderProperty resolves aliases, parses colors/textures
Unity->>Unity: Material/Renderer updated (AssetDatabase / Undo as needed)
Unity-->>Server: response dict {status, message, data}
Server-->>Client: forward response
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 (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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
214-220: Serializer inconsistency:CreateDefault()lacks custom Unity converters.
MaterialOps.ApplyPropertiesis called withJsonSerializer.CreateDefault(), which doesn't include the custom converters (Vector3Converter, ColorConverter, etc.) thatManageGameObject.InputSerializerprovides. This could cause deserialization failures for complex property types.Consider reusing the shared serializer from
ManageGameObject:- MaterialOps.ApplyProperties(mat, propertiesForApply, Newtonsoft.Json.JsonSerializer.CreateDefault()); + MaterialOps.ApplyProperties(mat, propertiesForApply, ManageGameObject.InputSerializer);
440-445: Same serializer inconsistency in ModifyAsset.This has the same issue as the create path - using
CreateDefault()instead of the shared serializer with Unity type converters.- modified |= MaterialOps.ApplyProperties(material, properties, Newtonsoft.Json.JsonSerializer.CreateDefault()); + modified |= MaterialOps.ApplyProperties(material, properties, ManageGameObject.InputSerializer);
🧹 Nitpick comments (12)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
1-10: Remove unused imports.
System.IOandSystem.Linqappear to be unused in this test file.using System; -using System.IO; using System.Collections.Generic; -using System.Linq; using Newtonsoft.Json.Linq; using NUnit.Framework; using UnityEditor;Server/tests/integration/test_manage_asset_json_parsing.py (1)
35-37: Tighten behavior assertions and loosen log-string couplingTwo small suggestions for the JSON-parsing tests:
- For
manage_assetlogging (Line 36), you currently assert exact membership inctx.log_info:assert "manage_asset: coerced properties using centralized parser" in ctx.log_infoGiven
ctx.log_infois a collection of messages and you already use substring checks elsewhere, consider matching more defensively to tolerate prefixes/suffixes or structured logging:assert any( "manage_asset: coerced properties using centralized parser" in msg for msg in ctx.log_info )This reduces brittleness if logging formatting changes slightly.
- For
manage_gameobject(Lines 115–143), the test no longer asserts that JSON parsing actually occurred; it only checkssuccessfrommanage_gameobject. Since you're monkeypatchingasync_send_command_with_retry, you can also assert thatcomponent_propertiesreached Unity as adict, which directly validatesparse_json_payloadbehavior. For example:captured_params = {} async def fake_send(cmd, params, **kwargs): captured_params.update(params) return {"success": True, "message": "GameObject created successfully"} monkeypatch.setattr( "services.tools.manage_gameobject.async_send_command_with_retry", fake_send, ) # ...call manage_gameobject... assert isinstance(captured_params.get("component_properties"), dict)This keeps the test resilient to log-message wording while still verifying the JSON coercion path.
Also applies to: 115-143
Server/src/services/tools/utils.py (1)
5-5: parse_json_payload is safe and useful; minor doc tweak possibleThe helper is a good, conservative JSON parser for loosely-typed inputs: it avoids touching non-JSON-looking strings, uses
json.loadsonly when appropriate, and returns the original value on failure, which is ideal for tolerant tooling.Minor nit: the docstring emphasizes “object/list” results, but the implementation also intentionally parses scalar JSON values (
true/false/nulland numeric literals). If that behavior is desired (it seems useful for booleans and numerics), consider explicitly mentioning scalars in the docstring to align expectations with behavior.Also applies to: 27-59
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (1)
72-191: Stress tests exercise key ManageMaterial paths; consider tightening namingThe three tests do a good job of covering:
- Error reporting for bad paths, bad color arrays, and out-of-range slots.
- PropertyBlock isolation vs shared-material color.
- End-to-end assignment then shared-material color modification.
One minor clarity nit:
Integration_WithManageGameObject_AssignsMaterialAndModifiesonly callsManageMaterial.HandleCommandand doesn’t involvemanage_gameobject. Renaming the test (or updating the comment) to reflect that it’s purely aManageMaterialintegration flow would avoid confusion for future readers.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (1)
99-123: Flexible error message assertion may become fragile.The assertion on lines 121-122 checks for multiple possible error message patterns (
"Path","line","position","End of input"). While this handles different Newtonsoft.Json versions, consider usingStringAssert.Containswith the most stable expected substring, or verifying just that the message is non-empty and different from the generic prefix.- Assert.IsTrue(msg.Contains("Path") || msg.Contains("line") || msg.Contains("position") || msg.Contains("End of input"), - $"Message should contain details. Got: {msg}"); + // Verify the message contains more than just the prefix (has exception details) + Assert.IsTrue(msg.Length > "Invalid JSON".Length, + $"Message should contain exception details. Got: {msg}");TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (1)
40-43: Consider extractingToJObjecthelper to a shared test utilities class.This helper is duplicated across
ManageMaterialPropertiesTests,ManageMaterialReproTests,MaterialParameterToolTests, andReadConsoleTests. Extracting to a sharedTestHelpersclass would reduce duplication.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (1)
47-68: Remove dead code comments in TearDown.Lines 49-67 contain extensive comments discussing implementation approaches but the actual cleanup logic for
Assets/Tempis never executed. Theifblock starting at line 47 does nothing.Either implement the cleanup logic or remove the dead code:
// Clean up parent Temp folder if it's empty if (AssetDatabase.IsValidFolder("Assets/Temp")) { - // Check if empty using AssetDatabase (sub-assets) - var guids = AssetDatabase.FindAssets("", new[] { "Assets/Temp" }); - // FindAssets returns the folder itself too usually, or contents? - // Actually Directory.GetFiles/GetDirectories is fine for checking emptiness of a folder on disk, - // but AssetDatabase.DeleteAsset("Assets/Temp") is safer if we know it's empty. - // The review suggested using AssetDatabase consistently. - // Let's check sub-folders via AssetDatabase? - // Actually, if we just want to remove if empty, we can try to delete and catch, or check existence of sub items. - // But let's stick to the reviewer's point: "using AssetDatabase APIs consistently would be more robust". - // We can't easily check "is empty" with AssetDatabase without listing assets. - // So I will stick to the logic but use AssetDatabase for deletion (which it already does). - // Wait, lines 50-51 use Directory.GetDirectories. - // I will assume the reviewer wants us to avoid System.IO.Directory. - // I'll skip this change if it's too complex to do purely with AssetDatabase without a helper, - // OR I can just try to delete it and let Unity handle it if it's not empty? No, Unity deletes recursively. - // So checking emptiness is important. - // I will just use Directory.GetFiles but with Application.dataPath relative path? - // The reviewer said "using AssetDatabase APIs consistently". - // I'll leave it for now or use `AssetDatabase.GetSubFolders`. + // Only delete if empty + var subFolders = AssetDatabase.GetSubFolders("Assets/Temp"); + if (subFolders.Length == 0) + { + AssetDatabase.DeleteAsset("Assets/Temp"); + } }MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
307-319: Undo may not revert material instantiation as noted, but consider warning the user.The comment at line 312 correctly notes that Undo cannot fully revert material instantiation. Consider returning a warning in the response to inform the caller that the operation creates a persistent material instance.
if (slot >= 0 && slot < renderer.materials.Length) { Material mat = renderer.materials[slot]; // Note: Undo cannot fully revert material instantiation Undo.RecordObject(mat, "Set Instance Material Color"); if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color); else mat.SetColor("_Color", color); - return new { status = "success", message = "Set instance material color" }; + return new { status = "success", message = "Set instance material color", warning = "Material instance created; Undo cannot fully revert instantiation." }; }Server/src/services/tools/manage_material.py (1)
58-68: Slot coercion is overly complex for edge cases.The slot coercion logic handles
"0"(digit string) and potentially"0"as JSON. Thejson.loads(slot)at line 66 would fail for"0"since it's not valid JSON (it needs to be just0without quotes in the string). The first checkslot.isdigit()already handles numeric strings, so the JSON fallback is unlikely to trigger meaningfully.Consider simplifying:
# Coerce slot to int if it's a string if slot is not None: if isinstance(slot, str): - if slot.isdigit(): + try: slot = int(slot) - else: - # Try parsing if it's a JSON number string - try: - slot = int(json.loads(slot)) - except (json.JSONDecodeError, ValueError, TypeError): - pass # Let it fail downstream or keep as string if that was intended (though C# expects int) + except ValueError: + pass # Let it fail downstream; C# expects intMCPForUnity/Editor/Helpers/MaterialOps.cs (2)
80-83: Silent error swallowing may hide issues.The empty
catch { }block at line 82 silently ignores parsing errors for color values. Consider at minimum logging a warning to aid debugging when color parsing fails unexpectedly.try { Color newColor = ParseColor(colorArr, serializer); if (mat.HasProperty(propName) && mat.GetColor(propName) != newColor) { mat.SetColor(propName, newColor); modified = true; } } - catch { } + catch (Exception ex) + { + Debug.LogWarning($"[MaterialOps] Failed to parse color array: {ex.Message}"); + }
222-246: Multiple silent catch blocks obscure failures.Lines 228, 229, 236, and 243 all have empty
catch { }blocks that silently swallow exceptions. While the intent is to try multiple conversions, completely silent failures make debugging difficult. The warning at line 292-294 only triggers if all attempts fail.Consider logging at debug level when individual conversions fail, or at minimum ensure the final warning includes context about what was attempted.
Server/src/services/tools/manage_gameobject.py (1)
96-126: LGTM on the parsing refactor.The updated
_coerce_vecfunction correctly prioritizes the centralizedparse_json_payloadfor JSON arrays while maintaining backward compatibility with legacy comma/space-separated string formats.Nit (optional): The
import mathon line 103 is inside the function and will be executed on every call. Consider moving it to the module level for a minor efficiency improvement, though this is pre-existing code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)MCPForUnity/Editor/Helpers/MaterialOps.cs.meta(1 hunks)MCPForUnity/Editor/Tools/ManageAsset.cs(2 hunks)MCPForUnity/Editor/Tools/ManageGameObject.cs(3 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs(1 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs.meta(1 hunks)Server/src/services/tools/manage_asset.py(2 hunks)Server/src/services/tools/manage_gameobject.py(8 hunks)Server/src/services/tools/manage_material.py(1 hunks)Server/src/services/tools/read_console.py(2 hunks)Server/src/services/tools/script_apply_edits.py(4 hunks)Server/src/services/tools/utils.py(2 hunks)Server/tests/integration/test_manage_asset_json_parsing.py(2 hunks)TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs.meta(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/manage_asset.py
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageGameObject.cs
🧬 Code graph analysis (11)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (1)
JObject(71-74)MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
HandleCommand(14-55)
Server/src/services/tools/manage_material.py (3)
Server/src/services/tools/utils.py (1)
parse_json_payload(27-59)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(78-103)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (3)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (2)
JObject(40-43)Test(45-70)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (3)
JObject(51-54)Test(16-30)Test(32-49)MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
HandleCommand(14-55)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
ReadConsole(38-126)
Server/src/services/tools/script_apply_edits.py (1)
Server/src/services/tools/utils.py (1)
parse_json_payload(27-59)
Server/src/services/tools/manage_asset.py (1)
Server/src/services/tools/utils.py (1)
parse_json_payload(27-59)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
MaterialOps(12-340)TrySetShaderProperty(201-296)
Server/src/services/tools/manage_gameobject.py (3)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(78-103)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)Server/src/services/tools/utils.py (2)
coerce_bool(11-24)parse_json_payload(27-59)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (2)
MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
HandleCommand(14-55)MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
Color(301-339)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
JObject(51-54)MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
HandleCommand(14-55)
🔇 Additional comments (39)
TestProjects/UnityMCPTests/Assets/Scripts/Editor.meta (1)
1-8: No review needed for auto-generated metadata.This is a standard Unity folder metadata file generated automatically by the editor. No manual code review is required.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs.meta (1)
1-11: LGTM!Standard Unity-generated meta file with valid GUID for the new test asset.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs.meta (1)
1-11: LGTM!Standard Unity-generated meta file with valid GUID for the repro test asset.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs.meta (1)
1-11: LGTM!Standard Unity-generated meta file with valid GUID for the ReadConsole test asset.
Server/src/services/tools/read_console.py (1)
102-112: LGTM!The refactored stacktrace stripping logic properly handles both the standard
{"data": {"lines": [...]}}format and the legacy direct list format. The defensive checks withisinstanceand the use ofpopwith a default value make this robust against varying response structures.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (3)
16-30: LGTM!Clean test following arrange-act-assert pattern with helpful diagnostic output on assertion failure.
51-54: LGTM!The helper method cleanly handles both
JObjectresults and other object types that need serialization.
32-49: Verify the expected data structure format.The test asserts
result["data"]is aJArray, but ensure this matches the actual return format ofReadConsole.HandleCommand. If the Python server code expects{"data": {"lines": [...]}}(wheredatacontains a nestedlineskey), verify consistency between the C# handler and Python expectations.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs.meta (1)
1-11: Unity test .meta looks standardMonoImporter configuration is conventional and poses no issues.
MCPForUnity/Editor/Helpers/MaterialOps.cs.meta (1)
1-11: MaterialOps .meta is fineStandard MonoImporter metadata for the new helper script; no concerns.
MCPForUnity/Editor/Tools/ManageMaterial.cs.meta (1)
1-11: ManageMaterial .meta is validMeta file matches the usual pattern for editor tools; nothing to change.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs.meta (1)
1-11: Stress-test .meta is standardMeta for
ManageMaterialStressTests.csis conventional; no action needed.Server/src/services/tools/manage_asset.py (1)
12-13: Centralized JSON parsing integration looks solidThe
_normalize_propertieschanges cleanly delegate toparse_json_payloadfirst and only fall back to the legacyjson.loads/ast.literal_evalpath when needed, preserving existing error messages while avoidingliteral_evalfor well-formed JSON. Logging of both the centralized path and the fallback source (JSONvsPython literal) remains clear and should keep diagnostics intact.Also applies to: 59-79
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (1)
18-65: SetUp/TearDown and temp-asset handling look robustThe test fixture’s setup/teardown correctly creates a dedicated temp root, persists the material asset, and cleans up both the cube and
Assets/Temponly when empty. This should keep the test environment isolated and avoid polluting the project.MCPForUnity/Editor/Tools/ManageGameObject.cs (3)
25-38: Visibility change tointernalenables cross-module reuse.Widening
InputSerializerfromprivatetointernalallowsMaterialOpsand other helpers to share the same configured serializer instance, avoiding duplication. This is a sensible change for the centralized material handling approach.
58-69: Usability improvement: aliasingname→targetfor non-create actions.This is a helpful UX enhancement. However, note that mutating
@params["target"](line 67) has a side effect on the caller's JObject. This is intentional for downstream methods but worth documenting.Consider adding a brief inline comment clarifying this is intentional mutation for downstream consistency.
2124-2127: Good refactor: Centralizing shader property handling viaMaterialOps.Delegating to
MaterialOps.TrySetShaderPropertyremoves duplicated property-setting logic and ensures consistent behavior acrossManageGameObject,ManageAsset, andManageMaterial. The serializer is correctly passed for type conversions.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs (3)
10-42: Well-structured test fixture with proper setup/teardown.The test class properly creates and cleans up temporary asset folders. The
ToJObjecthelper normalizes results for consistent assertions. Good foundation for the property tests.
43-81: Good coverage for valid JSON input formats.These tests verify that both JSON string arrays (
"{\"_Color\": [1.0, 0.0, 0.0, 1.0]}") and nativeJObjectarrays work correctly for setting material properties. The color assertions (Color.red,Color.green) confirm the properties are actually applied.
125-146: Null property handling test is appropriately defensive.Testing that null property values don't crash the system is important. The assertion allowing either
"success"or"error"(but not a crash/internal error) is a reasonable approach for graceful degradation.MCPForUnity/Editor/Tools/ManageAsset.cs (1)
378-435: New GameObject/Prefab component modification pathway looks correct.The logic properly:
- Iterates component properties from the JSON
- Uses
ComponentResolver.TryResolvefor type resolution- Falls back to string-based lookup
- Logs appropriate warnings for missing components
- Aggregates modification state with
|=Good defensive programming with the warnings for unresolved components.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialReproTests.cs (2)
10-43: Test fixture structure is sound.Good use of GUID-based unique paths (
Guid.NewGuid().ToString("N")) to ensure test isolation across parallel runs or repeated executions.
45-70: Good regression test for detailed error messaging.The test verifies that invalid JSON produces an error with detailed exception information (line 69 ensures the message contains more than just the generic prefix). This is useful for debugging malformed inputs.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialParameterToolTests.cs (2)
117-153: Test correctly migrated to use newManageMaterialtool.The refactored test:
- Uses the dedicated
assign_material_to_rendereraction instead of genericmodify- Provides explicit
materialPathandslotparameters- Asserts on
status == "success"matching the new tool's response formatThis aligns with the PR's goal of separating material logic from
ManageGameObject.
155-176: Dependent test correctly updated to call renamed method.The
ReadRendererData_DoesNotInstantiateMaterial_AndIncludesSharedMaterialtest properly calls the renamed setup method. The rest of the test logic remains unchanged, verifying that serialization doesn't instantiate materials.Server/src/services/tools/script_apply_edits.py (1)
375-379: LGTM! Good defensive parsing for stringified JSON.The addition of
parse_json_payloadhandling aligns with the centralized JSON parsing pattern used by other tools in this PR (e.g.,manage_material.py). The validation ensures a list is always passed downstream.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs (2)
76-107: Good test coverage for shader property differences.The test correctly handles both URP (
_BaseColor) and Standard (_Color) shader variations, checking the material's shader name before setting the appropriate property. The assertion logic also adapts based on the actual shader in use.
134-168: Good use of try/finally for runtime object cleanup.The test properly cleans up the runtime
GameObjectin thefinallyblock to avoid polluting other tests.MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
434-442: Apply properties before saving to ensure changes persist.Properties are applied after
CreateAssetat line 434 but beforeSaveAssetsat line 442. TheEditorUtility.SetDirty(material)at line 441 should ensure the changes are saved. This flow looks correct.Server/src/services/tools/manage_material.py (2)
70-73:action.lower()may be unnecessary given Literal constraint.The
actionparameter is annotated withLiteral[...]containing lowercase values. If clients adhere to the type hint,.lower()is redundant. However, keeping it provides defensive normalization for clients that may not respect the type hints.
88-91: LGTM! Proper async delegation pattern.The function correctly uses
send_with_unity_instancewithasync_send_command_with_retry, following the established pattern from other tools in the codebase.MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
154-178: Good alias resolution for cross-pipeline compatibility.The
ResolvePropertyNamemethod provides robust mapping between common property aliases (e.g.,_Color↔_BaseColor,_MainTex↔_BaseMap) and handles both shader property names and friendly names like"metallic"and"albedo". This supports the PR's goal of smart alias resolution.
301-339: ParseColor handles multiple formats robustly.The method correctly handles:
- Stringified JSON arrays
- 3-element arrays (RGB with alpha defaulting to 1)
- 4-element arrays (RGBA)
- Falls back to serializer for object formats
Server/src/services/tools/manage_gameobject.py (6)
1-9: LGTM!The imports are correctly updated to include
Unionfor the updated type annotations andparse_json_payloadfrom the centralized utilities module. This aligns with the broader refactoring toward shared JSON parsing utilities.
17-17: Good defensive pattern for handling missing action.Making
actionOptional with explicit validation provides a better user experience with a clear error message instead of a cryptic type error.
28-33: LGTM!The
Union[list[float], str]type annotations accurately document that these vector parameters accept both native lists and JSON strings, improving clarity for MCP clients.
49-54: LGTM!The Union type for
component_propertiesproperly documents that it accepts both dict objects and JSON strings.
140-145: LGTM!Clean simplification using the centralized
parse_json_payloadutility with proper type validation afterward.
240-241: LGTM!The
{e!s}formatting is a clean standardization of exception string conversion.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (2)
14-47: Strengthen assertions to validate console behavior, not just shapeBoth tests currently only assert
success(and thatdatais aJArray), without checking that the console was actually cleared or that the logged message made it through the pipeline. This makes the tests pass even if the underlying implementation partially breaks.Consider tightening the assertions, e.g.:
- In
HandleCommand_Clear_Works, call agetbefore and afterclear(or inspect the returned payload ifclearalready returns a count) and assert that the entries are reduced/empty after clear.- In
HandleCommand_Get_Works, use theDebug.Log("Test Log Message")setup to assert that at least one entry exists and ideally that one of the returned log entries’messagefield contains"Test Log Message".This will better guard against regressions in the reflection layer around
UnityEditor.LogEntriesand ensure you’re testing observable behavior, not just the contract surface.
49-52: Handle nullresultinToJObjectfor clearer test failures
ToJObjectwill throw viaJObject.FromObject(result)ifHandleCommandever returnsnull, which can obscure the failure cause.You could make null handling explicit and keep assertion messages clearer:
- private static JObject ToJObject(object result) - { - return result as JObject ?? JObject.FromObject(result); - } + private static JObject ToJObject(object result) + { + if (result == null) + { + Assert.Fail("ReadConsole.HandleCommand returned null."); + return new JObject(); // Unreachable, but satisfies return type. + } + + return result as JObject ?? JObject.FromObject(result); + }This keeps the helper convenient while giving a precise error if the contract is broken.
Server/tests/integration/test_manage_asset_json_parsing.py (1)
144-157: Remove dead comment block or complete the test.Lines 144-157 contain an extended comment explaining limitations but no actual assertion. The test
test_component_properties_json_string_parsingeffectively only assertsresult["success"] is Truewithout verifying that JSON parsing occurred. Sincetest_component_properties_parsing_verification(lines 158-181) already properly tests this behavior with captured params, consider removing the redundant/incomplete test or consolidating them.@pytest.mark.asyncio async def test_component_properties_json_string_parsing(self, monkeypatch): - """Test that JSON string component_properties are correctly parsed.""" + """Test that JSON string component_properties result in successful operation.""" from services.tools.manage_gameobject import manage_gameobject ctx = DummyContext() async def fake_send(cmd, params, **kwargs): return {"success": True, "message": "GameObject created successfully"} monkeypatch.setattr( "services.tools.manage_gameobject.async_send_command_with_retry", fake_send, ) # Test with JSON string component_properties result = await manage_gameobject( ctx=ctx, action="create", name="TestObject", component_properties='{"MeshRenderer": {"material": "Assets/Materials/BlueMaterial.mat"}}' ) - # Verify JSON parsing was logged - # Verify the result assert result["success"] is True - - # Verify that component_properties reached Unity as a dict - # We can't easily check 'captured_params' here without refactoring the test to capture args, - # but since we mocked the transport, we can trust the return value and rely on - # unit tests for parse_json_payload. - # However, to follow the feedback, let's verify implicit behavior or refactor. - # Since I cannot easily monkeypatch a capture variable here without changing the test structure significantly, - # I will rely on the fact that if it wasn't parsed, it would likely fail downstream or be passed as string. - # The feedback suggested: "captured_params = {} ... monkeypatch ... assert isinstance(captured_params...)" - # I'll implement that pattern.MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
11-12: Consider making the class static.All methods are
static, so the class should also be declaredstaticto prevent unnecessary instantiation and align withManageAsset.[McpForUnityTool("manage_material", AutoRegister = false)] - public class ManageMaterial + public static class ManageMaterial
283-293: Redundant slot bounds check.The slot bounds are already validated at lines 275-278, so the check at line 283 (
slot < renderer.sharedMaterials.Length) is always true and can be simplified.- if (slot < renderer.sharedMaterials.Length && renderer.sharedMaterials[slot] != null) + if (renderer.sharedMaterials[slot] != null) { Material mat = renderer.sharedMaterials[slot]; if (mat.HasProperty("_BaseColor")) block.SetColor("_BaseColor", color); else if (mat.HasProperty("_Color")) block.SetColor("_Color", color); else block.SetColor("_Color", color); }Server/src/services/tools/manage_material.py (1)
85-87: Consider formatting for readability.Line 85 is quite long. Consider breaking it across multiple lines for better readability.
- result = await send_with_unity_instance(async_send_command_with_retry, unity_instance, "manage_material", params_dict) + result = await send_with_unity_instance( + async_send_command_with_retry, + unity_instance, + "manage_material", + params_dict + )MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
309-347: Add error handling for serializer fallback.Line 346 uses the serializer as a fallback without error handling. If the token cannot be converted to a Color, this will throw an unhandled exception.
} - - return token.ToObject<Color>(serializer); + + try + { + return token.ToObject<Color>(serializer); + } + catch (Exception ex) + { + Debug.LogWarning($"[MaterialOps] Failed to parse color from token: {ex.Message}"); + return Color.white; // Return sensible default + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)MCPForUnity/Editor/Tools/ManageAsset.cs(2 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs(1 hunks)Server/src/services/tools/manage_gameobject.py(7 hunks)Server/src/services/tools/manage_material.py(1 hunks)Server/src/services/tools/utils.py(2 hunks)Server/tests/integration/test_manage_asset_json_parsing.py(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialTests.cs
🧰 Additional context used
🧬 Code graph analysis (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
ReadConsole(38-126)
Server/src/services/tools/manage_material.py (3)
Server/src/services/tools/utils.py (1)
parse_json_payload(27-60)Server/src/transport/unity_transport.py (1)
send_with_unity_instance(78-103)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)
MCPForUnity/Editor/Tools/ManageMaterial.cs (4)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
HandleCommand(42-217)Type(2465-2479)Color(2308-2321)GameObject(1512-1535)MCPForUnity/Editor/Tools/ManageAsset.cs (2)
HandleCommand(46-127)CreateAsset(163-300)MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (1)
JToken(536-555)MCPForUnity/Editor/Helpers/MaterialOps.cs (5)
MaterialOps(12-348)ResolvePropertyName(157-181)TrySetShaderProperty(204-304)Color(309-347)ApplyProperties(17-152)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
🪛 Ruff (0.14.7)
Server/tests/integration/test_manage_asset_json_parsing.py
165-165: Unused function argument: cmd
(ARG001)
165-165: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (17)
Server/src/services/tools/utils.py (1)
27-60: LGTM! Tolerant JSON parsing utility with sensible fallback.The fast-path heuristic correctly avoids unnecessary parsing attempts while gracefully falling back to the original value on failure. Minor note: scientific notation (e.g.,
"1e5") won't be detected by the fast path, but sincejson.loadswould handle it and failures return the original string, this is acceptable.Server/tests/integration/test_manage_asset_json_parsing.py (1)
158-181: LGTM! Proper verification of parsed parameters.This test correctly captures the parameters sent to the transport and verifies that
componentPropertiesis a dict after parsing. This is the right approach for verifying the centralized parser behavior.MCPForUnity/Editor/Tools/ManageAsset.cs (2)
216-216: LGTM! Clean delegation to centralized MaterialOps.Good refactor to use
MaterialOps.ApplyPropertiesfor material property application, ensuring consistent behavior across the codebase.
444-444: Consistent usage of MaterialOps for modification path.This aligns with the creation path and ensures material modifications use the same centralized logic.
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
14-55: LGTM! Clean command dispatcher with proper error handling.The action routing is well-structured with a catch-all exception handler that returns structured error responses including stack trace for debugging.
186-233: LGTM! Proper slot bounds validation.Good job validating slot bounds at lines 223-226 before accessing the materials array, addressing the pattern mentioned in past review comments.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialStressTests.cs (4)
18-40: LGTM! Well-structured test setup with proper isolation.Good use of GUID for unique test paths, shader fallback for compatibility across render pipelines, and proper asset database operations.
72-113: LGTM! Comprehensive error handling validation.Good coverage of error paths: invalid material path, malformed color array, and out-of-bounds slot index. The assertions properly verify error responses without throwing exceptions.
115-152: LGTM! Important state isolation test.This test properly validates that
property_blockmode doesn't leak color changes to the shared material asset - a critical behavior for the material management tool.
154-191: LGTM! Good integration test for the full workflow.Tests the complete flow of assigning a material and modifying its color, verifying the changes propagate correctly to the renderer.
Server/src/services/tools/manage_material.py (1)
1-17: LGTM!The imports are well-organized, and the tool registration with the
mcp_for_unity_tooldecorator is appropriate.MCPForUnity/Editor/Helpers/MaterialOps.cs (3)
157-181: LGTM!The property name resolution logic correctly handles common aliases with case-insensitive matching and sensible fallback behavior.
186-198: LGTM!The main color property detection logic is well-structured with appropriate fallback behavior.
286-298: Verify Object-to-Texture deserialization behavior.Deserializing a JObject directly to a Unity Texture type (line 290) requires custom JSON converters. With the default serializer, this will likely always fail and fall through to the warning at line 300. Verify whether this code path is intended to work or if it's dead code. Consider either adding custom JSON converters for Texture serialization, or removing this block if texture assignment via objects is not supported.
Server/src/services/tools/manage_gameobject.py (3)
2-3: LGTM!The addition of
Uniontypes andparse_json_payloadimport properly supports the centralized JSON parsing improvements in this PR.Also applies to: 10-10
90-94: Excellent UX improvement!Adding explicit validation for the missing
actionparameter prevents confusing downstream errors and provides clear guidance to users.
96-145: LGTM!The refactoring to use
parse_json_payloadcentralizes JSON parsing logic while maintaining backward compatibility with legacy comma-separated string formats.
|
Does it solve the material problem you mentioned? I was about to tackle it but saw this repo, looks like a big re-structure. |
|
@Scriptwonder I've tested it a bunch and it seems to work pretty well. Not perfect yet because material params are more complex, but even the dumber models seem to be able to figure it out if they get stuck. I'd like to move forward with it for now and see how it goes. It's not a huge change really,just a new smallish tool and removing some of that responsibility from manage_asset. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
Server/tests/integration/test_manage_asset_json_parsing.py (1)
147-170: Excellent addition - this test directly validates the parsing contract.The new test effectively verifies that
component_propertiesJSON strings are parsed to dictionaries before being dispatched to Unity, strengthening the test coverage for the centralized parsing approach.To silence the static analysis warning about unused arguments in
fake_send, consider prefixing them with underscores to indicate they're intentionally unused:- async def fake_send(cmd, params, **kwargs): + async def fake_send(_cmd, params, **_kwargs): captured_params.update(params) return {"success": True, "message": "GameObject created successfully"}TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (2)
43-75: Reduce chance of missing the unique log entry by increasingcount
HandleCommand_Get_Workssearches only the first 10 entries for the unique message. If there’s a burst of logging between theDebug.Logand thegetcall, the message could fall outside that window and make the test flaky.A small tweak like increasing
countmakes the test more robust without changing intent:- ["action"] = "get", - ["count"] = 10 // Fetch enough to likely catch our message + ["action"] = "get", + ["count"] = 1000 // Fetch enough to very likely catch our messageThe rest of the assertions and search loop look solid.
77-86: Helper for coercingHandleCommandresults looks good
ToJObjectgives strong guarantees for the tests: it fails fast onnulland cleanly handles bothJObjectand POCO-style results viaJObject.FromObject. The unreachablereturn new JObject()afterAssert.Failis harmless and can be left as-is for clarity or removed if you prefer strictness.MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
85-116: Texture-instruction branch skips alias resolution andHasPropertychecks.In
SetMaterialShaderProperty, whenvalueis an instruction object you callmat.SetTexture(property, tex)before runningResolvePropertyName. That means aliases like"albedo"or case variants won’t be mapped, and you may set a non‑existent property while returning success.Consider normalizing and validating
propertyup front so both the instruction and fallback paths share the same behavior:- Undo.RecordObject(mat, "Set Material Property"); - - // 1. Try handling Texture instruction explicitly (ManageMaterial special feature) - if (value.Type == JTokenType.Object) + Undo.RecordObject(mat, "Set Material Property"); + + // Normalize alias/casing once for all code paths + property = MaterialOps.ResolvePropertyName(mat, property); + + // 1. Try handling Texture instruction explicitly (ManageMaterial special feature) + if (value.Type == JTokenType.Object) { // Check if it looks like an instruction if (value is JObject obj && (obj.ContainsKey("find") || obj.ContainsKey("method"))) { Texture tex = ManageGameObject.FindObjectByInstruction(obj, typeof(Texture)) as Texture; - if (tex != null) + if (tex != null && mat.HasProperty(property)) { mat.SetTexture(property, tex); EditorUtility.SetDirty(mat); return new { status = "success", message = $"Set texture property {property} on {mat.name}" }; } } } - - // 2. Fallback to standard logic via MaterialOps (handles Colors, Floats, Strings->Path) - property = MaterialOps.ResolvePropertyName(mat, property); + + // 2. Fallback to standard logic via MaterialOps (handles Colors, Floats, Strings->Path) bool success = MaterialOps.TrySetShaderProperty(mat, property, value, ManageGameObject.InputSerializer);
401-438: Consider usingRenderPipelineUtility.ResolveShaderinstead ofShader.Find.
CreateMaterialcurrently callsShader.Find(shaderName)directly, so aliases likelit,urp_lit, etc. and pipeline‑aware fallbacks fromRenderPipelineUtilityare bypassed. Using the helper would make shader selection consistent withMaterialOps.ApplyPropertiesand more robust across pipelines.- Shader shader = Shader.Find(shaderName); + Shader shader = RenderPipelineUtility.ResolveShader(shaderName);MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
227-286: Guard numeric properties withHasPropertyto avoid false “success”.In
TrySetShaderProperty, float/bool branches callmaterial.SetFloat(propertyName, ...)without checkingmaterial.HasProperty(propertyName). If the property doesn’t exist, Unity will log an error but this method still returnstrue, which can mislead callers (e.g.,ApplyPropertiestreating it as modified).For consistency with the color/vector paths and
ResolvePropertyName, consider adding aHasPropertycheck:- else if (value.Type == JTokenType.Float || value.Type == JTokenType.Integer) - { - try { material.SetFloat(propertyName, value.ToObject<float>(serializer)); return true; } + else if (value.Type == JTokenType.Float || value.Type == JTokenType.Integer) + { + if (!material.HasProperty(propertyName)) + return false; + + try { material.SetFloat(propertyName, value.ToObject<float>(serializer)); return true; } catch (Exception ex) { Debug.Log($"[MaterialOps] SetFloat attempt for '{propertyName}' failed: {ex.Message}"); } } else if (value.Type == JTokenType.Boolean) { - try { material.SetFloat(propertyName, value.ToObject<bool>(serializer) ? 1f : 0f); return true; } + if (!material.HasProperty(propertyName)) + return false; + + try { material.SetFloat(propertyName, value.ToObject<bool>(serializer) ? 1f : 0f); return true; } catch (Exception ex) { Debug.Log($"[MaterialOps] SetFloat (bool) attempt for '{propertyName}' failed: {ex.Message}"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs(1 hunks)Server/src/services/tools/manage_material.py(1 hunks)Server/tests/integration/test_manage_asset_json_parsing.py(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
🧬 Code graph analysis (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
ReadConsole(38-126)
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (1)
RenderPipelineUtility(7-194)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
Server/tests/integration/test_manage_asset_json_parsing.py (2)
Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-240)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
MCPForUnity/Editor/Tools/ManageGameObject.cs (4)
UnityEngine(2361-2458)Type(2465-2479)Color(2308-2321)GameObject(1512-1535)MCPForUnity/Editor/Helpers/MaterialOps.cs (5)
MaterialOps(12-384)ResolvePropertyName(160-184)TrySetShaderProperty(207-328)Color(333-383)ApplyProperties(17-155)
🪛 Ruff (0.14.7)
Server/tests/integration/test_manage_asset_json_parsing.py
154-154: Unused function argument: cmd
(ARG001)
154-154: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (3)
Server/tests/integration/test_manage_asset_json_parsing.py (2)
36-39: LGTM!The assertion correctly verifies that the centralized parser was invoked by checking for the expected log message. This aligns with the PR's goal of centralizing JSON parsing.
123-123: LGTM!The updated docstring appropriately focuses on the test outcome rather than implementation details, making it more maintainable.
Server/src/services/tools/manage_material.py (1)
18-67: Async tool wiring and slot coercion look solid.The parameter mapping to C# (
materialPath,searchMethod, etc.), JSON payload parsing viaparse_json_payload, and explicitslotcoercion with a clear error on invalid strings all look correct and should give much clearer failures than deferring to Unity.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
213-224: Optional: log parse/texture-conversion failures for easier debuggingThe
catch { }blocks aroundJToken.Parse(stringified JSON) andvalue.ToObject<Texture>swallow exceptions silently. While the final warning at the end ofTrySetShaderPropertygives some feedback, having at least a lightweight debug-level log when these conversions fail would make diagnosing malformed JSON or unexpected texture payloads much easier.Consider adding a
Debug.Log(or project-standard logging) includingpropertyNameand the exception message in these catches, similar to the color/vector/float branches you already updated.Also applies to: 312-313, 327-327
🧹 Nitpick comments (1)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
15-42: Avoid potential flakiness by not asserting global console emptinessRequiring
entriesAfter == null || entriesAfter.Count == 0assumes no other logs are written afterclear, which might not hold if Unity or other tests log during the window, causing intermittent failures even whenClearworks.Consider asserting that the specific marker log you added is gone instead of asserting the entire console is empty, e.g.:
- // Ensure there's something to clear - Debug.Log("Log to clear"); + // Ensure there's something to clear + const string markerMessage = "Log to clear"; + Debug.Log(markerMessage); @@ - var entriesAfter = getAfter["data"] as JArray; - Assert.IsTrue(entriesAfter == null || entriesAfter.Count == 0, "Console should be empty after clear."); + var entriesAfter = getAfter["data"] as JArray; + if (entriesAfter != null) + { + foreach (var entry in entriesAfter) + { + Assert.IsFalse( + entry["message"]?.ToString().Contains(markerMessage) == true, + "Console should not contain the marker log after clear." + ); + } + }This still validates that
clearremoved the intended entry but is resilient to unrelated logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs(1 hunks)Server/tests/integration/test_manage_asset_json_parsing.py(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Tools/ManageMaterial.cs
🧰 Additional context used
🧬 Code graph analysis (3)
Server/tests/integration/test_manage_asset_json_parsing.py (2)
Server/src/services/tools/manage_gameobject.py (1)
manage_gameobject(16-240)Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (1)
MCPForUnity/Editor/Tools/ReadConsole.cs (1)
ReadConsole(38-126)
🪛 Ruff (0.14.7)
Server/tests/integration/test_manage_asset_json_parsing.py
128-128: Unused function argument: params
(ARG001)
🔇 Additional comments (7)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReadConsoleTests.cs (2)
45-77: Get test is robust and focused on observable behaviorUsing a GUID-backed
uniqueMessage, assertingsuccess, and then searching the returned data for that message gives a clear, behavior-focused test ofget. This looks good as-is.
79-88: Null-guard and normalization helper looks good
ToJObjectfailing fast onnulland normalizing non-JObjectresults viaJObject.FromObjectis a reasonable, defensive helper for these tests.MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
14-155: ApplyProperties, aliasing, and structured formats look solidThe overall flow in
ApplyProperties(shader/color/float/texture structured handling + alias-aware fallback viaResolvePropertyNameandTrySetShaderProperty) is coherent and defensive: null checks,HasPropertychecks, and modification tracking are all in place and align well with the centralized material-handling goal of the PR.
339-389: ParseColor covers key cases; consider extending formats only if needed
ParseColor’s handling of arrays (RGB/RGBA) and delegating to the serializer for object cases is clear and matches the material JSON formats used in this PR. The explicit exception and warning path for invalid arrays is also helpful for callers.If future payloads introduce alternative formats (e.g.,
"r,g,b,a"strings or 0–255 integer channels), they can be added here without touching the higher-level APIs.Server/tests/integration/test_manage_asset_json_parsing.py (3)
35-39: More robust log assertion for JSON coercionUsing
any("manage_asset: coerced properties using centralized parser" in msg for msg in ctx.log_info)is a good change: it makes the test resilient to additional info logs while still verifying that the coercion path was exercised.
148-170: New parsing verification test accurately asserts dict coercion
test_component_properties_parsing_verificationcorrectly captures the params sent to Unity and asserts thatcomponentPropertiesis adict, aligning the test with theparse_json_payloadbehavior inmanage_gameobject. This is a useful safeguard against regressions in the centralized parsing path.
121-161: Clarify whichfake_sendfunction has unused parametersThe review states both helpers have unused
paramsarguments, but the second function's code snippet showscaptured_params.update(params)—indicating the parameter is actively used. If the second function usesparams, it should not be flagged as unused, and renaming it provides no lint benefit. Clarify whether the second function actually uses this parameter or if only the first function needs the rename.
…operty checks - Add comprehensive ManageMaterial tool documentation to MCPForUnity/README.md - Add manage_material to tools list in README.md and README-zh.md - Fix MaterialOps.cs to check HasProperty before SetTexture calls to prevent Unity warnings - Ensures consistency with other property setters in MaterialOps
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
293-328: Avoid empty catch blocks in texture conversion branchesThe string/object texture branches still swallow exceptions silently (
catch { }), unlike the color/vector/float branches above where you now log failures. This can make it hard to understand why a texture assignment failed when the final generic warning at Lines 330–332 isn’t enough.Consider logging at least the exception message and some context:
- else if (value.Type == JTokenType.String) - { - try - { + else if (value.Type == JTokenType.String) + { + try + { string path = value.ToString(); if (!string.IsNullOrEmpty(path) && path.Contains("/")) { var sanitizedPath = AssetPathUtility.SanitizeAssetPath(path); Texture tex = AssetDatabase.LoadAssetAtPath<Texture>(sanitizedPath); if (tex != null && material.HasProperty(propertyName)) { material.SetTexture(propertyName, tex); return true; } } - } - catch { } + } + catch (Exception ex) + { + Debug.LogWarning($"[MaterialOps] SetTexture (string path) for '{propertyName}' failed: {ex.Message}"); + } } … - if (value.Type == JTokenType.Object) - { - try - { + if (value.Type == JTokenType.Object) + { + try + { Texture texture = value.ToObject<Texture>(serializer); if (texture != null && material.HasProperty(propertyName)) { material.SetTexture(propertyName, texture); return true; } - } - catch { } + } + catch (Exception ex) + { + Debug.LogWarning($"[MaterialOps] SetTexture (object) for '{propertyName}' failed: {ex.Message}"); + } }This keeps control flow the same while giving you concrete signals when conversions fail.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
46-69: Restrict structured color arrays to 3 or 4 elements to matchParseColor
ParseColorthrows unless the array has exactly 3 or 4 elements, but the guard at Line 50 currently acceptscolArr.Count >= 3. For counts >4 this always falls into theelsebranch inParseColorand throws only to be caught/logged here.Tightening the check would make intent clearer and avoid work on obviously-invalid input:
- if (colorProps["value"] is JArray colArr && colArr.Count >= 3) + if (colorProps["value"] is JArray colArr && (colArr.Count == 3 || colArr.Count == 4))(Same pattern already exists implicitly in
ParseColor; this just aligns the structured path with it.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)README-zh.md(1 hunks)README.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (1)
RenderPipelineUtility(7-194)MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
AssetPathUtility(15-201)SanitizeAssetPath(20-34)
🔇 Additional comments (3)
MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
17-155: Centralized JSON→material application looks robust and low-noiseThe overall
ApplyPropertiesflow (null-guards, case-insensitive lookup, separation of structured vs direct keys, alias resolution +HasPropertychecks, andmodifiedtracking) is well-structured and should behave predictably across shaders and pipelines.README-zh.md (1)
42-61:manage_materialtool entry matches actual capabilitiesThe new
manage_materialbullet accurately reflects the tool’s responsibilities (creation, property setting, renderer assignment, querying info) and is consistent with the implementation.README.md (1)
42-63: Docs formanage_materialtool are clear and aligned with the implementationThe new
manage_materialdescription concisely captures the supported actions and fits well in the tools list alongside existing entries.
|
Sounds good to me, and looking forward to actually use it during the gamejam (which I am sure we can get a lot of feedback on the tools during it). Can you also edit the readme and specify it? Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
14-55: Add a null guard for@paramsinHandleCommand.
HandleCommandassumes@paramsis non-null; a null call will throw before you hit the existing"Action is required"check. Consider an early guard:public static object HandleCommand(JObject @params) { - string action = @params["action"]?.ToString(); + if (@params == null) + { + return new { status = "error", message = "params object is required" }; + } + + string action = @params["action"]?.ToString();This keeps the tool robust against malformed calls.
128-194: UseResolvePropertyName/GetMainColorPropertyNameto align color property behavior.
SetMaterialColorcurrently:
- Checks the provided
propertyname directly withmat.HasProperty(property).- Falls back only to
_BaseColor/_Color.Elsewhere you already centralize aliasing and main-color detection in
MaterialOps, so this entrypoint will behave differently (e.g.,"albedo"/"basecolor"won’t resolve, and shaders using_MainColor/_Tintare ignored here).You can reuse the helpers to make behavior consistent:
- bool foundProp = false; - if (!string.IsNullOrEmpty(property)) - { - if (mat.HasProperty(property)) - { - mat.SetColor(property, color); - foundProp = true; - } - } - else - { - // Fallback logic: _BaseColor (URP/HDRP) then _Color (Built-in) - if (mat.HasProperty("_BaseColor")) - { - mat.SetColor("_BaseColor", color); - foundProp = true; - property = "_BaseColor"; - } - else if (mat.HasProperty("_Color")) - { - mat.SetColor("_Color", color); - foundProp = true; - property = "_Color"; - } - } + bool foundProp = false; + if (!string.IsNullOrEmpty(property)) + { + // Normalize aliases/casing for explicit property names + property = MaterialOps.ResolvePropertyName(mat, property); + if (mat.HasProperty(property)) + { + mat.SetColor(property, color); + foundProp = true; + } + } + else + { + // Use shared helper to pick the main color slot (_BaseColor, _Color, _Tint, etc.) + property = MaterialOps.GetMainColorPropertyName(mat); + if (mat.HasProperty(property)) + { + mat.SetColor(property, color); + foundProp = true; + } + }This way
manage_materialcolor setting matches the alias and shader‑agnostic behavior you already expose viaMaterialOps.MCPForUnity/Editor/Helpers/MaterialOps.cs (1)
45-111: Consider resolving aliases for structured"color"/"float"names as well.In
ApplyProperties:
- Structured blocks (
"color": { "name": "...", "value": [...] }and"float": { "name": "...", "value": ... }) use the rawnamewhen checkingmat.HasProperty(propName).- Direct properties below already pass all keys through
ResolvePropertyName.That means
"albedo","metallic", etc. work in the direct form but not inside the structured"color"/"float"objects, which is a bit surprising.You could normalize structured names too, e.g.:
// color (structured) - string propName = colorProps["name"]?.ToString() ?? GetMainColorPropertyName(mat); + string rawColorName = colorProps["name"]?.ToString(); + string propName = string.IsNullOrEmpty(rawColorName) + ? GetMainColorPropertyName(mat) + : ResolvePropertyName(mat, rawColorName); // float (structured) - string propName = floatProps["name"]?.ToString(); + string rawFloatName = floatProps["name"]?.ToString(); + string propName = string.IsNullOrEmpty(rawFloatName) + ? null + : ResolvePropertyName(mat, rawFloatName);This keeps alias behavior consistent regardless of whether callers use structured or direct property formats.
Also applies to: 139-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/MaterialOps.cs(1 hunks)MCPForUnity/Editor/Tools/ManageMaterial.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Tools/ManageMaterial.cs
🔇 Additional comments (5)
MCPForUnity/Editor/Tools/ManageMaterial.cs (3)
57-71: Path normalization looks solid and cross‑platform‑friendly.Using
AssetPathUtility.SanitizeAssetPathplus enforcing the.matextension solves the earlier double-Assets/and backslash issues and keeps behavior consistent with other tools.
73-126: Shader property setting flow is well‑structured.Good composition here: you resolve aliases via
MaterialOps.ResolvePropertyName, special‑case{ find/method }texture instructions, and then delegate everything else toMaterialOps.TrySetShaderPropertywith properHasPropertychecks andEditorUtility.SetDirtyon success. This should cover most material workflows cleanly.
459-516: Material creation flow and safety checks look good.
CreateMaterialnow:
- Normalizes/sanitizes the path and explicitly enforces
Assets/forCreateAsset.- Resolves shaders via
RenderPipelineUtility.ResolveShader.- Prevents silent overwrites with
AssetDatabase.LoadAssetAtPath<Material>pre‑check.- Applies initial properties via
MaterialOps.ApplyPropertiesand saves assets.That matches Unity editor best practices for asset creation and keeps behavior predictable.
MCPForUnity/Editor/Helpers/MaterialOps.cs (2)
203-340:TrySetShaderPropertycovers the main JSON → shader cases with good diagnostics.The conversion flow here is comprehensive:
- Handles stringified JSON (
"[...]"/"{...}") by reparsing and recursing.- Uses array length heuristics (4/3/2) for Color/Vector4/Vector2 with
HasPropertyguards.- Treats numbers and booleans as floats only when the property exists.
- Assigns textures from sanitized asset paths and object values, now guarded by
material.HasProperty(propertyName).- Logs detailed per-attempt failures plus a final warning when no conversion succeeds.
This should make property application debuggable without spamming Unity warnings for missing properties.
345-395: Color parsing helper is robust and consistent with the rest of the API.
ParseColor:
- Supports stringified JSON tokens by reparsing into arrays/objects.
- Handles 3- and 4-component arrays with sensible default alpha.
- Falls back to
ToObject<Color>for object forms and logs before rethrowing on failure.This pairs well with both
SetMaterialColorandTrySetShaderPropertyand should be easy to reason about for callers.
| private static object SetRendererColor(JObject @params) | ||
| { | ||
| string target = @params["target"]?.ToString(); | ||
| string searchMethod = @params["searchMethod"]?.ToString(); | ||
| JToken colorToken = @params["color"]; | ||
| int slot = @params["slot"]?.ToObject<int>() ?? 0; | ||
| string mode = @params["mode"]?.ToString() ?? "property_block"; | ||
|
|
||
| if (string.IsNullOrEmpty(target) || colorToken == null) | ||
| { | ||
| return new { status = "error", message = "target and color are required" }; | ||
| } | ||
|
|
||
| Color color; | ||
| try | ||
| { | ||
| color = MaterialOps.ParseColor(colorToken, ManageGameObject.InputSerializer); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return new { status = "error", message = $"Invalid color format: {e.Message}" }; | ||
| } | ||
|
|
||
| var goInstruction = new JObject { ["find"] = target }; | ||
| if (!string.IsNullOrEmpty(searchMethod)) goInstruction["method"] = searchMethod; | ||
|
|
||
| GameObject go = ManageGameObject.FindObjectByInstruction(goInstruction, typeof(GameObject)) as GameObject; | ||
| if (go == null) | ||
| { | ||
| return new { status = "error", message = $"Could not find target GameObject: {target}" }; | ||
| } | ||
|
|
||
| Renderer renderer = go.GetComponent<Renderer>(); | ||
| if (renderer == null) | ||
| { | ||
| return new { status = "error", message = $"GameObject {go.name} has no Renderer component" }; | ||
| } | ||
|
|
||
| if (mode == "property_block") | ||
| { | ||
| if (slot < 0 || slot >= renderer.sharedMaterials.Length) | ||
| { | ||
| return new { status = "error", message = $"Slot {slot} out of bounds (count: {renderer.sharedMaterials.Length})" }; | ||
| } | ||
|
|
||
| MaterialPropertyBlock block = new MaterialPropertyBlock(); | ||
| renderer.GetPropertyBlock(block, slot); | ||
|
|
||
| if (renderer.sharedMaterials[slot] != null) | ||
| { | ||
| Material mat = renderer.sharedMaterials[slot]; | ||
| if (mat.HasProperty("_BaseColor")) block.SetColor("_BaseColor", color); | ||
| else if (mat.HasProperty("_Color")) block.SetColor("_Color", color); | ||
| else block.SetColor("_Color", color); | ||
| } | ||
| else | ||
| { | ||
| block.SetColor("_Color", color); | ||
| } | ||
|
|
||
| renderer.SetPropertyBlock(block, slot); | ||
| EditorUtility.SetDirty(renderer); | ||
| return new { status = "success", message = $"Set renderer color (PropertyBlock) on slot {slot}" }; | ||
| } | ||
| else if (mode == "shared") | ||
| { | ||
| if (slot >= 0 && slot < renderer.sharedMaterials.Length) | ||
| { | ||
| Material mat = renderer.sharedMaterials[slot]; | ||
| if (mat == null) | ||
| { | ||
| return new { status = "error", message = $"No material in slot {slot}" }; | ||
| } | ||
| Undo.RecordObject(mat, "Set Material Color"); | ||
| if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color); | ||
| else mat.SetColor("_Color", color); | ||
| EditorUtility.SetDirty(mat); | ||
| return new { status = "success", message = "Set shared material color" }; | ||
| } | ||
| return new { status = "error", message = "Invalid slot" }; | ||
| } | ||
| else if (mode == "instance") | ||
| { | ||
| if (slot >= 0 && slot < renderer.materials.Length) | ||
| { | ||
| Material mat = renderer.materials[slot]; | ||
| if (mat == null) | ||
| { | ||
| return new { status = "error", message = $"No material in slot {slot}" }; | ||
| } | ||
| // Note: Undo cannot fully revert material instantiation | ||
| Undo.RecordObject(mat, "Set Instance Material Color"); | ||
| if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color); | ||
| else mat.SetColor("_Color", color); | ||
| return new { status = "success", message = "Set instance material color", warning = "Material instance created; Undo cannot fully revert instantiation." }; | ||
| } | ||
| return new { status = "error", message = "Invalid slot" }; | ||
| } | ||
|
|
||
| return new { status = "error", message = $"Unknown mode: {mode}" }; | ||
| } |
There was a problem hiding this comment.
Guard SetColor with HasProperty in shared/instance modes and mark instance materials dirty.
In SetRendererColor:
sharedmode callsmat.SetColor("_BaseColor")ormat.SetColor("_Color")without guaranteeing that either property exists. For custom shaders this will emit Unity warnings.instancemode has the same pattern and also doesn’t callEditorUtility.SetDirty(mat).
You already have MaterialOps.GetMainColorPropertyName, so you can avoid warnings and centralize the color slot selection:
else if (mode == "shared")
{
if (slot >= 0 && slot < renderer.sharedMaterials.Length)
{
Material mat = renderer.sharedMaterials[slot];
if (mat == null)
{
return new { status = "error", message = $"No material in slot {slot}" };
}
- Undo.RecordObject(mat, "Set Material Color");
- if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color);
- else mat.SetColor("_Color", color);
- EditorUtility.SetDirty(mat);
- return new { status = "success", message = "Set shared material color" };
+ string propName = MaterialOps.GetMainColorPropertyName(mat);
+ if (!mat.HasProperty(propName))
+ {
+ return new { status = "error", message = $"Material {mat.name} has no supported color property" };
+ }
+ Undo.RecordObject(mat, "Set Material Color");
+ mat.SetColor(propName, color);
+ EditorUtility.SetDirty(mat);
+ return new { status = "success", message = "Set shared material color" };
}
return new { status = "error", message = "Invalid slot" };
}
else if (mode == "instance")
{
if (slot >= 0 && slot < renderer.materials.Length)
{
Material mat = renderer.materials[slot];
if (mat == null)
{
return new { status = "error", message = $"No material in slot {slot}" };
}
- // Note: Undo cannot fully revert material instantiation
- Undo.RecordObject(mat, "Set Instance Material Color");
- if (mat.HasProperty("_BaseColor")) mat.SetColor("_BaseColor", color);
- else mat.SetColor("_Color", color);
- return new { status = "success", message = "Set instance material color", warning = "Material instance created; Undo cannot fully revert instantiation." };
+ // Note: Undo cannot fully revert material instantiation
+ string propName = MaterialOps.GetMainColorPropertyName(mat);
+ if (!mat.HasProperty(propName))
+ {
+ return new { status = "error", message = $"Material {mat.name} has no supported color property" };
+ }
+ Undo.RecordObject(mat, "Set Instance Material Color");
+ mat.SetColor(propName, color);
+ EditorUtility.SetDirty(mat);
+ return new { status = "success", message = "Set instance material color", warning = "Material instance created; Undo cannot fully revert instantiation." };
}
return new { status = "error", message = "Invalid slot" };
}This avoids noisy console warnings and keeps color selection consistent with the rest of the material tooling.
2 participants
Summary
Introduces the
manage_materialtool, enabling comprehensive material management via MCP. This tool allows LLMs to create materials, modify shader properties (colors, textures, floats), and assign materials to GameObjects using various modes (shared, instance, property block). Previously,manage_assethandled material manipulation, but was error ridden. This will allow us to separate concerns and de-bloat manage_asset a bit, plus focus more on improved material handling features, which are complex and should live in their own tool.Key Features
_BaseMapor_MainTex) and handles case-insensitive property lookups.shared: Modifies the shared material asset directly.instance: Creates and assigns a unique material instance.property_block: UsesMaterialPropertyBlockfor performant, non-destructive per-instance overrides.Technical Implementation
ManageMaterial.csfor tool logic andMaterialOps.cshelper for robust JSON parsing and property application.manage_materialservice with full async support.HasPropertyverification before assignment, and path sanitization for cross-platform compatibility.Testing
ManageMaterialTests,ManageMaterialPropertiesTests) covering creation, property setting, and renderer assignment.Summary by CodeRabbit
New Features
Refactor
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.