Fix material mesh instantiation warnings #331
Conversation
- GameObjectSerializer: in edit mode, normalize material/mesh access - material -> sharedMaterial - materials -> sharedMaterials - mesh -> sharedMesh - also guard materials/sharedMaterial/sharedMaterials property names - Tests: add strong instanceID equality checks for Renderer/MeshFilter - prove no instantiation by shared instanceID before/after - cover multi-material, null cases; prune brittle presence/console tests - Clean up and relax ManageGameObject tests to avoid false negatives
…[SerializeField]; fix tests to destroy temp primitives and use dictionary access; strengthen instanceID assertions
- ManageGameObjectTests.cs: Destroy temporary primitive GameObject after extracting sharedMesh - MaterialMeshInstantiationTests.cs: Destroy instantiated uniqueMesh after test completion These fixes prevent resource leaks in Unity test files by properly cleaning up temporary objects created during tests.
- Replace incorrect reflection-based field access with proper dictionary key access - GetComponentData() returns Dictionary<string, object> where 'properties' is a key, not a field - Tests now properly access the properties dictionary and run assertions - Fixes ineffective tests that were silently failing due to reflection returning null
WalkthroughGameObjectSerializer.GetComponentData gains an includeNonPublicSerializedFields parameter controlling field serialization visibility. Edit-mode handling uses sharedMaterial/sharedMesh/sharedMaterials to prevent material/mesh instantiation. Caching now incorporates this flag. Comprehensive test coverage validates the new behavior across multiple Unity component scenarios. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant GetComponentData
participant FieldLogic
participant EditModeCheck
participant PropertyAccess
Caller->>GetComponentData: GetComponentData(component, includeNonPublicSerializedFields)
GetComponentData->>FieldLogic: Determine shouldInclude based on flag
alt includeNonPublicSerializedFields = true
FieldLogic->>FieldLogic: Include public + SerializeField (non-public)
else includeNonPublicSerializedFields = false
FieldLogic->>FieldLogic: Include only public fields
end
GetComponentData->>EditModeCheck: Check Application.isPlaying
alt In Edit Mode
EditModeCheck->>PropertyAccess: Use sharedMaterial/sharedMesh
PropertyAccess-->>GetComponentData: Material/Mesh reference (no instantiation)
else In Play Mode
EditModeCheck->>PropertyAccess: Use material/mesh
PropertyAccess-->>GetComponentData: Material/Mesh instance
end
GetComponentData-->>Caller: Serialized component data dict
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce conditional logic in field serialization and add edit-mode branching for property access, requiring careful verification of caching coherence and material/mesh handling paths. Test additions are largely homogeneous, but the implementation change warrants thorough review of the visibility flag logic and edit-mode detection patterns. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (1)
362-389: Centralize edit‑mode material/mesh handlingLogic is correct. Consider extracting this into a small helper to avoid drift and make future extensions (e.g., other components with shared* patterns) trivial.
- object value; - if (!Application.isPlaying && (propName == "material" || propName == "materials" || propName == "mesh")) + object value; + if (!Application.isPlaying && (propName == "material" || propName == "materials" || propName == "mesh")) { - // In edit mode, use sharedMaterial/sharedMesh to avoid instantiation warnings - if ((propName == "material" || propName == "materials") && c is Renderer renderer) - { - if (propName == "material") - value = renderer.sharedMaterial; - else // materials - value = renderer.sharedMaterials; - } - else if (propName == "mesh" && c is MeshFilter meshFilter) - { - value = meshFilter.sharedMesh; - } - else - { - // Fallback to normal property access if type doesn't match - value = propInfo.GetValue(c); - } + value = GetSharedValueForEditMode(c, propInfo, propName) ?? propInfo.GetValue(c); } else { value = propInfo.GetValue(c); }Add within this class:
private static object GetSharedValueForEditMode(Component c, PropertyInfo propInfo, string propName) { if ((propName == "material" || propName == "materials") && c is Renderer r) return propName == "material" ? (object)r.sharedMaterial : r.sharedMaterials; if (propName == "mesh" && c is MeshFilter mf) return mf.sharedMesh; return null; }TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (3)
365-377: Remove unused MeshFilter from the materials testThe MeshFilter is created and assigned but never used in this test, adding noise and allocations.
- var meshRenderer = testObject.AddComponent<MeshRenderer>(); - var meshFilter = testObject.AddComponent<MeshFilter>(); + var meshRenderer = testObject.AddComponent<MeshRenderer>(); @@ - meshFilter.sharedMesh = testMesh;
454-456: Set HideFlags on test materials to avoid editor asset clutter if a test abortsMinor robustness: mark throwaway materials as HideAndDontSave to prevent leaks across domain reloads on failure.
- var testMaterial = new Material(Shader.Find("Standard")); + var testMaterial = new Material(Shader.Find("Standard")) { hideFlags = HideFlags.HideAndDontSave }; @@ - var material1 = new Material(Shader.Find("Standard")); + var material1 = new Material(Shader.Find("Standard")) { hideFlags = HideFlags.HideAndDontSave }; material1.name = "TestMaterial1"; - var material2 = new Material(Shader.Find("Standard")); + var material2 = new Material(Shader.Find("Standard")) { hideFlags = HideFlags.HideAndDontSave }; material2.name = "TestMaterial2";Also applies to: 541-547
489-491: Avoid renaming shared primitive meshesRenaming the shared primitive mesh mutates a shared asset and can affect other tests. Skip renaming or use an instantiated mesh when you need to mutate.
- testMesh.name = "TestMesh"; meshFilter.sharedMesh = testMesh;TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs (3)
30-37: Harden shader choice and mark temp assets as hiddenOn some pipelines, "Standard" may be missing. Add a small fallback and set HideFlags to avoid editor leaks.
- testMaterial = new Material(Shader.Find("Standard")); + var shader = Shader.Find("Standard") + ?? Shader.Find("Universal Render Pipeline/Lit") + ?? Shader.Find("Unlit/Color"); + Assert.IsNotNull(shader, "Could not find a fallback shader for test material."); + testMaterial = new Material(shader) { hideFlags = HideFlags.HideAndDontSave };
97-126: Mark instantiated meshes as hidden to avoid lingering objects on failureYou destroy the mesh—good. Adding HideFlags is a belt-and-suspenders improvement.
- var uniqueMesh = UnityEngine.Object.Instantiate(testMesh); + var uniqueMesh = UnityEngine.Object.Instantiate(testMesh); + uniqueMesh.hideFlags = HideFlags.HideAndDontSave;
54-91: Assert edit‑mode context explicitlyOptionally assert/assume the tests are running in edit mode to make failures clearer when misconfigured.
[Test] public void GetComponentData_UsesSharedMaterialInsteadOfMaterial() { + Assume.That(Application.isPlaying, Is.False, "This test must run in edit mode."); @@ [Test] public void GetComponentData_UsesSharedMeshInsteadOfMesh() { + Assume.That(Application.isPlaying, Is.False, "This test must run in edit mode."); @@ [Test] public void GetComponentData_EditModeDetectionWorks() { - // This test verifies that our edit mode detection is working + // This test verifies that our edit mode detection is working + Assume.That(Application.isPlaying, Is.False, "Expected edit mode.");Also applies to: 93-127, 182-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (18)
SetUp(17-22)TearDown(24-32)Test(34-41)Test(43-50)Test(52-71)Test(73-82)Test(84-111)Test(113-128)Test(130-139)Test(141-160)Test(162-175)Test(177-198)Test(200-290)Test(292-358)Test(360-404)Test(406-444)Test(446-476)Test(478-509)MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (2)
GameObjectSerializer(17-556)GetComponentData(122-429)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (2)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs (6)
Test(54-91)Test(93-126)Test(130-142)Test(144-156)Test(158-180)Test(182-203)MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (2)
GameObjectSerializer(17-556)GetComponentData(122-429)
🔇 Additional comments (1)
MCPForUnity/Editor/Helpers/GameObjectSerializer.cs (1)
258-283: <!-- [scratchpad_start]
[task verification_1/1 check_attributes_usage_3/3 assess_review_validity_complete][observations]
- Context confirms lines 258-283 are in a field caching loop within a hierarchy traversal method
- Purpose: serialize Component fields, respecting Unity's public/private and [SerializeField] rules
- Current logic: includes public fields OR non-public with [SerializeField] (when includeNonPublicSerializedFields=true)
- NO usage of [NonSerialized] or [SerializeReference] exists in the codebase currently
- Suggested fix changes inherit: true → inherit: false AND adds [SerializeReference] check
- The inherit parameter change is a subtle behavior shift worth noting
[analysis]
Review is architecturally sound but PROACTIVE:
- Respecting [NonSerialized] on public fields aligns with Unity's serialization contract
- Supporting [SerializeReference] alongside [SerializeField] is consistent
- However, these attributes don't exist in use yet, so the concern is defensive
Concerns about the diff:
- inherit: true → inherit: false changes BEHAVIOR (narrower attribute lookup)
- This could affect subclasses if they or their parents use these attributes
- The diff doesn't explain why this behavior change is necessary
[pending]
None - sufficient evidence gathered.[conclusions]
- Review suggestion is architecturally valid (aligns with Unity's rules)
- But it's proactive (no current usage found)
- The inherit parameter change needs developer verification
- Developer should confirm: (1) is this defensive design intentional? (2) is narrower inheritance lookup correct?
[actions]
- Provide tag with guidance to developer
- Note that the suggestion is sound but requires manual verification of intent
- Highlight the inherit parameter change as needing explicit review
</scratchpad_end] -->Verify architectural intent for attribute handling; note subtle inherit parameter change
The review's suggestion to respect [NonSerialized] and [SerializeReference] attributes is architecturally sound and aligns with Unity's serialization contract. However, neither attribute is currently used anywhere in the codebase, making this a proactive improvement.
One aspect requires your careful review: the suggested diff changes the inherit parameter from
truetofalsefor attribute lookups. This narrows the scope to only check attributes declared directly on each field, not inherited ones. Verify this behavior change is intentional and appropriate for your use case, especially if you expect [SerializeField] or [SerializeReference] to be inherited from base classes.
1 participant
Prevent material/mesh instantiation warnings in edit mode
Addresses #323
Summary by CodeRabbit
Bug Fixes
New Features
Tests