Open D4KU opened 2 years ago
Thank you for this PR! After my current to-do list is finished (which will take some time, unfortunately), I'll take a good look at your commits, I'm already looking forward to it 😄
I didn't review all files yet but from what I saw, there'll be considerable amount of changes needed before I merge this PR. The major points are:
obj is X x
or out X x
) can't be used#if UNITY_X_OR_NEWER #endif
directivestestField
below correctly? Are changes to that field in the RuntimeInspector correctly applied to the TestClass?public class TestClass : MonoBehaviour
{
[System.Serializable]
public struct TestStruct
{
public int testField;
}
[System.Serializable]
public struct TestStruct2
{
public TestStruct testStruct;
}
public TestStruct2 testStruct2;
}
After these changes, I may still ask for some medium-scale changes (though I don't think they would be as significant as these), so I'd understand it if you're unwilling to do these changes. Thank you for your effort regardless!
Don't worry, I was expecting this to take some iterations, I'm still willing to follow through with this PR. I already started working on your remarks and will update you soon. :)
Alright, the new commits:
However, there is problem in regards to Unity 5.6 I wanted to discuss.
I chose IEnumerable
(and now IReadOnlyList
) to ensure that code wanting to change the BoundValues
property (formerly Value
) of an InspectorField
can only set the whole list instead of adding, removing, or changing entries. The getter of BoundValues
is public, so exposing ways to manipulate its entries without the owning InspectorField
receiving any callback would be a design flaw in my opinion.
The problem: in Unity 5.6, with .NET 3.5 as only option, ReadOnlyCollection<T>
is the only index-able, read-only list class or interface available. There is no non-generic equivalent. Generic Variance, which my system relies on, only works on interfaces. The only way I see to make it work with 5.6 then is to rip out my generics again and use ReadOnlyCollection<object>
everywhere, thus reintroducing all the ugly casting and loosing the ability the communicate that the list is only meant the store apples; not fruits, cars, or anything else.
Having said that, I would prefer to drop support for .NET 3.5. I like that the plugin is that much backwards compatible, but here I think the benefits for the code base of ditching 5.6 outweigh the benefits of keeping it. What do you think?
I'm sorry for replying late. Thank you for your new commits 😺 I was actually ready to convert the prefabs to 5.6 myself, sorry that I didn't mention it in my previous post.
I didn't check out all changes yet but could you share where you're using the non-generic equivalent of ReadOnlyCollection<T>
?
Oh sorry you must have misunderstood something, to my knowledge there is no non-generic ReadOnlyCollection
. I'm using IReadOnlyList<T>
now where I used IEnumerable<T>
before. For example, as type for InspectorField<T>.BoundValues
.
I'm trying to understand which part of the code is currently not Unity 5.6 compatible.
While typing my response I actually figured out a way to make it work in 5.6 again. :)
I'm aware of my late response times. Stuff has been happening and I wasn't in the mood to review a large PR for quite some time. There'll still be a delay before I review this PR but know that this is one of the few things in my small to-do list.
No stress, take your time. I'm not dependent on the changes being upstream.
I worked with my multi-edit system for several weeks now and found a few improvements to my code that I just included in this PR, in case you're wondering where the latest commits come from.
I extended multi-selection support over to the inspector.
InspectorField
'sValue
property is now an IEnumerable and got renamed toBoundValues
, meaning that one drawer can now bind to multiple values.Since some breaking changes were unavoidable, I used the opportunity to introduce some static type safety, which avoids many casts in sub-classes and made the public API clearer. Some concepts of multi-editing can get hard to communicate when everything is an object. The way it works is that
InspectorField
got a type parameter, which specifies the type with which bound values are stored. It still has a non-generic base class, which can't access theBoundValues
. For this, I introduced two interfaces:IBound
andISupportsType
. See their declaration for more information.