Skip to content

Commit 075d68d

Browse files
authored
Material tools: support direct shader property keys + add EditMode coverage (#344)
* Add TDD tests for MCP material management issues - MCPMaterialTests.cs: Tests for material creation, assignment, and data reading - MCPParameterHandlingTests.cs: Tests for JSON parameter parsing issues - SphereMaterialWorkflowTests.cs: Tests for complete sphere material workflow These tests document the current issues with: - JSON parameter parsing in manage_asset and manage_gameobject tools - Material creation with properties - Material assignment to GameObjects - Material component data reading All tests currently fail (Red phase of TDD) and serve as specifications for what needs to be fixed in the MCP system. * Refine TDD tests to focus on actual MCP tool parameter parsing issues - Removed redundant tests that verify working functionality (GameObjectSerializer, Unity APIs) - Kept focused tests that document the real issue: MCP tool parameter validation - Tests now clearly identify the root cause: JSON string parsing in MCP tools - Tests specify exactly what needs to be fixed: parameter type flexibility The issue is NOT in Unity APIs or serialization (which work fine), but in MCP tool parameter validation being too strict. * Fix port discovery protocol mismatch - Update _try_probe_unity_mcp to recognize Unity bridge welcome message - Unity bridge sends 'WELCOME UNITY-MCP' instead of JSON pong response - Maintains backward compatibility with JSON pong format - Fixes MCP server connection to Unity Editor * Resolve merge: unify manage_gameobject param coercion and schema widening * Tests: add MaterialParameterToolTests; merge port probe fix; widen tool schemas for JSON-string params * feat(material): support direct shader property keys and texture paths; add EditMode tests * chore(tests): track required .meta files; remove ephemeral Assets/Editor test helper * fix(manage_gameobject): validate parsed component_properties is a dict; return clear error
1 parent a0287af commit 075d68d

File tree

14 files changed

+357
-59
lines changed

14 files changed

+357
-59
lines changed

MCPForUnity/Editor/Tools/ManageAsset.cs

Lines changed: 115 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,19 @@ public static object HandleCommand(JObject @params)
6464
string path = @params["path"]?.ToString();
6565

6666
// Coerce string JSON to JObject for 'properties' if provided as a JSON string
67-
JsonUtil.CoerceJsonStringParameter(@params, "properties");
67+
var propertiesToken = @params["properties"];
68+
if (propertiesToken != null && propertiesToken.Type == JTokenType.String)
69+
{
70+
try
71+
{
72+
var parsed = JObject.Parse(propertiesToken.ToString());
73+
@params["properties"] = parsed;
74+
}
75+
catch (Exception e)
76+
{
77+
Debug.LogWarning($"[ManageAsset] Could not parse 'properties' JSON string: {e.Message}");
78+
}
79+
}
6880

6981
try
7082
{
@@ -1002,7 +1014,108 @@ private static bool ApplyMaterialProperties(Material mat, JObject properties)
10021014
}
10031015
}
10041016

1005-
// TODO: Add handlers for other property types (Vectors, Ints, Keywords, RenderQueue, etc.)
1017+
// --- Flexible direct property assignment ---
1018+
// Allow payloads like: { "_Color": [r,g,b,a] }, { "_Glossiness": 0.5 }, { "_MainTex": "Assets/.." }
1019+
// while retaining backward compatibility with the structured keys above.
1020+
// This iterates all top-level keys except the reserved structured ones and applies them
1021+
// if they match known shader properties.
1022+
var reservedKeys = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "shader", "color", "float", "texture" };
1023+
1024+
// Helper resolves common URP/Standard aliasing (e.g., _Color <-> _BaseColor, _MainTex <-> _BaseMap, _Glossiness <-> _Smoothness)
1025+
string ResolvePropertyName(string name)
1026+
{
1027+
if (string.IsNullOrEmpty(name)) return name;
1028+
string[] candidates;
1029+
switch (name)
1030+
{
1031+
case "_Color": candidates = new[] { "_Color", "_BaseColor" }; break;
1032+
case "_BaseColor": candidates = new[] { "_BaseColor", "_Color" }; break;
1033+
case "_MainTex": candidates = new[] { "_MainTex", "_BaseMap" }; break;
1034+
case "_BaseMap": candidates = new[] { "_BaseMap", "_MainTex" }; break;
1035+
case "_Glossiness": candidates = new[] { "_Glossiness", "_Smoothness" }; break;
1036+
case "_Smoothness": candidates = new[] { "_Smoothness", "_Glossiness" }; break;
1037+
default: candidates = new[] { name }; break;
1038+
}
1039+
foreach (var candidate in candidates)
1040+
{
1041+
if (mat.HasProperty(candidate)) return candidate;
1042+
}
1043+
return name; // fall back to original
1044+
}
1045+
1046+
foreach (var prop in properties.Properties())
1047+
{
1048+
if (reservedKeys.Contains(prop.Name)) continue;
1049+
string shaderProp = ResolvePropertyName(prop.Name);
1050+
JToken v = prop.Value;
1051+
1052+
// Color: numeric array [r,g,b,(a)]
1053+
if (v is JArray arr && arr.Count >= 3 && arr.All(t => t.Type == JTokenType.Float || t.Type == JTokenType.Integer))
1054+
{
1055+
if (mat.HasProperty(shaderProp))
1056+
{
1057+
try
1058+
{
1059+
var c = new Color(
1060+
arr[0].ToObject<float>(),
1061+
arr[1].ToObject<float>(),
1062+
arr[2].ToObject<float>(),
1063+
arr.Count > 3 ? arr[3].ToObject<float>() : 1f
1064+
);
1065+
if (mat.GetColor(shaderProp) != c)
1066+
{
1067+
mat.SetColor(shaderProp, c);
1068+
modified = true;
1069+
}
1070+
}
1071+
catch (Exception ex)
1072+
{
1073+
Debug.LogWarning($"Error setting color '{shaderProp}': {ex.Message}");
1074+
}
1075+
}
1076+
continue;
1077+
}
1078+
1079+
// Float: single number
1080+
if (v.Type == JTokenType.Float || v.Type == JTokenType.Integer)
1081+
{
1082+
if (mat.HasProperty(shaderProp))
1083+
{
1084+
try
1085+
{
1086+
float f = v.ToObject<float>();
1087+
if (!Mathf.Approximately(mat.GetFloat(shaderProp), f))
1088+
{
1089+
mat.SetFloat(shaderProp, f);
1090+
modified = true;
1091+
}
1092+
}
1093+
catch (Exception ex)
1094+
{
1095+
Debug.LogWarning($"Error setting float '{shaderProp}': {ex.Message}");
1096+
}
1097+
}
1098+
continue;
1099+
}
1100+
1101+
// Texture: string path
1102+
if (v.Type == JTokenType.String)
1103+
{
1104+
string texPath = v.ToString();
1105+
if (!string.IsNullOrEmpty(texPath) && mat.HasProperty(shaderProp))
1106+
{
1107+
var tex = AssetDatabase.LoadAssetAtPath<Texture>(AssetPathUtility.SanitizeAssetPath(texPath));
1108+
if (tex != null && mat.GetTexture(shaderProp) != tex)
1109+
{
1110+
mat.SetTexture(shaderProp, tex);
1111+
modified = true;
1112+
}
1113+
}
1114+
continue;
1115+
}
1116+
}
1117+
1118+
// TODO: Add handlers for other property types (Vectors, Ints, Keywords, RenderQueue, etc.)
10061119
return modified;
10071120
}
10081121

MCPForUnity/Editor/Tools/ManageGameObject.cs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,19 @@ public static object HandleCommand(JObject @params)
6767
// --- End add parameter ---
6868

6969
// Coerce string JSON to JObject for 'componentProperties' if provided as a JSON string
70-
JsonUtil.CoerceJsonStringParameter(@params, "componentProperties");
70+
var componentPropsToken = @params["componentProperties"];
71+
if (componentPropsToken != null && componentPropsToken.Type == JTokenType.String)
72+
{
73+
try
74+
{
75+
var parsed = JObject.Parse(componentPropsToken.ToString());
76+
@params["componentProperties"] = parsed;
77+
}
78+
catch (Exception e)
79+
{
80+
Debug.LogWarning($"[ManageGameObject] Could not parse 'componentProperties' JSON string: {e.Message}");
81+
}
82+
}
7183

7284
// --- Prefab Redirection Check ---
7385
string targetPath =

MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ async def manage_asset(
3939
try:
4040
properties = json.loads(properties)
4141
ctx.info("manage_asset: coerced properties from JSON string to dict")
42-
except json.JSONDecodeError as e:
42+
except Exception as e:
4343
ctx.warn(f"manage_asset: failed to parse properties JSON string: {e}")
4444
# Leave properties as-is; Unity side may handle defaults
4545
# Ensure properties is a dict if None

MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ def manage_gameobject(
4545
"List of component names to remove"] | None = None,
4646
component_properties: Annotated[dict[str, dict[str, Any]] | str,
4747
"""Dictionary of component names to their properties to set. For example:
48-
Can also be provided as a JSON string representation of the dict.
4948
`{"MyScript": {"otherObject": {"find": "Player", "method": "by_name"}}}` assigns GameObject
5049
`{"MyScript": {"playerHealth": {"find": "Player", "component": "HealthComponent"}}}` assigns Component
5150
Example set nested property:
@@ -122,6 +121,9 @@ def _to_vec3(parts):
122121
ctx.info("manage_gameobject: coerced component_properties from JSON string to dict")
123122
except json.JSONDecodeError as e:
124123
return {"success": False, "message": f"Invalid JSON in component_properties: {e}"}
124+
# Ensure final type is a dict (object) if provided
125+
if component_properties is not None and not isinstance(component_properties, dict):
126+
return {"success": False, "message": "component_properties must be a JSON object (dict)."}
125127
try:
126128
# Map tag to search_term when search_method is by_tag for backward compatibility
127129
if action == "find" and search_method == "by_tag" and tag is not None and search_term is None:

TestProjects/UnityMCPTests/Assets/Temp/LiveTests.meta

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Temp/MCPToolParameterTests.meta

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Temp/MaterialDirectPropertiesTests.meta

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Temp/MaterialParameterToolTests.meta

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,24 +18,21 @@ namespace Tests.EditMode
1818
/// </summary>
1919
public class MCPToolParameterTests
2020
{
21-
private const string TempDir = "Assets/Temp/MCPToolParameterTests";
22-
23-
[SetUp]
24-
public void SetUp()
21+
[Test]
22+
public void Test_ManageAsset_ShouldAcceptJSONProperties()
2523
{
24+
// Arrange: create temp folder
25+
const string tempDir = "Assets/Temp/MCPToolParameterTests";
2626
if (!AssetDatabase.IsValidFolder("Assets/Temp"))
2727
{
2828
AssetDatabase.CreateFolder("Assets", "Temp");
2929
}
30-
if (!AssetDatabase.IsValidFolder(TempDir))
30+
if (!AssetDatabase.IsValidFolder(tempDir))
3131
{
3232
AssetDatabase.CreateFolder("Assets/Temp", "MCPToolParameterTests");
3333
}
34-
}
35-
[Test]
36-
public void Test_ManageAsset_ShouldAcceptJSONProperties()
37-
{
38-
var matPath = $"{TempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
34+
35+
var matPath = $"{tempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
3936

4037
// Build params with properties as a JSON string (to be coerced)
4138
var p = new JObject
@@ -73,7 +70,10 @@ public void Test_ManageAsset_ShouldAcceptJSONProperties()
7370
[Test]
7471
public void Test_ManageGameObject_ShouldAcceptJSONComponentProperties()
7572
{
76-
var matPath = $"{TempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
73+
const string tempDir = "Assets/Temp/MCPToolParameterTests";
74+
if (!AssetDatabase.IsValidFolder("Assets/Temp")) AssetDatabase.CreateFolder("Assets", "Temp");
75+
if (!AssetDatabase.IsValidFolder(tempDir)) AssetDatabase.CreateFolder("Assets/Temp", "MCPToolParameterTests");
76+
var matPath = $"{tempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
7777

7878
// Create a material first (object-typed properties)
7979
var createMat = new JObject
@@ -121,7 +121,10 @@ public void Test_ManageGameObject_ShouldAcceptJSONComponentProperties()
121121
[Test]
122122
public void Test_JSONParsing_ShouldWorkInMCPTools()
123123
{
124-
var matPath = $"{TempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
124+
const string tempDir = "Assets/Temp/MCPToolParameterTests";
125+
if (!AssetDatabase.IsValidFolder("Assets/Temp")) AssetDatabase.CreateFolder("Assets", "Temp");
126+
if (!AssetDatabase.IsValidFolder(tempDir)) AssetDatabase.CreateFolder("Assets/Temp", "MCPToolParameterTests");
127+
var matPath = $"{tempDir}/JsonMat_{Guid.NewGuid().ToString("N")}.mat";
125128

126129
// manage_asset with JSON string properties (coercion path)
127130
var createMat = new JObject

0 commit comments

Comments
 (0)