Skip to content

Commit 552b2d3

Browse files
authored
Harden manage_scriptable_object Tool (#522)
* feat(manage_scriptable_object): harden tool with path normalization, auto-resize, bulk mapping Phase 1: Path Syntax & Auto-Resizing - Add NormalizePropertyPath() to convert field[index] to Array.data format - Add EnsureArrayCapacity() to auto-grow arrays when targeting out-of-bounds indices Phase 2: Consolidation - Replace duplicate TryGet* helpers with ParamCoercion/VectorParsing shared utilities - Add Vector4 parsing support to VectorParsing.cs Phase 3: Bulk Data Mapping - Handle JArray values for list/array properties (recursive element setting) - Handle JObject values for nested struct/class properties Phase 4: Enhanced Reference Resolution - Support plain 32-char GUID strings for ObjectReference fields Phase 5: Validation & Dry-Run - Add ValidatePatches() for pre-validation of all patches - Add dry_run parameter to validate without mutating Includes comprehensive stress test suite covering: - Big Bang (large nested arrays), Out of Bounds, Friendly Path Syntax - Deep Nesting, Mixed References, Rapid Fire, Type Mismatch - Bulk Array Mapping, GUID Shorthand, Dry Run validation * feat: Add AnimationCurve and Quaternion support to manage_scriptable_object tool - Implement TrySetAnimationCurve() supporting both {'keys': [...]} and direct [...] formats * Support keyframe properties: time, value, inSlope, outSlope, weightedMode, inWeight, outWeight * Gracefully default missing optional fields to 0 * Clear error messages for malformed structures - Implement TrySetQuaternion() with 4 input formats: * Euler array [x, y, z] - 3 elements interpreted as degrees * Raw array [x, y, z, w] - 4 components * Object format {x, y, z, w} - explicit components * Explicit euler {euler: [x, y, z]} - labeled format - Improve error handling: * Null values: AnimationCurve→empty, Quaternion→identity * Invalid inputs rejected with specific, actionable error messages * Validate keyframe objects and array sizes - Add comprehensive test coverage in ManageScriptableObjectStressTests.cs: * AnimationCurve with keyframe array format * AnimationCurve with direct array (no wrapper) * Quaternion via Euler angles * Quaternion via raw components * Quaternion via object format * Quaternion via explicit euler property - Fix test file compilation issues: * Replace undefined TestFolder with _runRoot * Add System.IO using statement * refactor: consolidate test utilities to eliminate duplication - Add TestUtilities.cs with shared helpers: - ToJObject() - consolidates 11 duplicates across test files - EnsureFolder() - consolidates 2 duplicates - WaitForUnityReady() - consolidates 2 duplicates - FindFallbackShader() - consolidates shader chain duplicates - SafeDeleteAsset() - helper for asset cleanup - CleanupEmptyParentFolders() - standardizes TearDown cleanup - Update 11 test files to use shared TestUtilities via 'using static' - Standardize TearDown cleanup patterns across all test files - Net reduction of ~40 lines while improving maintainability * fix: add missing animCurve and rotation fields to ComplexStressSO Add AnimationCurve and Quaternion fields required by Phase 6 stress tests.
1 parent 3090a01 commit 552b2d3

25 files changed

Lines changed: 2250 additions & 350 deletions

MCPForUnity/Editor/Helpers/VectorParsing.cs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,49 @@ public static Vector3 ParseVector3OrDefault(JToken token, Vector3 defaultValue =
9797
return null;
9898
}
9999

100+
/// <summary>
101+
/// Parses a JToken (array or object) into a Vector4.
102+
/// </summary>
103+
/// <param name="token">The JSON token to parse</param>
104+
/// <returns>The parsed Vector4 or null if parsing fails</returns>
105+
public static Vector4? ParseVector4(JToken token)
106+
{
107+
if (token == null || token.Type == JTokenType.Null)
108+
return null;
109+
110+
try
111+
{
112+
// Array format: [x, y, z, w]
113+
if (token is JArray array && array.Count >= 4)
114+
{
115+
return new Vector4(
116+
array[0].ToObject<float>(),
117+
array[1].ToObject<float>(),
118+
array[2].ToObject<float>(),
119+
array[3].ToObject<float>()
120+
);
121+
}
122+
123+
// Object format: {x: 1, y: 2, z: 3, w: 4}
124+
if (token is JObject obj && obj.ContainsKey("x") && obj.ContainsKey("y") &&
125+
obj.ContainsKey("z") && obj.ContainsKey("w"))
126+
{
127+
return new Vector4(
128+
obj["x"].ToObject<float>(),
129+
obj["y"].ToObject<float>(),
130+
obj["z"].ToObject<float>(),
131+
obj["w"].ToObject<float>()
132+
);
133+
}
134+
}
135+
catch (Exception ex)
136+
{
137+
Debug.LogWarning($"[VectorParsing] Failed to parse Vector4 from '{token}': {ex.Message}");
138+
}
139+
140+
return null;
141+
}
142+
100143
/// <summary>
101144
/// Parses a JToken (array or object) into a Quaternion.
102145
/// Supports both euler angles [x, y, z] and quaternion components [x, y, z, w].

MCPForUnity/Editor/Tools/ManageScriptableObject.cs

Lines changed: 727 additions & 176 deletions
Large diffs are not rendered by default.

Server/src/services/tools/manage_scriptable_object.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ async def manage_scriptable_object(
4141
target: Annotated[dict[str, Any] | str | None, "Target asset reference {guid|path} (for modify)."] = None,
4242
# --- shared ---
4343
patches: Annotated[list[dict[str, Any]] | str | None, "Patch list (or JSON string) to apply."] = None,
44+
# --- validation ---
45+
dry_run: Annotated[bool | str | None, "If true, validate patches without applying (modify only)."] = None,
4446
) -> dict[str, Any]:
4547
unity_instance = get_unity_instance_from_context(ctx)
4648

@@ -62,6 +64,7 @@ async def manage_scriptable_object(
6264
"overwrite": coerce_bool(overwrite, default=None),
6365
"target": parsed_target,
6466
"patches": parsed_patches,
67+
"dryRun": coerce_bool(dry_run, default=None),
6568
}
6669

6770
# Remove None values to keep Unity handler simpler

Server/tests/integration/test_manage_scriptable_object_tool.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,62 @@ async def fake_async_send(cmd, params, **kwargs):
6969
assert captured["params"]["patches"][0]["op"] == "array_resize"
7070

7171

72+
def test_manage_scriptable_object_forwards_dry_run_param(monkeypatch):
73+
captured = {}
74+
75+
async def fake_async_send(cmd, params, **kwargs):
76+
captured["cmd"] = cmd
77+
captured["params"] = params
78+
return {"success": True, "data": {"dryRun": True, "validationResults": []}}
79+
80+
monkeypatch.setattr(mod, "async_send_command_with_retry", fake_async_send)
81+
82+
ctx = DummyContext()
83+
ctx.set_state("unity_instance", "UnityMCPTests@dummy")
84+
85+
result = asyncio.run(
86+
mod.manage_scriptable_object(
87+
ctx=ctx,
88+
action="modify",
89+
target='{"guid":"abc123"}',
90+
patches=[{"propertyPath": "intValue", "op": "set", "value": 42}],
91+
dry_run=True,
92+
)
93+
)
94+
95+
assert result["success"] is True
96+
assert captured["cmd"] == "manage_scriptable_object"
97+
assert captured["params"]["action"] == "modify"
98+
assert captured["params"]["dryRun"] is True
99+
assert captured["params"]["target"] == {"guid": "abc123"}
100+
101+
102+
def test_manage_scriptable_object_dry_run_string_coercion(monkeypatch):
103+
"""Test that dry_run accepts string 'true' and coerces to boolean."""
104+
captured = {}
105+
106+
async def fake_async_send(cmd, params, **kwargs):
107+
captured["cmd"] = cmd
108+
captured["params"] = params
109+
return {"success": True, "data": {"dryRun": True}}
110+
111+
monkeypatch.setattr(mod, "async_send_command_with_retry", fake_async_send)
112+
113+
ctx = DummyContext()
114+
ctx.set_state("unity_instance", "UnityMCPTests@dummy")
115+
116+
result = asyncio.run(
117+
mod.manage_scriptable_object(
118+
ctx=ctx,
119+
action="modify",
120+
target={"guid": "xyz"},
121+
patches=[],
122+
dry_run="true", # String instead of bool
123+
)
124+
)
125+
126+
assert result["success"] is True
127+
assert captured["params"]["dryRun"] is True
128+
129+
72130

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
using System;
2+
using System.Collections;
3+
using System.IO;
4+
using Newtonsoft.Json.Linq;
5+
using NUnit.Framework;
6+
using UnityEditor;
7+
using UnityEngine;
8+
9+
namespace MCPForUnityTests.Editor
10+
{
11+
/// <summary>
12+
/// Shared test utilities for EditMode tests across the MCP for Unity test suite.
13+
/// Consolidates common patterns to avoid duplication across test files.
14+
/// </summary>
15+
public static class TestUtilities
16+
{
17+
/// <summary>
18+
/// Safely converts a command result to JObject, handling both JSON objects and other types.
19+
/// Returns an empty JObject if result is null.
20+
/// </summary>
21+
public static JObject ToJObject(object result)
22+
{
23+
if (result == null) return new JObject();
24+
return result as JObject ?? JObject.FromObject(result);
25+
}
26+
27+
/// <summary>
28+
/// Creates all parent directories for the given asset path if they don't exist.
29+
/// Handles normalization and validates against dangerous patterns.
30+
/// </summary>
31+
/// <param name="folderPath">An Assets-relative folder path (e.g., "Assets/Temp/MyFolder")</param>
32+
public static void EnsureFolder(string folderPath)
33+
{
34+
if (AssetDatabase.IsValidFolder(folderPath))
35+
return;
36+
37+
var sanitized = MCPForUnity.Editor.Helpers.AssetPathUtility.SanitizeAssetPath(folderPath);
38+
if (string.Equals(sanitized, "Assets", StringComparison.OrdinalIgnoreCase))
39+
return;
40+
41+
var parts = sanitized.Split('/');
42+
string current = "Assets";
43+
for (int i = 1; i < parts.Length; i++)
44+
{
45+
var next = current + "/" + parts[i];
46+
if (!AssetDatabase.IsValidFolder(next))
47+
{
48+
AssetDatabase.CreateFolder(current, parts[i]);
49+
}
50+
current = next;
51+
}
52+
}
53+
54+
/// <summary>
55+
/// Waits for Unity to finish compiling and updating, with a configurable timeout.
56+
/// Some EditMode tests trigger script compilation/domain reload.
57+
/// Tools intentionally return "compiling_or_reloading" during these windows.
58+
/// </summary>
59+
/// <param name="timeoutSeconds">Maximum time to wait before failing the test.</param>
60+
public static IEnumerator WaitForUnityReady(double timeoutSeconds = 30.0)
61+
{
62+
double start = EditorApplication.timeSinceStartup;
63+
while (EditorApplication.isCompiling || EditorApplication.isUpdating)
64+
{
65+
if (EditorApplication.timeSinceStartup - start > timeoutSeconds)
66+
{
67+
Assert.Fail($"Timed out waiting for Unity to finish compiling/updating (>{timeoutSeconds:0.0}s).");
68+
}
69+
yield return null;
70+
}
71+
}
72+
73+
/// <summary>
74+
/// Finds a fallback shader for creating materials in tests.
75+
/// Tries modern pipelines first, then falls back to Standard/Unlit.
76+
/// </summary>
77+
/// <returns>A shader suitable for test materials, or null if none found.</returns>
78+
public static Shader FindFallbackShader()
79+
{
80+
return Shader.Find("Universal Render Pipeline/Lit")
81+
?? Shader.Find("HDRP/Lit")
82+
?? Shader.Find("Standard")
83+
?? Shader.Find("Unlit/Color");
84+
}
85+
86+
/// <summary>
87+
/// Safely deletes an asset if it exists.
88+
/// </summary>
89+
/// <param name="path">The asset path to delete.</param>
90+
public static void SafeDeleteAsset(string path)
91+
{
92+
if (!string.IsNullOrEmpty(path) && AssetDatabase.LoadAssetAtPath<UnityEngine.Object>(path) != null)
93+
{
94+
AssetDatabase.DeleteAsset(path);
95+
}
96+
}
97+
98+
/// <summary>
99+
/// Cleans up empty parent folders recursively up to but not including "Assets".
100+
/// Useful in TearDown to avoid leaving folder debris.
101+
/// </summary>
102+
/// <param name="folderPath">The starting folder path to check.</param>
103+
public static void CleanupEmptyParentFolders(string folderPath)
104+
{
105+
if (string.IsNullOrEmpty(folderPath))
106+
return;
107+
108+
var parent = Path.GetDirectoryName(folderPath)?.Replace('\\', '/');
109+
while (!string.IsNullOrEmpty(parent) && parent != "Assets")
110+
{
111+
if (AssetDatabase.IsValidFolder(parent))
112+
{
113+
try
114+
{
115+
var dirs = Directory.GetDirectories(parent);
116+
var files = Directory.GetFiles(parent);
117+
if (dirs.Length == 0 && files.Length == 0)
118+
{
119+
AssetDatabase.DeleteAsset(parent);
120+
parent = Path.GetDirectoryName(parent)?.Replace('\\', '/');
121+
}
122+
else
123+
{
124+
break;
125+
}
126+
}
127+
catch
128+
{
129+
break;
130+
}
131+
}
132+
else
133+
{
134+
break;
135+
}
136+
}
137+
}
138+
}
139+
}
140+

TestProjects/UnityMCPTests/Assets/Tests/EditMode/TestUtilities.cs.meta

Lines changed: 11 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/Tools/Fixtures/StressTestSOs.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.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
using UnityEngine;
2+
3+
[CreateAssetMenu(fileName = "ArrayStressSO", menuName = "StressTests/ArrayStressSO")]
4+
public class ArrayStressSO : ScriptableObject
5+
{
6+
public float[] floatArray = new float[3];
7+
}

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/Fixtures/StressTestSOs/ArrayStressSO.cs.meta

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
using UnityEngine;
2+
using System.Collections.Generic;
3+
4+
[System.Serializable]
5+
public struct NestedData
6+
{
7+
public string id;
8+
public float value;
9+
public Vector3 position;
10+
}
11+
12+
[System.Serializable]
13+
public class ComplexSubClass
14+
{
15+
public string name;
16+
public int level;
17+
public List<float> scores;
18+
}
19+
20+
public enum TestEnum
21+
{
22+
Alpha,
23+
Beta,
24+
Gamma
25+
}
26+
27+
[CreateAssetMenu(fileName = "ComplexStressSO", menuName = "StressTests/ComplexStressSO")]
28+
public class ComplexStressSO : ScriptableObject
29+
{
30+
[Header("Basic Types")]
31+
public int intValue;
32+
public float floatValue;
33+
public string stringValue;
34+
public bool boolValue;
35+
public Vector3 vectorValue;
36+
public Color colorValue;
37+
public TestEnum enumValue;
38+
39+
[Header("Arrays & Lists")]
40+
public int[] intArray;
41+
public List<string> stringList;
42+
public Vector3[] vectorArray;
43+
44+
[Header("Complex Types")]
45+
public NestedData nestedStruct;
46+
public ComplexSubClass nestedClass;
47+
public List<NestedData> nestedDataList;
48+
49+
[Header("Extended Types (Phase 6)")]
50+
public AnimationCurve animCurve;
51+
public Quaternion rotation;
52+
}

0 commit comments

Comments
 (0)