Replace System.Object with UnityEngine.Object in Editor.Button.Draw

[ribbanya]

The type of Editor.targets is UnityEngine.Object[], not System.Object[]. Attempting to implicitly convert Object[] to object[] causes Rider to complain (and it's not great practice anyway).

The alternative to this PR is what I'm currently doing in my code, which I'd rather not:

public override void OnInspectorGUI()
{
	DrawDefaultInspector();
	var objects = Array.ConvertAll(targets, item => (object) item);
	_buttonsDrawer.DrawButtons(objects);
}

[madsbangh]

Nice find! Really awesome PR <3
I am pretty sure UnityEngine.Object was the actual intended type, so good thing you caught it :)

I will check this out when I get home.

Also pinging @SolidAlloy just to be sure this is indeed correct.

[SolidAlloy]

I agree the only objects that can be passed at the moment are of type UnityEngine.Object. However, I intended it for future development where someone adds the ability to use the Button attribute in custom serializable classes ( #16 ). In case a custom serializable class has a button-marked method, the type of target will need to be System.Object, not UnityEngine one.

The problem with casting arrays of UnityEngine.Object can be resolved by accepting IEnumerable<System.Object> instead of System.Object[]. I'm not sure why I didn't do it from the start. I see that the only thing we do with the array is iterating over it, so IEnumerable<System.Object> should suffice. @ribbanya , will such a fix be ok for you?

[ribbanya]

If that's the case, I would suggest there be a public-facing overload that's friendly with Editor (accepts UnityEngine.Object[]).

Either that, or make Button generic and make the default implementation a Button<UnityEngine.Object>. That way people could write their own Button<MyClass> later.

---

If we go with generics, I think it's not hard to keep the existing ButtonAttribute as-is and simply have Button search the assemblies for an appropriate subclass and instantiate it using MakeGenericType.

[ribbanya]

Thinking about it some more, I'm okay with the IEnumerable<System.Object> implementation, I just don't like using object when I can help it.

If you feel the generic implementation is overly complex, your solution does solve my use case.

[ribbanya]

@madsbangh I made #22 so that you can pick which implementation you want to go with.

Just merge the one you want and close the one you don't. If there are any other necessary changes I'll be happy to make them in either branch.

[SolidAlloy]

Specifying a type like UnityEngine.Object will not help reflection because the MethodInfo.Invoke() method is not generic and only accepts System.Object, so every instance will be converted to System.Object anyway. Or do I overlook something here?

I'm not against UnityEngine.Object. We can change the type now, and when/if someone eventually comes to implement the [Button] attribute on custom serializable classes, they will think of a more suitable solution, be it IEnumerable<object> or a generic Button class.

Still, I like IEnumerable<System.Object> more as it can accept a collection of any type without requiring you to cast anything explicitly and will not be slower than a generic Button class because Reflection will do the casting anyway.

The final word is on @madsbangh , of course.

[ribbanya]

Specifying a type like UnityEngine.Object will not help reflection because the MethodInfo.Invoke() method is not generic and only accepts System.Object, so every instance will be converted to System.Object anyway.

You would need to create a generic type with MakeGenericType and then invoke the method on the generic type. It would be type-checked at runtime and the implementing code would be checked at compile time.

My thinking was along the lines of how JsonConverter works, in that it appears generic to the end user but the internal implementation doesn't actually care that it's generic.

This is just how I would write it, if I were planning on making it extensible. I think you have the right idea when you say:

when/if someone eventually comes to implement the [Button] attribute on custom serializable classes, they will think of a more suitable solution

[ribbanya]

I'm actually going to go ahead and close this because I think #22 provides a cleaner solution in the short-term and is more extensible in the long-term.

[madsbangh]

Thank you @ribbanya and @SolidAlloy :)

I think this is the right choice. I like to keep things simple and add complexity when needed, so yeah!
Nice discussion too, and good points.

#22 keeps the code as simple as it is now, while increasing usability 👍

Awesome ❤️