Test/478 matrix4x4 serialization crash (#481)
* Fix #478: Add Matrix4x4Converter to prevent Cinemachine serialization crash The `get_components` action crashes Unity when serializing Cinemachine camera components because Newtonsoft.Json accesses computed Matrix4x4 properties (lossyScale, rotation) that call ValidTRS() on non-TRS matrices. This fix adds a safe Matrix4x4Converter that only accesses raw matrix elements (m00-m33), avoiding the dangerous computed properties entirely. Changes: - Add Matrix4x4Converter to UnityTypeConverters.cs - Register converter in GameObjectSerializer serializer settings Tested with Cinemachine 3.1.5 on Unity 6 - get_components now returns full component data without crashing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Add unit tests for Matrix4x4Converter Tests cover: - Identity matrix serialization/deserialization - Translation matrix round-trip - Degenerate matrix (determinant=0) - key regression test - Non-TRS matrix (projection) - validates ValidTRS() is never called - Null handling - Ensures dangerous properties are not in output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Address code review feedback - Fix null handling consistency: return zero matrix instead of identity (consistent with missing field defaults of 0f) - Improve degenerate matrix test to verify: - JSON only contains raw mXY properties - Values roundtrip correctly - Rename test to reflect expanded coverage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Move tests to TestProject per review feedback Moved Matrix4x4ConverterTests from MCPForUnity/Editor/Tests/ to TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ as requested. Also added MCPForUnity.Runtime reference to the test asmdef since the converter lives in the Runtime assembly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix Matrix4x4 deserialization guard + UI Toolkit USS warning --------- Co-authored-by: Alexander Mangel <cygnusfear@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>main
parent
90758f19f4
commit
91b6f4d8d6
|
|
@ -524,6 +524,7 @@ namespace MCPForUnity.Editor.Helpers
|
||||||
new ColorConverter(),
|
new ColorConverter(),
|
||||||
new RectConverter(),
|
new RectConverter(),
|
||||||
new BoundsConverter(),
|
new BoundsConverter(),
|
||||||
|
new Matrix4x4Converter(), // Fix #478: Safe Matrix4x4 serialization for Cinemachine
|
||||||
new UnityEngineObjectConverter() // Handles serialization of references
|
new UnityEngineObjectConverter() // Handles serialization of references
|
||||||
},
|
},
|
||||||
ReferenceLoopHandling = ReferenceLoopHandling.Ignore,
|
ReferenceLoopHandling = ReferenceLoopHandling.Ignore,
|
||||||
|
|
|
||||||
|
|
@ -4,7 +4,6 @@
|
||||||
flex-direction: column;
|
flex-direction: column;
|
||||||
flex-grow: 1;
|
flex-grow: 1;
|
||||||
overflow: hidden;
|
overflow: hidden;
|
||||||
box-sizing: border-box;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Title */
|
/* Title */
|
||||||
|
|
|
||||||
|
|
@ -160,6 +160,67 @@ namespace MCPForUnity.Runtime.Serialization
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Safe converter for Matrix4x4 that only accesses raw matrix elements (m00-m33).
|
||||||
|
/// Avoids computed properties (lossyScale, rotation, inverse) that call ValidTRS()
|
||||||
|
/// and can crash Unity on non-TRS matrices (common in Cinemachine components).
|
||||||
|
/// Fixes: https://github.com/CoplayDev/unity-mcp/issues/478
|
||||||
|
/// </summary>
|
||||||
|
public class Matrix4x4Converter : JsonConverter<Matrix4x4>
|
||||||
|
{
|
||||||
|
public override void WriteJson(JsonWriter writer, Matrix4x4 value, JsonSerializer serializer)
|
||||||
|
{
|
||||||
|
writer.WriteStartObject();
|
||||||
|
// Only access raw matrix elements - NEVER computed properties like lossyScale/rotation
|
||||||
|
writer.WritePropertyName("m00"); writer.WriteValue(value.m00);
|
||||||
|
writer.WritePropertyName("m01"); writer.WriteValue(value.m01);
|
||||||
|
writer.WritePropertyName("m02"); writer.WriteValue(value.m02);
|
||||||
|
writer.WritePropertyName("m03"); writer.WriteValue(value.m03);
|
||||||
|
writer.WritePropertyName("m10"); writer.WriteValue(value.m10);
|
||||||
|
writer.WritePropertyName("m11"); writer.WriteValue(value.m11);
|
||||||
|
writer.WritePropertyName("m12"); writer.WriteValue(value.m12);
|
||||||
|
writer.WritePropertyName("m13"); writer.WriteValue(value.m13);
|
||||||
|
writer.WritePropertyName("m20"); writer.WriteValue(value.m20);
|
||||||
|
writer.WritePropertyName("m21"); writer.WriteValue(value.m21);
|
||||||
|
writer.WritePropertyName("m22"); writer.WriteValue(value.m22);
|
||||||
|
writer.WritePropertyName("m23"); writer.WriteValue(value.m23);
|
||||||
|
writer.WritePropertyName("m30"); writer.WriteValue(value.m30);
|
||||||
|
writer.WritePropertyName("m31"); writer.WriteValue(value.m31);
|
||||||
|
writer.WritePropertyName("m32"); writer.WriteValue(value.m32);
|
||||||
|
writer.WritePropertyName("m33"); writer.WriteValue(value.m33);
|
||||||
|
writer.WriteEndObject();
|
||||||
|
}
|
||||||
|
|
||||||
|
public override Matrix4x4 ReadJson(JsonReader reader, Type objectType, Matrix4x4 existingValue, bool hasExistingValue, JsonSerializer serializer)
|
||||||
|
{
|
||||||
|
if (reader.TokenType == JsonToken.Null)
|
||||||
|
return new Matrix4x4(); // Return zero matrix for null (consistent with missing field defaults)
|
||||||
|
|
||||||
|
if (reader.TokenType != JsonToken.StartObject)
|
||||||
|
throw new JsonSerializationException($"Expected JSON object or null when deserializing Matrix4x4, got '{reader.TokenType}'.");
|
||||||
|
|
||||||
|
JObject jo = JObject.Load(reader);
|
||||||
|
var matrix = new Matrix4x4();
|
||||||
|
matrix.m00 = jo["m00"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m01 = jo["m01"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m02 = jo["m02"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m03 = jo["m03"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m10 = jo["m10"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m11 = jo["m11"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m12 = jo["m12"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m13 = jo["m13"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m20 = jo["m20"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m21 = jo["m21"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m22 = jo["m22"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m23 = jo["m23"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m30 = jo["m30"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m31 = jo["m31"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m32 = jo["m32"]?.Value<float>() ?? 0f;
|
||||||
|
matrix.m33 = jo["m33"]?.Value<float>() ?? 0f;
|
||||||
|
return matrix;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Converter for UnityEngine.Object references (GameObjects, Components, Materials, Textures, etc.)
|
// Converter for UnityEngine.Object references (GameObjects, Components, Materials, Textures, etc.)
|
||||||
public class UnityEngineObjectConverter : JsonConverter<UnityEngine.Object>
|
public class UnityEngineObjectConverter : JsonConverter<UnityEngine.Object>
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,134 @@
|
||||||
|
using System.Linq;
|
||||||
|
using MCPForUnity.Runtime.Serialization;
|
||||||
|
using Newtonsoft.Json;
|
||||||
|
using Newtonsoft.Json.Linq;
|
||||||
|
using NUnit.Framework;
|
||||||
|
using UnityEngine;
|
||||||
|
|
||||||
|
namespace MCPForUnityTests.EditMode.Helpers
|
||||||
|
{
|
||||||
|
/// <summary>
|
||||||
|
/// Tests for Matrix4x4Converter to ensure it safely serializes matrices
|
||||||
|
/// without accessing dangerous computed properties (lossyScale, rotation).
|
||||||
|
/// Regression test for https://github.com/CoplayDev/unity-mcp/issues/478
|
||||||
|
/// </summary>
|
||||||
|
public class Matrix4x4ConverterTests
|
||||||
|
{
|
||||||
|
private JsonSerializerSettings _settings;
|
||||||
|
|
||||||
|
[SetUp]
|
||||||
|
public void SetUp()
|
||||||
|
{
|
||||||
|
_settings = new JsonSerializerSettings
|
||||||
|
{
|
||||||
|
Converters = { new Matrix4x4Converter() }
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Serialize_IdentityMatrix_ReturnsCorrectJson()
|
||||||
|
{
|
||||||
|
var matrix = Matrix4x4.identity;
|
||||||
|
var json = JsonConvert.SerializeObject(matrix, _settings);
|
||||||
|
|
||||||
|
Assert.That(json, Does.Contain("\"m00\":1"));
|
||||||
|
Assert.That(json, Does.Contain("\"m11\":1"));
|
||||||
|
Assert.That(json, Does.Contain("\"m22\":1"));
|
||||||
|
Assert.That(json, Does.Contain("\"m33\":1"));
|
||||||
|
Assert.That(json, Does.Contain("\"m01\":0"));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Deserialize_IdentityMatrix_ReturnsIdentity()
|
||||||
|
{
|
||||||
|
var original = Matrix4x4.identity;
|
||||||
|
var json = JsonConvert.SerializeObject(original, _settings);
|
||||||
|
var result = JsonConvert.DeserializeObject<Matrix4x4>(json, _settings);
|
||||||
|
|
||||||
|
Assert.That(result, Is.EqualTo(original));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Serialize_TranslationMatrix_PreservesValues()
|
||||||
|
{
|
||||||
|
var matrix = Matrix4x4.Translate(new Vector3(10, 20, 30));
|
||||||
|
var json = JsonConvert.SerializeObject(matrix, _settings);
|
||||||
|
var result = JsonConvert.DeserializeObject<Matrix4x4>(json, _settings);
|
||||||
|
|
||||||
|
Assert.That(result.m03, Is.EqualTo(10f));
|
||||||
|
Assert.That(result.m13, Is.EqualTo(20f));
|
||||||
|
Assert.That(result.m23, Is.EqualTo(30f));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Serialize_DegenerateMatrix_DoesNotCrashAndRoundtrips()
|
||||||
|
{
|
||||||
|
// This is the key test - a degenerate matrix that would crash
|
||||||
|
// if we accessed lossyScale or rotation properties
|
||||||
|
var matrix = new Matrix4x4();
|
||||||
|
matrix.m00 = 0; matrix.m11 = 0; matrix.m22 = 0; // Degenerate - determinant = 0
|
||||||
|
|
||||||
|
// This should NOT throw or crash - the old code would fail here
|
||||||
|
var json = JsonConvert.SerializeObject(matrix, _settings);
|
||||||
|
var result = JsonConvert.DeserializeObject<Matrix4x4>(json, _settings);
|
||||||
|
|
||||||
|
// Verify JSON only contains raw mXY properties
|
||||||
|
var jo = JObject.Parse(json);
|
||||||
|
var expectedProps = new[]
|
||||||
|
{
|
||||||
|
"m00", "m01", "m02", "m03",
|
||||||
|
"m10", "m11", "m12", "m13",
|
||||||
|
"m20", "m21", "m22", "m23",
|
||||||
|
"m30", "m31", "m32", "m33"
|
||||||
|
};
|
||||||
|
CollectionAssert.AreEquivalent(expectedProps, jo.Properties().Select(p => p.Name).ToArray());
|
||||||
|
|
||||||
|
// Verify values roundtrip correctly (all zeros for degenerate matrix)
|
||||||
|
Assert.That(result.m00, Is.EqualTo(0f));
|
||||||
|
Assert.That(result.m11, Is.EqualTo(0f));
|
||||||
|
Assert.That(result.m22, Is.EqualTo(0f));
|
||||||
|
Assert.That(result, Is.EqualTo(matrix));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Serialize_NonTRSMatrix_DoesNotCrash()
|
||||||
|
{
|
||||||
|
// Projection matrices are NOT valid TRS matrices
|
||||||
|
// Accessing lossyScale/rotation on them causes ValidTRS() assertion
|
||||||
|
var matrix = Matrix4x4.Perspective(60f, 1.77f, 0.1f, 1000f);
|
||||||
|
|
||||||
|
// Verify it's not a valid TRS matrix
|
||||||
|
Assert.That(matrix.ValidTRS(), Is.False, "Test requires non-TRS matrix");
|
||||||
|
|
||||||
|
// This should NOT throw - the fix ensures we never access computed properties
|
||||||
|
Assert.DoesNotThrow(() =>
|
||||||
|
{
|
||||||
|
var json = JsonConvert.SerializeObject(matrix, _settings);
|
||||||
|
var result = JsonConvert.DeserializeObject<Matrix4x4>(json, _settings);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Deserialize_NullToken_ReturnsZeroMatrix()
|
||||||
|
{
|
||||||
|
var json = "null";
|
||||||
|
var result = JsonConvert.DeserializeObject<Matrix4x4>(json, _settings);
|
||||||
|
|
||||||
|
// Returns zero matrix (consistent with missing field defaults of 0f)
|
||||||
|
Assert.That(result, Is.EqualTo(new Matrix4x4()));
|
||||||
|
}
|
||||||
|
|
||||||
|
[Test]
|
||||||
|
public void Serialize_DoesNotContainDangerousProperties()
|
||||||
|
{
|
||||||
|
var matrix = Matrix4x4.TRS(Vector3.one, Quaternion.identity, Vector3.one);
|
||||||
|
var json = JsonConvert.SerializeObject(matrix, _settings);
|
||||||
|
|
||||||
|
// Ensure we're not serializing the dangerous computed properties
|
||||||
|
Assert.That(json, Does.Not.Contain("lossyScale"));
|
||||||
|
Assert.That(json, Does.Not.Contain("rotation"));
|
||||||
|
Assert.That(json, Does.Not.Contain("inverse"));
|
||||||
|
Assert.That(json, Does.Not.Contain("transpose"));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,11 @@
|
||||||
|
fileFormatVersion: 2
|
||||||
|
guid: d2c5dd202b4944675ae606581676a24a
|
||||||
|
MonoImporter:
|
||||||
|
externalObjects: {}
|
||||||
|
serializedVersion: 2
|
||||||
|
defaultReferences: []
|
||||||
|
executionOrder: 0
|
||||||
|
icon: {instanceID: 0}
|
||||||
|
userData:
|
||||||
|
assetBundleName:
|
||||||
|
assetBundleVariant:
|
||||||
|
|
@ -3,6 +3,7 @@
|
||||||
"rootNamespace": "",
|
"rootNamespace": "",
|
||||||
"references": [
|
"references": [
|
||||||
"MCPForUnity.Editor",
|
"MCPForUnity.Editor",
|
||||||
|
"MCPForUnity.Runtime",
|
||||||
"TestAsmdef",
|
"TestAsmdef",
|
||||||
"UnityEngine.TestRunner",
|
"UnityEngine.TestRunner",
|
||||||
"UnityEditor.TestRunner"
|
"UnityEditor.TestRunner"
|
||||||
|
|
@ -23,4 +24,4 @@
|
||||||
],
|
],
|
||||||
"versionDefines": [],
|
"versionDefines": [],
|
||||||
"noEngineReferences": false
|
"noEngineReferences": false
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue