From 51eb59f04fa70c39d4e574282511d32503d70652 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Thu, 10 Apr 2025 12:52:18 -0700 Subject: [PATCH] Fix(MCP): Resolve ValidTRS crash during component serialization The Unity Editor was crashing with ValidTRS() assertions when attempting to get components from certain GameObjects like the Main Camera. Investigation revealed the crash occurred during JSON serialization when reflection code accessed specific matrix properties (e.g., Camera.cullingMatrix, Transform.rotation, Transform.lossyScale). Accessing these properties appears to trigger internal Transform state validation failures, potentially due to interactions with the JSON serializer's reflection mechanism. This fix addresses the issue by: - Replacing LINQ iteration in GetComponentsFromTarget with a standard loop over a copied list to prevent potential premature serialization interactions. - Explicitly skipping known problematic Camera matrix properties (cullingMatrix, pixelRect, rect) and generic matrix properties (worldToLocalMatrix, localToWorldMatrix) within GetComponentData's reflection logic. - Retaining manual serialization for Transform component properties to avoid related reflection issues. --- .../Editor/Helpers/GameObjectSerializer.cs | 96 +++++++++++++++++-- .../Editor/Tools/ManageGameObject.cs | 53 +++++++++- 2 files changed, 135 insertions(+), 14 deletions(-) diff --git a/UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs b/UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs index 8a65ada..1a1b462 100644 --- a/UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs +++ b/UnityMcpBridge/Editor/Helpers/GameObjectSerializer.cs @@ -13,7 +13,7 @@ namespace UnityMcpBridge.Editor.Helpers /// /// Handles serialization of GameObjects and Components for MCP responses. /// Includes reflection helpers and caching for performance. - /// + /// tew public static class GameObjectSerializer { // --- Data Serialization --- @@ -121,9 +121,42 @@ namespace UnityMcpBridge.Editor.Helpers // Add the flag parameter here public static object GetComponentData(Component c, bool includeNonPublicSerializedFields = true) { + // --- Add Early Logging --- + // Debug.Log($"[GetComponentData] Starting for component: {c?.GetType()?.FullName ?? "null"} (ID: {c?.GetInstanceID() ?? 0})"); + // --- End Early Logging --- + if (c == null) return null; Type componentType = c.GetType(); + // --- Special handling for Transform to avoid reflection crashes and problematic properties --- + if (componentType == typeof(Transform)) + { + Transform tr = c as Transform; + // Debug.Log($"[GetComponentData] Manually serializing Transform (ID: {tr.GetInstanceID()})"); + return new Dictionary + { + { "typeName", componentType.FullName }, + { "instanceID", tr.GetInstanceID() }, + // Manually extract known-safe properties. Avoid Quaternion 'rotation' and 'lossyScale'. + { "position", CreateTokenFromValue(tr.position, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "localPosition", CreateTokenFromValue(tr.localPosition, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "eulerAngles", CreateTokenFromValue(tr.eulerAngles, typeof(Vector3))?.ToObject() ?? new JObject() }, // Use Euler angles + { "localEulerAngles", CreateTokenFromValue(tr.localEulerAngles, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "localScale", CreateTokenFromValue(tr.localScale, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "right", CreateTokenFromValue(tr.right, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "up", CreateTokenFromValue(tr.up, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "forward", CreateTokenFromValue(tr.forward, typeof(Vector3))?.ToObject() ?? new JObject() }, + { "parentInstanceID", tr.parent?.gameObject.GetInstanceID() ?? 0 }, + { "rootInstanceID", tr.root?.gameObject.GetInstanceID() ?? 0 }, + { "childCount", tr.childCount }, + // Include standard Object/Component properties + { "name", tr.name }, + { "tag", tr.tag }, + { "gameObjectInstanceID", tr.gameObject?.GetInstanceID() ?? 0 } + }; + } + // --- End Special handling for Transform --- + var data = new Dictionary { { "typeName", componentType.FullName }, @@ -195,11 +228,18 @@ namespace UnityMcpBridge.Editor.Helpers // --- Use cached metadata --- var serializablePropertiesOutput = new Dictionary(); + + // --- Add Logging Before Property Loop --- + // Debug.Log($"[GetComponentData] Starting property loop for {componentType.Name}..."); + // --- End Logging Before Property Loop --- + // Use cached properties foreach (var propInfo in cachedData.SerializableProperties) { - // --- Skip known obsolete/problematic Component shortcut properties --- string propName = propInfo.Name; + + // --- Skip known obsolete/problematic Component shortcut properties --- + bool skipProperty = false; if (propName == "rigidbody" || propName == "rigidbody2D" || propName == "camera" || propName == "light" || propName == "animation" || propName == "constantForce" || propName == "renderer" || propName == "audio" || propName == "networkView" || @@ -208,27 +248,63 @@ namespace UnityMcpBridge.Editor.Helpers // Also skip potentially problematic Matrix properties prone to cycles/errors propName == "worldToLocalMatrix" || propName == "localToWorldMatrix") { - continue; // Skip these properties + // Debug.Log($"[GetComponentData] Explicitly skipping generic property: {propName}"); // Optional log + skipProperty = true; + } + // --- End Skip Generic Properties --- + + // --- Skip specific potentially problematic Camera properties --- + if (componentType == typeof(Camera) && + (propName == "pixelRect" || + propName == "rect" || + propName == "cullingMatrix")) + { + // Debug.Log($"[GetComponentData] Explicitly skipping Camera property: {propName}"); + skipProperty = true; + } + // --- End Skip Camera Properties --- + + // --- Skip specific potentially problematic Transform properties --- + if (componentType == typeof(Transform) && (propName == "lossyScale" || propName == "rotation")) + { + // Debug.Log($"[GetComponentData] Explicitly skipping Transform property: {propName}"); + skipProperty = true; + } + // --- End Skip Transform Properties --- + + // Skip if flagged + if (skipProperty) + { + continue; } - // --- End Skip --- try { + // --- Add detailed logging --- + // Debug.Log($"[GetComponentData] Accessing: {componentType.Name}.{propName}"); + // --- End detailed logging --- object value = propInfo.GetValue(c); Type propType = propInfo.PropertyType; AddSerializableValue(serializablePropertiesOutput, propName, propType, value); } catch (Exception ex) { - Debug.LogWarning($"Could not read property {propName} on {componentType.Name}: {ex.Message}"); + // Debug.LogWarning($"Could not read property {propName} on {componentType.Name}: {ex.Message}"); } } + // --- Add Logging Before Field Loop --- + // Debug.Log($"[GetComponentData] Starting field loop for {componentType.Name}..."); + // --- End Logging Before Field Loop --- + // Use cached fields foreach (var fieldInfo in cachedData.SerializableFields) { try { + // --- Add detailed logging for fields --- + // Debug.Log($"[GetComponentData] Accessing Field: {componentType.Name}.{fieldInfo.Name}"); + // --- End detailed logging for fields --- object value = fieldInfo.GetValue(c); string fieldName = fieldInfo.Name; Type fieldType = fieldInfo.FieldType; @@ -236,7 +312,7 @@ namespace UnityMcpBridge.Editor.Helpers } catch (Exception ex) { - Debug.LogWarning($"Could not read field {fieldInfo.Name} on {componentType.Name}: {ex.Message}"); + // Debug.LogWarning($"Could not read field {fieldInfo.Name} on {componentType.Name}: {ex.Message}"); } } // --- End Use cached metadata --- @@ -273,7 +349,7 @@ namespace UnityMcpBridge.Editor.Helpers catch (Exception e) { // Catch potential errors during JToken conversion or addition to dictionary - Debug.LogWarning($"[AddSerializableValue] Error processing value for '{name}' (Type: {type.FullName}): {e.Message}. Skipping."); + // Debug.LogWarning($"[AddSerializableValue] Error processing value for '{name}' (Type: {type.FullName}): {e.Message}. Skipping."); } } @@ -329,7 +405,7 @@ namespace UnityMcpBridge.Editor.Helpers { return jValue.Value; } - Debug.LogWarning($"Unsupported JTokenType encountered: {token.Type}. Returning null."); + // Debug.LogWarning($"Unsupported JTokenType encountered: {token.Type}. Returning null."); return null; } } @@ -365,12 +441,12 @@ namespace UnityMcpBridge.Editor.Helpers } catch (JsonSerializationException e) { - Debug.LogWarning($"[GameObjectSerializer] Newtonsoft.Json Error serializing value of type {type.FullName}: {e.Message}. Skipping property/field."); + // Debug.LogWarning($"[GameObjectSerializer] Newtonsoft.Json Error serializing value of type {type.FullName}: {e.Message}. Skipping property/field."); return null; // Indicate serialization failure } catch (Exception e) // Catch other unexpected errors { - Debug.LogWarning($"[GameObjectSerializer] Unexpected error serializing value of type {type.FullName}: {e}. Skipping property/field."); + // Debug.LogWarning($"[GameObjectSerializer] Unexpected error serializing value of type {type.FullName}: {e}. Skipping property/field."); return null; // Indicate serialization failure } } diff --git a/UnityMcpBridge/Editor/Tools/ManageGameObject.cs b/UnityMcpBridge/Editor/Tools/ManageGameObject.cs index 1e22ebd..66c64cb 100644 --- a/UnityMcpBridge/Editor/Tools/ManageGameObject.cs +++ b/UnityMcpBridge/Editor/Tools/ManageGameObject.cs @@ -837,12 +837,57 @@ namespace UnityMcpBridge.Editor.Tools try { - Component[] components = targetGo.GetComponents(); - // Use the new serializer helper and pass the flag - var componentData = components.Select(c => Helpers.GameObjectSerializer.GetComponentData(c, includeNonPublicSerialized)).ToList(); + // --- Get components, immediately copy to list, and null original array --- + Component[] originalComponents = targetGo.GetComponents(); + List componentsToIterate = new List(originalComponents ?? Array.Empty()); // Copy immediately, handle null case + int componentCount = componentsToIterate.Count; + originalComponents = null; // Null the original reference + // Debug.Log($"[GetComponentsFromTarget] Found {componentCount} components on {targetGo.name}. Copied to list, nulled original. Starting REVERSE for loop..."); + // --- End Copy and Null --- + + var componentData = new List(); + + for (int i = componentCount - 1; i >= 0; i--) // Iterate backwards over the COPY + { + Component c = componentsToIterate[i]; // Use the copy + if (c == null) + { + // Debug.LogWarning($"[GetComponentsFromTarget REVERSE for] Encountered a null component at index {i} on {targetGo.name}. Skipping."); + continue; // Safety check + } + // Debug.Log($"[GetComponentsFromTarget REVERSE for] Processing component: {c.GetType()?.FullName ?? "null"} (ID: {c.GetInstanceID()}) at index {i} on {targetGo.name}"); + try + { + var data = Helpers.GameObjectSerializer.GetComponentData(c, includeNonPublicSerialized); + if (data != null) // Ensure GetComponentData didn't return null + { + componentData.Insert(0, data); // Insert at beginning to maintain original order in final list + } + // else + // { + // Debug.LogWarning($"[GetComponentsFromTarget REVERSE for] GetComponentData returned null for component {c.GetType().FullName} (ID: {c.GetInstanceID()}) on {targetGo.name}. Skipping addition."); + // } + } + catch (Exception ex) + { + Debug.LogError($"[GetComponentsFromTarget REVERSE for] Error processing component {c.GetType().FullName} (ID: {c.GetInstanceID()}) on {targetGo.name}: {ex.Message}\n{ex.StackTrace}"); + // Optionally add placeholder data or just skip + componentData.Insert(0, new JObject( // Insert error marker at beginning + new JProperty("typeName", c.GetType().FullName + " (Serialization Error)"), + new JProperty("instanceID", c.GetInstanceID()), + new JProperty("error", ex.Message) + )); + } + } + // Debug.Log($"[GetComponentsFromTarget] Finished REVERSE for loop."); + + // Cleanup the list we created + componentsToIterate.Clear(); + componentsToIterate = null; + return Response.Success( $"Retrieved {componentData.Count} components from '{targetGo.name}'.", - componentData + componentData // List was built in original order ); } catch (Exception e)