ExtendRealityLtd / Malimbe

[Obsolete - No longer maintained] A collection of tools to simplify writing public API components for the Unity software.
MIT License
51 stars 11 forks source link

VRTK removes custom editor/property drawers #53

Open fight4dream opened 5 years ago

fight4dream commented 5 years ago

Environment

Steps to reproduce

I am using this inspector https://github.com/SubjectNerd-Unity/ReorderableInspector I uncommented ReorderableArrayInspector.cs line 24 to turn all arrays into reorderable lists line 27 to make all ScriptableObject fields editable

then after I install VRTK, those inspectors are no longer available

Expected behavior

the inspectors from SubjectNerd-Unity/ReorderableInspector should be available

Current behavior

those inspectors from SubjectNerd-Unity/ReorderableInspector are not available

bddckr commented 5 years ago

I'm not aware of a solution. Both projects have to use Unity API (CustomEditor) in a hacky way to pull off each other's "replace the default inspector for anything" logic.

fight4dream commented 5 years ago

Is there a replacement already available in VRTK? I am particularly interested in Reorderable list, context menu button. I can forego ScriptableObject editor

bddckr commented 5 years ago

This is done in Malimbe here, which is used as part of VRTK. Malimbe does it to pull of reactive change handling for any property change (in this case for in-inspector changes). Without this, changes in the inspector to any VRTK/Zinnia component won't be noticed - components are no longer reactive to changes done in the inspector.

fight4dream commented 5 years ago

I just looked into malimbe and i think you had a possible solution to this https://github.com/ExtendRealityLtd/Malimbe/issues/38

bddckr commented 5 years ago

Ha well that would do it of course as it just gets rid of our "injected" drawer 😉 Big project, though. Perhaps there's a way to tell Unity to prefer one CustomEditor over the other, even though both specify to work with UnityEngine.Object.

I suggest to set isFallback to false here. That will probably open up some non-deterministic ordering inside Unity, so perhaps setting the script execution order so their inspector gets picked before ours works. Though I'm pretty sure that would still mean you lose Malimbe's change handling ability - ultimately what we all would need is some API from Unity to do a generic property drawer interception, not just a full-blown inspector panel replacement.

fight4dream commented 5 years ago

Thanks for the time to look into this. One final question then this issue could be closed, would PropertyDrawer works for the HandlesMemberChangeAttribute?

bddckr commented 5 years ago

Sadly it won't - that API does not allow replacing any type. The only alternative I was seeing in the past was to implement a wrapping type like ObservedChange<T> and force users (and us) to utilize that one instead.

While we can solve the inspector drawing that in a weird way (that's what a PropertyDrawer can handle after all), we can't solve two big issues with that approach:

  1. Users have to do some weird ObservedChange<T>.Value to get the T out of it and use the type to set it. That is dumb API and increases friction for any user. implicit operator can help here in C#, but you can't define that in the generic base class to convert from/to that generic T instead you'd be forced to copy n paste that conversion method into each (non-generic) subclass (see the next point on why you need those in the first place).
  2. Unity's serialization doesn't support generic types (Dictionary, List and T[] are supported because they specifically added support only for those). This means you have to create a subclass to "bind" the generic type parameter. You can see that often in our codebases in Zinnia and VRTK, most of the time when we create UnityEvent<T> subclasses that need to be used as serialized fields. I have plans to solve that automatically through Malimbe, too: https://github.com/ExtendRealityLtd/Malimbe/issues/2 but I didn't find the time to get around to that yet either.
bddckr commented 5 years ago

I believe the original issue can be resolved by making Malimbe's InspectorEditor look up other editors that use CustomEditor(typeof(Object)) and then manually tell those to draw, as part of Malimbe's own inspector drawing.

This would allow plugging in any additional (i.e. it's additive, I'm not interested in offering some conditional drawing just yet) drawing.

fight4dream commented 5 years ago

Great! Is it also possible to add a separator or even a foldable box for the additional custom parts?

Ps. I find that i can derive an editor for specific component to show that particular custom editor.

bddckr commented 5 years ago

Is it also possible to add a separator or even a foldable box for the additional custom parts?

Yes. This has always been the case. Custom property drawers are being used, we only have issues with other Editors that utilize [CustomEditor(typeof(UnityEngine.Object), true)].

I find that i can derive an editor for specific component to show that particular custom editor.

That is also expected and is default Unity behavior - it matches by type, and obviously UnityEngine.Object is the root type, of which all others inherit from. Again the issue is only with other components that utilize the same hack to draw any Object.

extendreality commented 4 years ago

Apparently Odin has done something to make Odin play nice with VRTK, would be good to find out exactly what that was to see if we could apply something to play nicely with other custom editors.