Skip to content

Commit 7156009

Browse files
dsarnoclaude
andcommitted
fix: Implement collect-and-continue behavior for component property operations
- Change from fail-fast to collect-and-continue at component iteration level - Previously: first component error would halt processing of remaining components - Now: all components are processed, errors collected and returned together - Maintain existing collect-and-continue behavior within individual components - Add comprehensive tests validating the collect-and-continue behavior works correctly - All valid properties are applied even when invalid ones fail - Processing continues through exceptions with proper error aggregation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 43abb00 commit 7156009

File tree

2 files changed

+143
-35
lines changed

2 files changed

+143
-35
lines changed

TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using NUnit.Framework;
44
using UnityEngine;
55
using UnityEditor;
6+
using UnityEngine.TestTools;
67
using Newtonsoft.Json.Linq;
78
using MCPForUnity.Editor.Tools;
89

@@ -194,5 +195,118 @@ public void PerformanceTest_CachingWorks()
194195
// Second call should be faster (though this test might be flaky)
195196
Assert.LessOrEqual(secondCallTime, firstCallTime * 2, "Cached call should not be significantly slower");
196197
}
198+
199+
[Test]
200+
public void SetComponentProperties_CollectsAllFailuresAndAppliesValidOnes()
201+
{
202+
// Arrange - add Transform and Rigidbody components to test with
203+
var transform = testGameObject.transform;
204+
var rigidbody = testGameObject.AddComponent<Rigidbody>();
205+
206+
// Create a params object with mixed valid and invalid properties
207+
var setPropertiesParams = new JObject
208+
{
209+
["action"] = "modify",
210+
["target"] = testGameObject.name,
211+
["search_method"] = "by_name",
212+
["componentProperties"] = new JObject
213+
{
214+
["Transform"] = new JObject
215+
{
216+
["localPosition"] = new JObject { ["x"] = 1.0f, ["y"] = 2.0f, ["z"] = 3.0f }, // Valid
217+
["rotatoin"] = new JObject { ["x"] = 0.0f, ["y"] = 90.0f, ["z"] = 0.0f }, // Invalid (typo - should be rotation)
218+
["localScale"] = new JObject { ["x"] = 2.0f, ["y"] = 2.0f, ["z"] = 2.0f } // Valid
219+
},
220+
["Rigidbody"] = new JObject
221+
{
222+
["mass"] = 5.0f, // Valid
223+
["invalidProp"] = "test", // Invalid - doesn't exist
224+
["useGravity"] = true // Valid
225+
}
226+
}
227+
};
228+
229+
// Store original values to verify changes
230+
var originalLocalPosition = transform.localPosition;
231+
var originalLocalScale = transform.localScale;
232+
var originalMass = rigidbody.mass;
233+
var originalUseGravity = rigidbody.useGravity;
234+
235+
Debug.Log($"BEFORE TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}");
236+
237+
// Expect the warning logs from the invalid properties
238+
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'rotatoin' not found"));
239+
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'invalidProp' not found"));
240+
241+
// Act
242+
var result = ManageGameObject.HandleCommand(setPropertiesParams);
243+
244+
Debug.Log($"AFTER TEST - Mass: {rigidbody.mass}, UseGravity: {rigidbody.useGravity}");
245+
Debug.Log($"AFTER TEST - LocalPosition: {transform.localPosition}");
246+
Debug.Log($"AFTER TEST - LocalScale: {transform.localScale}");
247+
248+
// Assert - verify that valid properties were set despite invalid ones
249+
Assert.AreEqual(new Vector3(1.0f, 2.0f, 3.0f), transform.localPosition,
250+
"Valid localPosition should be set even with other invalid properties");
251+
Assert.AreEqual(new Vector3(2.0f, 2.0f, 2.0f), transform.localScale,
252+
"Valid localScale should be set even with other invalid properties");
253+
Assert.AreEqual(5.0f, rigidbody.mass, 0.001f,
254+
"Valid mass should be set even with other invalid properties");
255+
Assert.AreEqual(true, rigidbody.useGravity,
256+
"Valid useGravity should be set even with other invalid properties");
257+
258+
// Verify the result indicates errors (since we had invalid properties)
259+
Assert.IsNotNull(result, "Should return a result object");
260+
261+
// The collect-and-continue behavior means we should get an error response
262+
// that contains info about the failed properties, but valid ones were still applied
263+
// This proves the collect-and-continue behavior is working
264+
}
265+
266+
[Test]
267+
public void SetComponentProperties_ContinuesAfterException()
268+
{
269+
// Arrange - create scenario that might cause exceptions
270+
var rigidbody = testGameObject.AddComponent<Rigidbody>();
271+
272+
// Set initial values that we'll change
273+
rigidbody.mass = 1.0f;
274+
rigidbody.useGravity = true;
275+
276+
var setPropertiesParams = new JObject
277+
{
278+
["action"] = "modify",
279+
["target"] = testGameObject.name,
280+
["search_method"] = "by_name",
281+
["componentProperties"] = new JObject
282+
{
283+
["Rigidbody"] = new JObject
284+
{
285+
["mass"] = 2.5f, // Valid - should be set
286+
["velocity"] = "invalid_type", // Invalid type - will cause exception
287+
["useGravity"] = false // Valid - should still be set after exception
288+
}
289+
}
290+
};
291+
292+
// Expect the error logs from the invalid property
293+
LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex("Unexpected error converting token to UnityEngine.Vector3"));
294+
LogAssert.Expect(LogType.Error, new System.Text.RegularExpressions.Regex("SetProperty.*Failed to set 'velocity'"));
295+
LogAssert.Expect(LogType.Warning, new System.Text.RegularExpressions.Regex("Property 'velocity' not found"));
296+
297+
// Act
298+
var result = ManageGameObject.HandleCommand(setPropertiesParams);
299+
300+
// Assert - verify that valid properties before AND after the exception were still set
301+
Assert.AreEqual(2.5f, rigidbody.mass, 0.001f,
302+
"Mass should be set even if later property causes exception");
303+
Assert.AreEqual(false, rigidbody.useGravity,
304+
"UseGravity should be set even if previous property caused exception");
305+
306+
Assert.IsNotNull(result, "Should return a result even with exceptions");
307+
308+
// The key test: processing continued after the exception and set useGravity
309+
// This proves the collect-and-continue behavior works even with exceptions
310+
}
197311
}
198312
}

UnityMcpBridge/Editor/Tools/ManageGameObject.cs

Lines changed: 29 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,7 @@ string searchMethod
785785
}
786786

787787
// Set Component Properties
788+
var componentErrors = new List<object>();
788789
if (@params["componentProperties"] is JObject componentPropertiesObj)
789790
{
790791
foreach (var prop in componentPropertiesObj.Properties())
@@ -799,12 +800,26 @@ string searchMethod
799800
propertiesToSet
800801
);
801802
if (setResult != null)
802-
return setResult;
803-
modified = true;
803+
{
804+
componentErrors.Add(setResult);
805+
}
806+
else
807+
{
808+
modified = true;
809+
}
804810
}
805811
}
806812
}
807813

814+
// Return component errors if any occurred (after processing all components)
815+
if (componentErrors.Count > 0)
816+
{
817+
return Response.Error(
818+
$"One or more component property operations failed on '{targetGo.name}'.",
819+
new { componentErrors = componentErrors }
820+
);
821+
}
822+
808823
if (!modified)
809824
{
810825
// Use the new serializer helper
@@ -1534,59 +1549,38 @@ private static object SetComponentPropertiesInternal(
15341549

15351550
Undo.RecordObject(targetComponent, "Set Component Properties");
15361551

1552+
var failures = new List<string>();
15371553
foreach (var prop in propertiesToSet.Properties())
15381554
{
15391555
string propName = prop.Name;
15401556
JToken propValue = prop.Value;
15411557

15421558
try
15431559
{
1544-
if (!SetProperty(targetComponent, propName, propValue))
1560+
bool setResult = SetProperty(targetComponent, propName, propValue);
1561+
if (!setResult)
15451562
{
1546-
// Get available properties and AI suggestions for better error messages
15471563
var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType());
15481564
var suggestions = ComponentResolver.GetAIPropertySuggestions(propName, availableProperties);
1549-
1550-
var errorMessage = $"[ManageGameObject] Could not set property '{propName}' on component '{compName}' ('{targetComponent.GetType().Name}').";
1551-
1552-
if (suggestions.Any())
1553-
{
1554-
errorMessage += $" Did you mean: {string.Join(", ", suggestions)}?";
1555-
}
1556-
1557-
errorMessage += $" Available properties: [{string.Join(", ", availableProperties)}]";
1558-
1559-
Debug.LogWarning(errorMessage);
1560-
1561-
// Return enhanced error with suggestions for better UX
1562-
if (suggestions.Any())
1563-
{
1564-
return Response.Error(
1565-
$"Property '{propName}' not found on {compName}. " +
1566-
$"Did you mean: {string.Join(", ", suggestions)}? " +
1567-
$"Available properties: [{string.Join(", ", availableProperties)}]"
1568-
);
1569-
}
1570-
else
1571-
{
1572-
return Response.Error(
1573-
$"Property '{propName}' not found on {compName}. " +
1574-
$"Available properties: [{string.Join(", ", availableProperties)}]"
1575-
);
1576-
}
1565+
var msg = suggestions.Any()
1566+
? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]"
1567+
: $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]";
1568+
Debug.LogWarning($"[ManageGameObject] {msg}");
1569+
failures.Add(msg);
15771570
}
15781571
}
15791572
catch (Exception e)
15801573
{
15811574
Debug.LogError(
15821575
$"[ManageGameObject] Error setting property '{propName}' on '{compName}': {e.Message}"
15831576
);
1584-
// Optionally return an error here
1585-
// return Response.Error($"Error setting property '{propName}' on '{compName}': {e.Message}");
1577+
failures.Add($"Error setting '{propName}': {e.Message}");
15861578
}
15871579
}
15881580
EditorUtility.SetDirty(targetComponent);
1589-
return null; // Success (or partial success if warnings were logged)
1581+
return failures.Count == 0
1582+
? null
1583+
: Response.Error($"One or more properties failed on '{compName}'.", new { errors = failures });
15901584
}
15911585

15921586
/// <summary>

0 commit comments

Comments
 (0)