fix: Add special handling for UIDocument serialization to prevent infinite loops (#586)
* fix: search inactive objects when setActive=true in modify When trying to activate an inactive GameObject via manage_gameobject modify with setActive=true, the lookup would fail because inactive objects were not included in the search by default. Now automatically sets searchInactive=true when setActive=true is specified, allowing inactive objects to be found and activated. * fix: Add special handling for UIDocument serialization to prevent infinite loops (#585) UIDocument.rootVisualElement contains circular parent/child references that can cause infinite serialization loops. This adds special handling similar to Transform and Camera components. The fix: - Safely serializes panelSettings, visualTreeAsset, sortingOrder, enabled, parentUI - Explicitly skips rootVisualElement to prevent circular reference issues - Includes a note explaining why rootVisualElement is skipped Tested on Unity 2021.3 and Unity 6.3. * refactor: Extract SerializeAssetReference helper and align UIDocument structure - Add SerializeAssetReference() helper for consistent asset reference serialization - UIDocument now uses same return structure as Camera (typeName, instanceID, properties) - Reduces code duplication in special-case handlers - Enhanced test coverage to verify structure matches Camera pattern * fix: Handle UIDocument subclasses and add negative assertion for rootVisualElement Address code review feedback: - Add IsOrDerivedFrom() helper to detect UIDocument and any subclasses by walking the base-type chain, ensuring derived types also get special-case handling - Add negative assertion verifying rootVisualElement is NOT in serialized outputmain
parent
0158165064
commit
aaf6308b33
|
|
@ -114,6 +114,48 @@ namespace MCPForUnity.Editor.Helpers
|
||||||
private static readonly Dictionary<Tuple<Type, bool>, CachedMetadata> _metadataCache = new Dictionary<Tuple<Type, bool>, CachedMetadata>();
|
private static readonly Dictionary<Tuple<Type, bool>, CachedMetadata> _metadataCache = new Dictionary<Tuple<Type, bool>, CachedMetadata>();
|
||||||
// --- End Metadata Caching ---
|
// --- End Metadata Caching ---
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Checks if a type is or derives from a type with the specified full name.
|
||||||
|
/// Used to detect special-case components including their subclasses.
|
||||||
|
/// </summary>
|
||||||
|
private static bool IsOrDerivedFrom(Type type, string baseTypeFullName)
|
||||||
|
{
|
||||||
|
Type current = type;
|
||||||
|
while (current != null)
|
||||||
|
{
|
||||||
|
if (current.FullName == baseTypeFullName)
|
||||||
|
return true;
|
||||||
|
current = current.BaseType;
|
||||||
|
}
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Serializes a UnityEngine.Object reference to a dictionary with name, instanceID, and assetPath.
|
||||||
|
/// Used for consistent serialization of asset references in special-case component handlers.
|
||||||
|
/// </summary>
|
||||||
|
/// <param name="obj">The Unity object to serialize</param>
|
||||||
|
/// <param name="includeAssetPath">Whether to include the asset path (default true)</param>
|
||||||
|
/// <returns>A dictionary with the object's reference info, or null if obj is null</returns>
|
||||||
|
private static Dictionary<string, object> SerializeAssetReference(UnityEngine.Object obj, bool includeAssetPath = true)
|
||||||
|
{
|
||||||
|
if (obj == null) return null;
|
||||||
|
|
||||||
|
var result = new Dictionary<string, object>
|
||||||
|
{
|
||||||
|
{ "name", obj.name },
|
||||||
|
{ "instanceID", obj.GetInstanceID() }
|
||||||
|
};
|
||||||
|
|
||||||
|
if (includeAssetPath)
|
||||||
|
{
|
||||||
|
var assetPath = AssetDatabase.GetAssetPath(obj);
|
||||||
|
result["assetPath"] = string.IsNullOrEmpty(assetPath) ? null : assetPath;
|
||||||
|
}
|
||||||
|
|
||||||
|
return result;
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Creates a serializable representation of a Component, attempting to serialize
|
/// Creates a serializable representation of a Component, attempting to serialize
|
||||||
/// public properties and fields using reflection, with caching and control over non-public fields.
|
/// public properties and fields using reflection, with caching and control over non-public fields.
|
||||||
|
|
@ -220,6 +262,72 @@ namespace MCPForUnity.Editor.Helpers
|
||||||
}
|
}
|
||||||
// --- End Special handling for Camera ---
|
// --- End Special handling for Camera ---
|
||||||
|
|
||||||
|
// --- Special handling for UIDocument to avoid infinite loops from VisualElement hierarchy (Issue #585) ---
|
||||||
|
// UIDocument.rootVisualElement contains circular parent/child references that cause infinite serialization loops.
|
||||||
|
// Use IsOrDerivedFrom to also catch subclasses of UIDocument.
|
||||||
|
if (IsOrDerivedFrom(componentType, "UnityEngine.UIElements.UIDocument"))
|
||||||
|
{
|
||||||
|
var uiDocProperties = new Dictionary<string, object>();
|
||||||
|
|
||||||
|
try
|
||||||
|
{
|
||||||
|
// Get panelSettings reference safely
|
||||||
|
var panelSettingsProp = componentType.GetProperty("panelSettings");
|
||||||
|
if (panelSettingsProp != null)
|
||||||
|
{
|
||||||
|
var panelSettings = panelSettingsProp.GetValue(c) as UnityEngine.Object;
|
||||||
|
uiDocProperties["panelSettings"] = SerializeAssetReference(panelSettings);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get visualTreeAsset reference safely (the UXML file)
|
||||||
|
var visualTreeAssetProp = componentType.GetProperty("visualTreeAsset");
|
||||||
|
if (visualTreeAssetProp != null)
|
||||||
|
{
|
||||||
|
var visualTreeAsset = visualTreeAssetProp.GetValue(c) as UnityEngine.Object;
|
||||||
|
uiDocProperties["visualTreeAsset"] = SerializeAssetReference(visualTreeAsset);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get sortingOrder safely
|
||||||
|
var sortingOrderProp = componentType.GetProperty("sortingOrder");
|
||||||
|
if (sortingOrderProp != null)
|
||||||
|
{
|
||||||
|
uiDocProperties["sortingOrder"] = sortingOrderProp.GetValue(c);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get enabled state (from Behaviour base class)
|
||||||
|
var enabledProp = componentType.GetProperty("enabled");
|
||||||
|
if (enabledProp != null)
|
||||||
|
{
|
||||||
|
uiDocProperties["enabled"] = enabledProp.GetValue(c);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get parentUI reference safely (no asset path needed - it's a scene reference)
|
||||||
|
var parentUIProp = componentType.GetProperty("parentUI");
|
||||||
|
if (parentUIProp != null)
|
||||||
|
{
|
||||||
|
var parentUI = parentUIProp.GetValue(c) as UnityEngine.Object;
|
||||||
|
uiDocProperties["parentUI"] = SerializeAssetReference(parentUI, includeAssetPath: false);
|
||||||
|
}
|
||||||
|
|
||||||
|
// NOTE: rootVisualElement is intentionally skipped - it contains circular
|
||||||
|
// parent/child references that cause infinite serialization loops
|
||||||
|
uiDocProperties["_note"] = "rootVisualElement skipped to prevent circular reference loops";
|
||||||
|
}
|
||||||
|
catch (Exception e)
|
||||||
|
{
|
||||||
|
McpLog.Warn($"[GetComponentData] Error reading UIDocument properties: {e.Message}");
|
||||||
|
}
|
||||||
|
|
||||||
|
// Return structure matches Camera special handling (typeName, instanceID, properties)
|
||||||
|
return new Dictionary<string, object>
|
||||||
|
{
|
||||||
|
{ "typeName", componentType.FullName },
|
||||||
|
{ "instanceID", c.GetInstanceID() },
|
||||||
|
{ "properties", uiDocProperties }
|
||||||
|
};
|
||||||
|
}
|
||||||
|
// --- End Special handling for UIDocument ---
|
||||||
|
|
||||||
var data = new Dictionary<string, object>
|
var data = new Dictionary<string, object>
|
||||||
{
|
{
|
||||||
{ "typeName", componentType.FullName },
|
{ "typeName", componentType.FullName },
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,247 @@
|
||||||
|
using System;
|
||||||
|
using System.Collections.Generic;
|
||||||
|
using NUnit.Framework;
|
||||||
|
using UnityEngine;
|
||||||
|
using UnityEngine.UIElements;
|
||||||
|
using UnityEditor;
|
||||||
|
using MCPForUnity.Editor.Helpers;
|
||||||
|
|
||||||
|
namespace MCPForUnityTests.Editor.Tools
|
||||||
|
{
|
||||||
|
/// <summary>
|
||||||
|
/// Tests for UIDocument component serialization.
|
||||||
|
/// Reproduces issue #585: UIDocument component causes infinite loop when serializing components
|
||||||
|
/// due to circular parent/child references in rootVisualElement.
|
||||||
|
/// </summary>
|
||||||
|
public class UIDocumentSerializationTests
|
||||||
|
{
|
||||||
|
private GameObject testGameObject;
|
||||||
|
private PanelSettings testPanelSettings;
|
||||||
|
private VisualTreeAsset testVisualTreeAsset;
|
||||||
|
|
||||||
|
[SetUp]
|
||||||
|
public void SetUp()
|
||||||
|
{
|
||||||
|
// Create a test GameObject
|
||||||
|
testGameObject = new GameObject("UIDocumentTestObject");
|
||||||
|
|
||||||
|
// Create PanelSettings asset (required for UIDocument to have a rootVisualElement)
|
||||||
|
testPanelSettings = ScriptableObject.CreateInstance<PanelSettings>();
|
||||||
|
|
||||||
|
// Create a minimal VisualTreeAsset
|
||||||
|
// Note: VisualTreeAsset cannot be created via CreateInstance, we need to use AssetDatabase
|
||||||
|
// For the test, we'll create a temporary UXML file
|
||||||
|
CreateTestVisualTreeAsset();
|
||||||
|
}
|
||||||
|
|
||||||
|
[TearDown]
|
||||||
|
public void TearDown()
|
||||||
|
{
|
||||||
|
// Clean up test GameObject
|
||||||
|
if (testGameObject != null)
|
||||||
|
{
|
||||||
|
UnityEngine.Object.DestroyImmediate(testGameObject);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Clean up ScriptableObject instances
|
||||||
|
if (testPanelSettings != null)
|
||||||
|
{
|
||||||
|
UnityEngine.Object.DestroyImmediate(testPanelSettings);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Clean up temporary UXML file
|
||||||
|
CleanupTestVisualTreeAsset();
|
||||||
|
}
|
||||||
|
|
||||||
|
private void CreateTestVisualTreeAsset()
|
||||||
|
{
|
||||||
|
// Create a minimal UXML file for testing
|
||||||
|
string uxmlPath = "Assets/Tests/EditMode/Tools/TestUIDocument.uxml";
|
||||||
|
string uxmlContent = @"<ui:UXML xmlns:ui=""UnityEngine.UIElements"">
|
||||||
|
<ui:VisualElement name=""root"">
|
||||||
|
<ui:Label text=""Test Label"" />
|
||||||
|
</ui:VisualElement>
|
||||||
|
</ui:UXML>";
|
||||||
|
|
||||||
|
// Ensure directory exists
|
||||||
|
string directory = System.IO.Path.GetDirectoryName(uxmlPath);
|
||||||
|
if (!System.IO.Directory.Exists(directory))
|
||||||
|
{
|
||||||
|
System.IO.Directory.CreateDirectory(directory);
|
||||||
|
}
|
||||||
|
|
||||||
|
System.IO.File.WriteAllText(uxmlPath, uxmlContent);
|
||||||
|
AssetDatabase.Refresh();
|
||||||
|
|
||||||
|
testVisualTreeAsset = AssetDatabase.LoadAssetAtPath<VisualTreeAsset>(uxmlPath);
|
||||||
|
}
|
||||||
|
|
||||||
|
private void CleanupTestVisualTreeAsset()
|
||||||
|
{
|
||||||
|
string uxmlPath = "Assets/Tests/EditMode/Tools/TestUIDocument.uxml";
|
||||||
|
if (System.IO.File.Exists(uxmlPath))
|
||||||
|
{
|
||||||
|
AssetDatabase.DeleteAsset(uxmlPath);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test that UIDocument component can be serialized without infinite loops.
|
||||||
|
/// This test reproduces issue #585 where UIDocument causes infinite loop
|
||||||
|
/// when both visualTreeAsset and panelSettings are assigned.
|
||||||
|
///
|
||||||
|
/// The bug: UIDocument.rootVisualElement returns a VisualElement with circular
|
||||||
|
/// parent/child references (children[] → child elements → parent → back to parent).
|
||||||
|
///
|
||||||
|
/// Note: NUnit [Timeout] will fail this test if serialization hangs.
|
||||||
|
/// </summary>
|
||||||
|
[Test]
|
||||||
|
[Timeout(10000)] // 10 second timeout - if serialization hangs, test fails
|
||||||
|
public void GetComponentData_UIDocument_WithBothAssetsAssigned_DoesNotHang()
|
||||||
|
{
|
||||||
|
// Skip test if we couldn't create the VisualTreeAsset
|
||||||
|
if (testVisualTreeAsset == null)
|
||||||
|
{
|
||||||
|
Assert.Inconclusive("Could not create test VisualTreeAsset - test cannot run");
|
||||||
|
}
|
||||||
|
|
||||||
|
// Arrange - Add UIDocument component with both assets assigned
|
||||||
|
var uiDocument = testGameObject.AddComponent<UIDocument>();
|
||||||
|
uiDocument.panelSettings = testPanelSettings;
|
||||||
|
uiDocument.visualTreeAsset = testVisualTreeAsset;
|
||||||
|
|
||||||
|
// Act - This should NOT hang or cause infinite loop
|
||||||
|
// The [Timeout] attribute will fail the test if it takes too long
|
||||||
|
object result = GameObjectSerializer.GetComponentData(uiDocument);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.IsNotNull(result, "Should return serialized component data");
|
||||||
|
|
||||||
|
var resultDict = result as Dictionary<string, object>;
|
||||||
|
Assert.IsNotNull(resultDict, "Result should be a dictionary");
|
||||||
|
Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"]);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test that UIDocument serialization includes expected properties.
|
||||||
|
/// Verifies the structure matches Camera special handling (typeName, instanceID, properties).
|
||||||
|
/// </summary>
|
||||||
|
[Test]
|
||||||
|
[Timeout(10000)]
|
||||||
|
public void GetComponentData_UIDocument_ReturnsExpectedProperties()
|
||||||
|
{
|
||||||
|
// Skip test if we couldn't create the VisualTreeAsset
|
||||||
|
if (testVisualTreeAsset == null)
|
||||||
|
{
|
||||||
|
Assert.Inconclusive("Could not create test VisualTreeAsset - test cannot run");
|
||||||
|
}
|
||||||
|
|
||||||
|
// Arrange
|
||||||
|
var uiDocument = testGameObject.AddComponent<UIDocument>();
|
||||||
|
uiDocument.panelSettings = testPanelSettings;
|
||||||
|
uiDocument.visualTreeAsset = testVisualTreeAsset;
|
||||||
|
uiDocument.sortingOrder = 42;
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = GameObjectSerializer.GetComponentData(uiDocument);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.IsNotNull(result, "Should return serialized component data");
|
||||||
|
|
||||||
|
var resultDict = result as Dictionary<string, object>;
|
||||||
|
Assert.IsNotNull(resultDict, "Result should be a dictionary");
|
||||||
|
|
||||||
|
// Check for expected top-level keys (matches Camera special handling structure)
|
||||||
|
Assert.IsTrue(resultDict.ContainsKey("typeName"), "Should contain typeName");
|
||||||
|
Assert.IsTrue(resultDict.ContainsKey("instanceID"), "Should contain instanceID");
|
||||||
|
Assert.IsTrue(resultDict.ContainsKey("properties"), "Should contain properties");
|
||||||
|
|
||||||
|
// Verify type name
|
||||||
|
Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"],
|
||||||
|
"typeName should be UIDocument");
|
||||||
|
|
||||||
|
// Verify properties dict contains expected keys
|
||||||
|
var properties = resultDict["properties"] as Dictionary<string, object>;
|
||||||
|
Assert.IsNotNull(properties, "properties should be a dictionary");
|
||||||
|
Assert.IsTrue(properties.ContainsKey("panelSettings"), "Should have panelSettings");
|
||||||
|
Assert.IsTrue(properties.ContainsKey("visualTreeAsset"), "Should have visualTreeAsset");
|
||||||
|
Assert.IsTrue(properties.ContainsKey("sortingOrder"), "Should have sortingOrder");
|
||||||
|
Assert.IsTrue(properties.ContainsKey("enabled"), "Should have enabled");
|
||||||
|
Assert.IsTrue(properties.ContainsKey("_note"), "Should have _note about skipped rootVisualElement");
|
||||||
|
|
||||||
|
// CRITICAL: Verify rootVisualElement is NOT included (this is the fix for Issue #585)
|
||||||
|
Assert.IsFalse(properties.ContainsKey("rootVisualElement"),
|
||||||
|
"Should NOT include rootVisualElement - it causes circular reference loops");
|
||||||
|
|
||||||
|
// Verify asset references use consistent structure (name, instanceID, assetPath)
|
||||||
|
var panelSettingsRef = properties["panelSettings"] as Dictionary<string, object>;
|
||||||
|
Assert.IsNotNull(panelSettingsRef, "panelSettings should be serialized as dictionary");
|
||||||
|
Assert.IsTrue(panelSettingsRef.ContainsKey("name"), "panelSettings should have name");
|
||||||
|
Assert.IsTrue(panelSettingsRef.ContainsKey("instanceID"), "panelSettings should have instanceID");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test that UIDocument WITHOUT assets assigned doesn't cause issues.
|
||||||
|
/// This is a baseline test - the bug only occurs with both assets assigned.
|
||||||
|
/// </summary>
|
||||||
|
[Test]
|
||||||
|
public void GetComponentData_UIDocument_WithoutAssets_Succeeds()
|
||||||
|
{
|
||||||
|
// Arrange - Add UIDocument component WITHOUT assets
|
||||||
|
var uiDocument = testGameObject.AddComponent<UIDocument>();
|
||||||
|
// Don't assign panelSettings or visualTreeAsset
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = GameObjectSerializer.GetComponentData(uiDocument);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.IsNotNull(result, "Should return serialized component data");
|
||||||
|
|
||||||
|
var resultDict = result as Dictionary<string, object>;
|
||||||
|
Assert.IsNotNull(resultDict, "Result should be a dictionary");
|
||||||
|
Assert.AreEqual("UnityEngine.UIElements.UIDocument", resultDict["typeName"]);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test that UIDocument with only panelSettings assigned doesn't cause issues.
|
||||||
|
/// </summary>
|
||||||
|
[Test]
|
||||||
|
public void GetComponentData_UIDocument_WithOnlyPanelSettings_Succeeds()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var uiDocument = testGameObject.AddComponent<UIDocument>();
|
||||||
|
uiDocument.panelSettings = testPanelSettings;
|
||||||
|
// Don't assign visualTreeAsset
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = GameObjectSerializer.GetComponentData(uiDocument);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.IsNotNull(result, "Should return serialized component data");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Test that UIDocument with only visualTreeAsset assigned doesn't cause issues.
|
||||||
|
/// </summary>
|
||||||
|
[Test]
|
||||||
|
public void GetComponentData_UIDocument_WithOnlyVisualTreeAsset_Succeeds()
|
||||||
|
{
|
||||||
|
// Skip test if we couldn't create the VisualTreeAsset
|
||||||
|
if (testVisualTreeAsset == null)
|
||||||
|
{
|
||||||
|
Assert.Inconclusive("Could not create test VisualTreeAsset - test cannot run");
|
||||||
|
}
|
||||||
|
|
||||||
|
// Arrange
|
||||||
|
var uiDocument = testGameObject.AddComponent<UIDocument>();
|
||||||
|
uiDocument.visualTreeAsset = testVisualTreeAsset;
|
||||||
|
// Don't assign panelSettings
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var result = GameObjectSerializer.GetComponentData(uiDocument);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.IsNotNull(result, "Should return serialized component data");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in New Issue