diff --git a/MCPForUnity/Editor/Helpers/ComponentOps.cs b/MCPForUnity/Editor/Helpers/ComponentOps.cs index 33a911a1d..5b19bb8a0 100644 --- a/MCPForUnity/Editor/Helpers/ComponentOps.cs +++ b/MCPForUnity/Editor/Helpers/ComponentOps.cs @@ -174,15 +174,37 @@ public static bool SetProperty(Component component, string propertyName, JToken return SetViaSerializedProperty(component, propertyName, normalizedName, value, out error); } - // Try property first - check both original and normalized names for backwards compatibility - PropertyInfo propInfo = type.GetProperty(propertyName, flags) + // Try reflection first (property, field, then non-public serialized field) + if (TrySetViaReflection(component, type, propertyName, normalizedName, flags, value, out error)) + return true; + + // Reflection failed — fall back to SerializedProperty which handles arrays, + // custom serialization (e.g. UdonSharp), and types reflection can't convert. + string reflectionError = error; + if (SetViaSerializedProperty(component, propertyName, normalizedName, value, out error)) + return true; + + // Both paths failed. If reflection found the member but couldn't convert, + // report that (more useful than the SerializedProperty error). + // If reflection didn't find it at all, report the SerializedProperty error. + if (reflectionError != null && !reflectionError.Contains("not found")) + error = reflectionError; + + return false; + } + + private static bool TrySetViaReflection(object component, Type type, string propertyName, string normalizedName, BindingFlags flags, JToken value, out string error) + { + error = null; + + // Try property first + PropertyInfo propInfo = type.GetProperty(propertyName, flags) ?? type.GetProperty(normalizedName, flags); if (propInfo != null && propInfo.CanWrite) { try { object convertedValue = PropertyConversion.ConvertToType(value, propInfo.PropertyType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for property '{propertyName}' to type '{propInfo.PropertyType.Name}'."; @@ -198,15 +220,14 @@ public static bool SetProperty(Component component, string propertyName, JToken } } - // Try field - check both original and normalized names for backwards compatibility - FieldInfo fieldInfo = type.GetField(propertyName, flags) + // Try field + FieldInfo fieldInfo = type.GetField(propertyName, flags) ?? type.GetField(normalizedName, flags); if (fieldInfo != null && !fieldInfo.IsInitOnly) { try { object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for field '{propertyName}' to type '{fieldInfo.FieldType.Name}'."; @@ -222,9 +243,7 @@ public static bool SetProperty(Component component, string propertyName, JToken } } - // Try non-public serialized fields - traverse inheritance hierarchy - // Type.GetField() with NonPublic only finds fields declared directly on that type, - // so we need to walk up the inheritance chain manually + // Try non-public serialized fields — traverse inheritance hierarchy fieldInfo = FindSerializedFieldInHierarchy(type, propertyName) ?? FindSerializedFieldInHierarchy(type, normalizedName); if (fieldInfo != null) @@ -232,7 +251,6 @@ public static bool SetProperty(Component component, string propertyName, JToken try { object convertedValue = PropertyConversion.ConvertToType(value, fieldInfo.FieldType); - // Detect conversion failure: null result when input wasn't null if (convertedValue == null && value.Type != JTokenType.Null) { error = $"Failed to convert value for serialized field '{propertyName}' to type '{fieldInfo.FieldType.Name}'."; @@ -307,7 +325,7 @@ public static List GetAccessibleMembers(Type componentType) /// Type.GetField() with NonPublic only returns fields declared directly on that type, /// so this method walks up the chain to find inherited private serialized fields. /// - private static FieldInfo FindSerializedFieldInHierarchy(Type type, string fieldName) + internal static FieldInfo FindSerializedFieldInHierarchy(Type type, string fieldName) { if (type == null || string.IsNullOrEmpty(fieldName)) return null; diff --git a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs index 8e35508c9..1e7063ae6 100644 --- a/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs +++ b/MCPForUnity/Editor/Tools/GameObjects/GameObjectComponentHelpers.cs @@ -172,14 +172,32 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin try { - bool setResult = SetProperty(targetComponent, propName, propValue); + bool setResult; + string setError; + + // Nested paths (e.g. "transform.position") need local handling + // since ComponentOps doesn't support dot/bracket notation. + if (propName.Contains('.') || propName.Contains('[')) + { + setResult = SetNestedProperty(targetComponent, propName, propValue, InputSerializer, out setError); + } + else + { + // ComponentOps handles reflection + SerializedProperty fallback + setResult = ComponentOps.SetProperty(targetComponent, propName, propValue, out setError); + } + if (!setResult) { - var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); - var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties); - var msg = suggestions.Any() - ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" - : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + string msg = setError; + if (msg == null || msg.Contains("not found")) + { + var availableProperties = ComponentResolver.GetAllComponentProperties(targetComponent.GetType()); + var suggestions = ComponentResolver.GetFuzzyPropertySuggestions(propName, availableProperties); + msg = suggestions.Any() + ? $"Property '{propName}' not found. Did you mean: {string.Join(", ", suggestions)}? Available: [{string.Join(", ", availableProperties)}]" + : $"Property '{propName}' not found. Available: [{string.Join(", ", availableProperties)}]"; + } McpLog.Warn($"[ManageGameObject] {msg}"); failures.Add(msg); } @@ -199,73 +217,17 @@ internal static object SetComponentPropertiesInternal(GameObject targetGo, strin private static JsonSerializer InputSerializer => UnityJsonSerializer.Instance; - private static bool SetProperty(object target, string memberName, JToken value) - { - Type type = target.GetType(); - BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase; - - string normalizedName = Helpers.ParamCoercion.NormalizePropertyName(memberName); - var inputSerializer = InputSerializer; - - try - { - if (memberName.Contains('.') || memberName.Contains('[')) - { - return SetNestedProperty(target, memberName, value, inputSerializer); - } - - PropertyInfo propInfo = type.GetProperty(memberName, flags) ?? type.GetProperty(normalizedName, flags); - if (propInfo != null && propInfo.CanWrite) - { - object convertedValue = ConvertJTokenToType(value, propInfo.PropertyType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - propInfo.SetValue(target, convertedValue); - return true; - } - } - else - { - FieldInfo fieldInfo = type.GetField(memberName, flags) ?? type.GetField(normalizedName, flags); - if (fieldInfo != null) - { - object convertedValue = ConvertJTokenToType(value, fieldInfo.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - fieldInfo.SetValue(target, convertedValue); - return true; - } - } - else - { - var npField = type.GetField(memberName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase) - ?? type.GetField(normalizedName, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase); - if (npField != null && npField.GetCustomAttribute() != null) - { - object convertedValue = ConvertJTokenToType(value, npField.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - npField.SetValue(target, convertedValue); - return true; - } - } - } - } - } - catch (Exception ex) - { - McpLog.Error($"[SetProperty] Failed to set '{memberName}' on {type.Name}: {ex.Message}\nToken: {value.ToString(Formatting.None)}"); - } - return false; - } - - private static bool SetNestedProperty(object target, string path, JToken value, JsonSerializer inputSerializer) + private static bool SetNestedProperty(object target, string path, JToken value, JsonSerializer inputSerializer, out string error) { + error = null; try { string[] pathParts = SplitPropertyPath(path); if (pathParts.Length == 0) + { + error = $"Invalid nested property path '{path}'."; return false; + } object currentObject = target; Type currentType = currentObject.GetType(); @@ -299,7 +261,7 @@ private static bool SetNestedProperty(object target, string path, JToken value, fieldInfo = currentType.GetField(part, flags); if (fieldInfo == null) { - McpLog.Warn($"[SetNestedProperty] Could not find property or field '{part}' on type '{currentType.Name}'"); + error = $"Could not find property or field '{part}' on type '{currentType.Name}' in path '{path}'."; return false; } } @@ -307,7 +269,7 @@ private static bool SetNestedProperty(object target, string path, JToken value, currentObject = propInfo != null ? propInfo.GetValue(currentObject) : fieldInfo.GetValue(currentObject); if (currentObject == null) { - McpLog.Warn($"[SetNestedProperty] Property '{part}' is null, cannot access nested properties."); + error = $"Property '{part}' is null in path '{path}', cannot access nested properties."; return false; } @@ -316,9 +278,14 @@ private static bool SetNestedProperty(object target, string path, JToken value, if (currentObject is Material[]) { var materials = currentObject as Material[]; + if (materials.Length == 0) + { + error = $"Material array is empty in path '{path}', cannot access index {arrayIndex}."; + return false; + } if (arrayIndex < 0 || arrayIndex >= materials.Length) { - McpLog.Warn($"[SetNestedProperty] Material index {arrayIndex} out of range (0-{materials.Length - 1})"); + error = $"Material index {arrayIndex} out of range (0-{materials.Length - 1}) in path '{path}'."; return false; } currentObject = materials[arrayIndex]; @@ -326,16 +293,21 @@ private static bool SetNestedProperty(object target, string path, JToken value, else if (currentObject is System.Collections.IList) { var list = currentObject as System.Collections.IList; + if (list.Count == 0) + { + error = $"List is empty in path '{path}', cannot access index {arrayIndex}."; + return false; + } if (arrayIndex < 0 || arrayIndex >= list.Count) { - McpLog.Warn($"[SetNestedProperty] Index {arrayIndex} out of range (0-{list.Count - 1})"); + error = $"Index {arrayIndex} out of range (0-{list.Count - 1}) in path '{path}'."; return false; } currentObject = list[arrayIndex]; } else { - McpLog.Warn($"[SetNestedProperty] Property '{part}' is not an array or list, cannot access by index."); + error = $"Property '{part}' is not an array or list in path '{path}', cannot access by index."; return false; } } @@ -347,7 +319,12 @@ private static bool SetNestedProperty(object target, string path, JToken value, if (currentObject is Material material && finalPart.StartsWith("_")) { - return MaterialOps.TrySetShaderProperty(material, finalPart, value, inputSerializer); + if (!MaterialOps.TrySetShaderProperty(material, finalPart, value, inputSerializer)) + { + error = $"Failed to set shader property '{finalPart}' on material '{material.name}' in path '{path}'."; + return false; + } + return true; } PropertyInfo finalPropInfo = currentType.GetProperty(finalPart, flags); @@ -359,24 +336,42 @@ private static bool SetNestedProperty(object target, string path, JToken value, finalPropInfo.SetValue(currentObject, convertedValue); return true; } + error = $"Failed to convert value for '{finalPart}' to type '{finalPropInfo.PropertyType.Name}' in path '{path}'."; + return false; } - else + + FieldInfo finalFieldInfo = currentType.GetField(finalPart, flags); + if (finalFieldInfo != null) { - FieldInfo finalFieldInfo = currentType.GetField(finalPart, flags); - if (finalFieldInfo != null) + object convertedValue = ConvertJTokenToType(value, finalFieldInfo.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) { - object convertedValue = ConvertJTokenToType(value, finalFieldInfo.FieldType, inputSerializer); - if (convertedValue != null || value.Type == JTokenType.Null) - { - finalFieldInfo.SetValue(currentObject, convertedValue); - return true; - } + finalFieldInfo.SetValue(currentObject, convertedValue); + return true; + } + error = $"Failed to convert value for '{finalPart}' to type '{finalFieldInfo.FieldType.Name}' in path '{path}'."; + return false; + } + + // Try non-public [SerializeField] fields (nested paths need this too) + FieldInfo serializedField = ComponentOps.FindSerializedFieldInHierarchy(currentType, finalPart); + if (serializedField != null) + { + object convertedValue = ConvertJTokenToType(value, serializedField.FieldType, inputSerializer); + if (convertedValue != null || value.Type == JTokenType.Null) + { + serializedField.SetValue(currentObject, convertedValue); + return true; } + error = $"Failed to convert value for '{finalPart}' to type '{serializedField.FieldType.Name}' in path '{path}'."; + return false; } + + error = $"Property or field '{finalPart}' not found on type '{currentType.Name}' in path '{path}'."; } catch (Exception ex) { - McpLog.Error($"[SetNestedProperty] Error setting nested property '{path}': {ex.Message}\nToken: {value.ToString(Formatting.None)}"); + error = $"Error setting nested property '{path}': {ex.Message}"; } return false; diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs new file mode 100644 index 000000000..f8b09f5e9 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs @@ -0,0 +1,108 @@ +using System.Text.RegularExpressions; +using NUnit.Framework; +using UnityEngine; +using UnityEngine.TestTools; +using Newtonsoft.Json.Linq; +using MCPForUnity.Editor.Helpers; +using MCPForUnity.Editor.Tools; +using MCPForUnity.Editor.Tools.GameObjects; + +namespace MCPForUnityTests.Editor.Tools +{ + /// + /// Tests for GameObjectComponentHelpers.SetComponentPropertiesInternal error reporting. + /// Reproduces issue #765: conversion failures incorrectly reported as "Property not found". + /// + public class GameObjectComponentHelpersErrorTests + { + private GameObject testGo; + + [SetUp] + public void SetUp() + { + testGo = new GameObject("ErrorTestGO"); + CommandRegistry.Initialize(); + } + + [TearDown] + public void TearDown() + { + if (testGo != null) + Object.DestroyImmediate(testGo); + } + + /// + /// When a property exists but conversion fails, the error should say + /// "Failed to convert" rather than "Property not found. Did you mean: X?" + /// + [Test] + public void SetComponentProperties_ConversionFailure_ReportsConversionError_NotPropertyNotFound() + { + // Expect conversion error log from PropertyConversion (ComponentOps reflection attempt) + LogAssert.Expect(LogType.Error, new Regex("Error converting token")); + // Expect the warning log from SetComponentPropertiesInternal + LogAssert.Expect(LogType.Warning, new Regex("Failed to set")); + + var audioSource = testGo.AddComponent(); + + // spatialBlend is a float property — passing an array triggers conversion failure + var props = new JObject { ["spatialBlend"] = JArray.Parse("[1, 2, 3]") }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNotNull(result, "Should return an error response"); + Assert.IsInstanceOf(result); + + var errorResponse = (ErrorResponse)result; + + // The error message must NOT say "not found" for a property that exists + Assert.IsFalse( + errorResponse.Error.Contains("not found"), + $"Error should report conversion failure, not 'not found'. Got: {errorResponse.Error}"); + } + + /// + /// When a property genuinely doesn't exist, the error should still say "not found" with suggestions. + /// + [Test] + public void SetComponentProperties_NonexistentProperty_ReportsNotFound() + { + // Expect the "not found" warning + LogAssert.Expect(LogType.Warning, new Regex("not found")); + + var audioSource = testGo.AddComponent(); + + var props = new JObject { ["totallyFakeProperty"] = 42 }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNotNull(result); + Assert.IsInstanceOf(result); + + var errorResponse = (ErrorResponse)result; + + Assert.IsTrue( + errorResponse.Error.Contains("not found") || errorResponse.Error.Contains("failed"), + $"Error for nonexistent property should say 'not found'. Got: {errorResponse.Error}"); + } + + /// + /// Valid property setting should still succeed. + /// + [Test] + public void SetComponentProperties_ValidProperty_Succeeds() + { + var audioSource = testGo.AddComponent(); + + var props = new JObject { ["volume"] = 0.42f }; + + var result = GameObjectComponentHelpers.SetComponentPropertiesInternal( + testGo, "AudioSource", props, audioSource); + + Assert.IsNull(result, "Should return null on success (no errors)"); + Assert.AreEqual(0.42f, audioSource.volume, 0.001f); + } + } +} diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta new file mode 100644 index 000000000..cf28b93b3 --- /dev/null +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/GameObjectComponentHelpersErrorTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 51225acc846dc412b82750041eb0ee61 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: