Bug: Caching drawers not compatible with arrays

[marc-antoine-girard]

Description

Given an array of: { raw, Object, Object, raw }. Right-clicking on element [0] and choosing "Clear" or "Properties" will clear show the property of the last element. [3]
Doing the same thing on element [1] will apply to element [2] since it is the last element of its type.

This is caused by the fact that drawers are reused and the GenericMenu events are processed late in Unity's loop

This is also true for everything related to CustomObjectDrawer. When ReferenceDrawer.OnButtonClicked is called, the Property.propertyPath is always the last element of the same type (raw or Object).

Demo:

Given

public class CodeExample : MonoBehaviour, IDog
{
    public List<SerializableInterface<IDog>> dogs;
}

[Serializable]
public class Dog : IDog
{
    public int Age;
    public string Name;
}

public interface IDog { }

[marc-antoine-girard]

@Thundernerd I've been looking for a while and, while I've been able to make the CustomObjectDrawer work with cached drawers, it is extremely tedious. Like, it requires caching / closure / queues and waiting. I'm currently trying a different approach, which would only require Queues, and a bit of waiting ( look for the <--------- and the comment )

// CustomObjectDrawer.cs

public SerializedProperty Property; // <----------
private readonly Queue<string> toClear = new Queue<string>(); // <----------

public void OnGUI(Rect position, GUIContent label, GUIContent content)
{
    ...
    HandleMouseDown(position, positionWithoutThumb);
    HandleKeyDown();
    HandleClearEvent();  // <----------
}

private void HandleMouseDown(Rect position, Rect positionWithoutThumb)
{
    ...
    else if (Event.button == 1 && positionWithoutThumb.Contains(Event.mousePosition))
    {
        ...
        menu.AddItem(new GUIContent("Clear"), false, () => toClear.Enqueue(Property.propertyPath)); // <----------
        ...
    }
}

private void HandleClearEvent() 
{
    if (toClear.Count > 0 && toClear.Peek() == Property.propertyPath) // <----------
    {
        toClear.Dequeue();
        DeletePressed?.Invoke();
    }
    else if(toClear.Count > 0) 
    {
        // This is only to illustrate the problem.
        // In an array of 10 elements of the same ReferenceMode (Raw or Unity), Clearing the last element
        // will trigger this log 9 times. The 10th time, the condition above is called.
        Debug.Log("Not the right time");
    }
}

// ReferenceDrawer.cs
protected void Initialize(SerializedProperty property, Type genericType, FieldInfo fieldInfo)
{
    CustomObjectDrawer.Property = property;  // <----------
    Property = property;
    GenericType = genericType;
    FieldInfo = fieldInfo;
}

Hopefully, this clears things up a bit. This approach seems to work well. This would need to be done for pretty much every events. This is also a draft, the final implementation should follow the same idea though.

1. Before I move forward, what's your take on this?
2. Is it worth it or should we revert back to not caching the drawers?

[Thundernerd]

Hey @marc-antoine-girard !
I was already working on a fix for this myself :D. Would you mind doing a checkout on the PR and see if you can break the solution in your (test) project?

[marc-antoine-girard]

@Thundernerd Everything seems to be working as expected😃

[Thundernerd]

Merged it!