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

Unity Editor
1

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:

2

First, I’d like to say that I absolutely agree with you that there should be EditorTool.OnActivated and EditorTool.OnDeactivated. I still would like to see it added, but there is a particularly sticky problem that prevents it.

The issue is with the order of callbacks. Usually, for tools you’d allocate your resources in OnEnable and release them with OnDisable. Now consider that OnActivated and OnDeactivated will likely be used to access these resources.

So for example you might write something like this:

class SomeEditorTool : EditorTool
{
    Camera m_Camera;

    void OnEnable()
    {
        m_Camera = new GameObject().AddComponent<Camera>();
        m_Camera.gameObject.hideFlags = HideFlags.HideAndDontSave;
    }

    void OnDisable()
    {
        Object.DestroyImmediate(m_Camera);
    }

    protected override void OnActivated()
    {
        m_Camera.enabled = true;
    }

    protected override void OnDeactivated()
    {
        m_Camera.enabled = false;
    }
}

Looks good, but there’s a problem. Due to the way managed resources are unloaded, we can’t guarantee that OnDeactivated will be called before OnDisable. It’s nearly certain that you will end up in a situation where you’re trying to access destroyed resources. Yes, feasibly we could document the behavior and leave it up to the user to be extra careful when writing their code, but I think that that is a worse situation than what we have currently.

Okay, so just the modify code to invoke OnDeactivated before any OnDisable callbacks, right? Well, without going into too much detail, it’s not quite so simple. The tool manager is written in managed code, whereas the lifecycle callbacks that we’d need access to in order to implement this are all in unmanaged code. Now because tools are at their core just UnityEngine.Object, they go through the same lifecycle as runtime objects. Adding a new event to the lifecycle of every C# object just for tools doesn’t seem like a good idea (I’m also not sure I could get that past the review process), so that’s out.

What about making OnEnable and OnDisable virtual then? Then you can throw a warning if users don’t call base.OnDisable and ensure that at least there’s some indication of why things aren’t working properly. That’s not a bad idea, and had I thought of that when I was writing this API I probably would have done so :slight_smile: Unfortunately changing those “magic” function calls to virtual at this point breaks API, and we don’t have an automatic updater for these kinds of changes. If it weren’t for the fact that these are “magic” functions, I think we’d still go this route. However, because making this change would break compatibility with no “warning” period, and these are such critical functions, that’s also out.

To make a long story short, I’d really like to have these functions. However it’s frustratingly difficult to do properly, and I am not comfortable shipping a feature that can’t be trusted to work in a consistent manner. I’m still looking for solutions, hopefully I will find something tenable soon.

3

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.

public class LifecycledEditorTool : EditorTool {

        protected virtual void OnEnable() {
            EditorTools.activeToolChanging += OnActiveToolWillChange;
            EditorTools.activeToolChanged += OnActiveToolDidChange;
        }

        protected virtual void OnDisable() {
            EditorTools.activeToolChanging -= OnActiveToolWillChange;
            EditorTools.activeToolChanged -= OnActiveToolDidChange;
        }
    
        void OnActiveToolWillChange() {
            if (EditorTools.IsActiveTool(this)) {
                try {
                    OnDeactivate();
                } catch (Exception e) {
                    // Done so exceptions during transition don't break the editor.
                    Debug.LogException(e);
                }
            }
        }

        void OnActiveToolDidChange() {
            if (EditorTools.IsActiveTool(this)) {
                try{
                    OnActivate();
                } catch (Exception e) {                  
                    // Done so exceptions during transition don't break the editor.
                    Debug.LogException(e);
                }
            }
        }
    
        protected virtual void OnDeactivate(){}
    
        protected virtual void OnActivate(){}

    }

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?

4

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.)

5

That’s a fair point, you’re correct that it is susceptible to the same issue. The difference is that the event doesn’t imply any bearing on the object’s lifecycle. It’s not ideal, but the logic was that it is better to provide something over nothing and continue with work on implementing OnActivated/Deactivated without blocking release.

You’re correct, current behavior is to only call the changing events when the active tool is changed through the property, not during domain reloads. This saw a lot of discussion, and ultimately I think we got it wrong. By not invoking the tool changed events (or ideally, what should have been the OnActivated / OnWillBeDeactivated virtual functions) on domain reloads we make resource management more opaque, in my opinion. In fact, I have a PR in flight that changes this behavior to fix a separate issue related to custom editor tools.

Thoughtful critiques like your post are an excellent way to raise issues. Generally speaking, user requests and bug reports have a considerable amount of weight with regards to allocating developer time. It’s much easier for me to justify spending time on something that has a clear need outlined by the community.