Feedback A Critique of API Design for Editor code, and hopefully a discussion

I've been working with Unity for years, in projects of various sizes. Work on editor extensions has always been a struggle. The life cycle of most things is not defined and/or rigid, and discovering what you need, in what order or when it needs to be called is 80% of the work.
Using Reflection, or even Harmony to patch Unity live, has almost become the norm when working with Editor Tooling.

Edit: reworded below to better communicate that I don't think the issue is in a single developer, but the overall approach to things and/or review process.

I'm writing this post after having worked with the EditorTool API and again being frustrated with the design choices made in its API.

This isn't a remark on the author of that API. I suspect that the choices he made were due to technical limitations or having the requirement of staying in-style. However, I want to address the problem that even in 2020, Unity is using a style and design of coding for a platform that is presumably intended to be extendable - however is anything but. I want to discuss what needs to be done so we can avoid having to use this type of design.

I hope that this post sparks somewhat of a discussion on Unity's API for the editor, or, at the very least those parts that are extendable.

As an example of this issue, I want to point out the relatively new Editor Tool API.
In this post, a Unity employee is outlining a pretty fundamental usecase: a Tool detecting whether it is activated or disabled. This functionality is hidden behind a global event handler and a somewhat unrelated external state check. How to implement this pattern is not mentioned in the official docs, only in a post on the forum.

- I don't understand why listening to the lifecycle is not API that can exist on the EditorTool itself. I.e. a method to override given that we already inherit from a base class.
- If it is not, why is the event handler not providing context on where it is switching from and to. This design would be both self-documenting (instead of the programming needing to lookup the active tool through an unrelated other variable) and functional (not dependent on outside state).
- Where can I find the documentation on what will be called, when?

Note that my issue isn't solved by updating the documentation.
My problem is with the approach to code design. The "simplicity" is taken to such an extreme that it actually hurts implementation time because developers need to spend time figuring out how to do basic stuff, /requiring/ good documentation - which isn't provided or fragmented over unrelated sources and code.

My point is that with good API design this understanding would flow naturally from the API - instead of documentation.

IMHO, the editor API is far too often C# implemented in a C++ "script" style (i.e. I can't even call it proper C++ style because there are excellent examples of extendable UI libraries out there in modern C++ (e.g.: Qt)).

Performance issues aren't the argument here either. Calling the method on the base class EditorTool for context swaps that happen at most (!) once per frame are not going to matter, even if virtual. Providing context in the form of two references to instances, isn't either.

Not to mention that this leads to everyone reinventing the wheel in slightly different ways. (Compare the source code per example of Bolt and OdinInspector)

And now for the truly tragic part: Initially it looked like the API design WAS going to be "better" C# design style. I.e. `EditorTool` would have `OnActivate()` and `OnDeactivate()`, per this post.

I would be very interested to learn why this decision was made, @kaarrrllll

I feel that Unity at this point has matured/aged enough that we should start holding new API to a higher standard. I can understand why old API isn't upgraded, but we should at least strive to uphold new API to a better standard. And if that API can't be written as such, that it's made clear that the underlying issues need addressing.

Related post:
- https://forum.unity.com/threads/getting-the-current-custom-editortool-instance-globally.694975/

 

Thank you very much for taking the time to reply!

So, if I understand correctly, in this situation the EditorTool lifecycle methods would be tied to the EditorTools / EditorToolsContext, done in managed side. I assume that the issue is that you can't control the order of execution on the OnDisable on the `EditorTool` instances prior to the `EditorTools`/`EditorToolsContext` - and thus end up in a situation where OnDeactivate gets called after OnDisable.

If so, I don't see how the solution outlined in this post won't lead to similar issues. Or better said, I assume that because in that example the EditorTool is handling its own de-registration, it simply won't get the `Deactivate` call at all.

* Edit:
I guess the problem is whether the `OnDisable()` should be tied to the `Deactivate()`. Given that the tool currently is a ScriptableObject, that lifecycle is mostly tied to domain reloads AFAIK. And in those cases I would probably assume that if the tool was active prior to domain reload, it would remain active after - if it does not, our current solution also doesn't fix things and we get into a very awkward situation where its behavior is nondeterministic (randomly could occur that the Tools Disable() happens before the Manager's (?)).

The problem here being that we delve into this whole deal of ScriptableObject's lifecycle.
So with that in mind, would it then not be better to have the EditorTool manager simply not call Activate/Deactivate on its Disable? (Which I think it is the current behaviour and/or it discards the current active tool IIRC)

Yeah, that was the approach I used internally initially.

Code (CSharp):

1. public class LifecycledEditorTool : EditorTool {
2.  
3.         protected virtual void OnEnable() {
4.             EditorTools.activeToolChanging += OnActiveToolWillChange;
5.             EditorTools.activeToolChanged += OnActiveToolDidChange;
6.         }
7.  
8.         protected virtual void OnDisable() {
9.             EditorTools.activeToolChanging -= OnActiveToolWillChange;
10.             EditorTools.activeToolChanged -= OnActiveToolDidChange;
11.         }
12.    
13.         void OnActiveToolWillChange() {
14.             if (EditorTools.IsActiveTool(this)) {
15.                 try {
16.                     OnDeactivate();
17.                 } catch (Exception e) {
18.                     // Done so exceptions during transition don't break the editor.
19.                     Debug.LogException(e);
20.                 }
21.             }
22.         }
23.  
24.         void OnActiveToolDidChange() {
25.             if (EditorTools.IsActiveTool(this)) {
26.                 try{
27.                     OnActivate();
28.                 } catch (Exception e) {                  
29.                     // Done so exceptions during transition don't break the editor.
30.                     Debug.LogException(e);
31.                 }
32.             }
33.         }
34.    
35.         protected virtual void OnDeactivate(){}
36.    
37.         protected virtual void OnActivate(){}
38.  
39.     }

What could we as a community do to help communicate the necessity of looking into things like this?
Is there anything we can do, or is it better to simply trust the direction that Unity is curently heading in?

 

Would maybe an alternative have been to separate out the context of the EditorTool (the serializable bit), and lift the rest of the EditorTool fully into managed? (I.e.: avoiding Unity.Objects lifecycle entirely and having it done by the Editor Tools Manager.)