Fixed object reference duplicate in arrays #19 #20
Conversation
This fixes the issue Thundernerd#19
removed unnecessary change
Since it only applies to SerializableInterfaces in Raw mode, it made more sense to only do it in RawReferenceDrawer instead of SerializableInterfacePropertyDrawer. Tested the same way, no apparent diff.
Would only work in UNITY_2021_1_OR_NEWER otherwise
Thundernerd
left a comment
Thundernerd
left a comment
There was a problem hiding this comment.
Thank you for the PR! I've added some questions and some formatting change requests, but can't blame you for that since I haven't specified the formatting anywhere :)
| activeDrawer = null; | ||
| serializedProperty = property; | ||
| genericType = GetGenericArgument(); | ||
|
|
There was a problem hiding this comment.
Could you explain the reason for this added blank line?
There was a problem hiding this comment.
It seems my Rider thought it looked better :) I'll remove it!
| private static object previousReferenceValue; | ||
| private static string previousPropertyPath; |
There was a problem hiding this comment.
Could you explain the reason for making this static?
There was a problem hiding this comment.
It used to be a local variable in SerializableInterfacePropertyDrawer.cs. The goal was to store the last element and check if the new element's rawReference object points to the same instance as the previous one. If it does AND this isn't actually the same variable (we know that using the propertyPath, see line 115 in RawReferenceDrawer.cs), then we need to create a new instance and reassign the values (line 124 - 125 in RawReferenceDrawer.cs). I've moved the code to RawReferenceDrawer.cs since it made more sense because it only applies to rawReferences, but since we always create a new instance for that drawer, we either need to pass the values of the previous object to its constructor, or save it in a static variable.
Passing it using the constructor would also add unnecessary code in SerializableInterfacePropertyDrawer.cs. So the static way made sense in that regard. I hope it clears up things a bit. Also, it wouldn't need to be static if the instance of ReferenceDrawers where reused :)
Ah I hadn't thought about that. Might be interesting to take a look at! I'll add that to the list |
|
I'm doing the changes and update the PR, thanks for your comments. This also is my first contribution on Github, so I'm still learning! :) |
|
@Thundernerd Also, in Edit: Implemented in #27 |
|
🎉 This PR is included in version 1.7.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
2 participants
Description
As described by the issue, when a new instance of SerializableInterface is added in an array using the inspector's gizmo, Unity clones the last SerializableInterface. Since rawReference is [SerializeReference], the reference of the object is copied by ref, which makes the new element references the same instance as the previous one.
Editing the last element will modify the previous one too.
This bug should only applies to pure serializable C# classes stored in the rawReference's field. This is why I moved the fix from SerializableInterfacePropertyDrawer to RawReferenceDrawer in my previous commits.
This fixes #19 without adding any dependencies and in an easy-to read (ish) fashion.
Type of change
Checklist:
Unrelated Note: Unless I'm missing something, the ReferenceDrawer instantiated in GetReferenceDrawer(..) could be reused for every instances, the same way Unity reuses the PropertyDrawer for multiple instances to avoid GC.