Dear Devs,
my name is Peter I’m the developer of UMotion Pro (an animation editor for Unity).
I recently had an issue where a customer contacted me because UMotion was not working correctly when used together with another asset that had some editor scripts in it. After digging around in the source code, it turned out that the developer of that other asset had a small typing mistake in one of its editor scripts that caused the issue. He wrote this:
EditorApplication.update = MyUpdateCallback;
instead of this:
EditorApplication.update += MyUpdateCallback;
(Notice the missing +).
The compiler is not complaining here because both is valid C# code. But the above code (using the assignment operator) is removing any previously registered callback from the “EditorApplication.update” event. So any other editor script in the current Untiy project that previously registered to “EditorApplication.update” is not suddenly not called anymore.
This design was used for other editor events too so it can happen with those too.
This is a problematic design because:
A small typing mistake (that’s very hard to spot afterwards) causes all other editor scripts that are using “EditorApplication.update” to “break”.
The developer writing this faulty code probably won’t notice this at first because his own callback is registered correctly so the code he is currently testing works correctly.
This is especially problematic as soon as the code is uploaded to the asset store (or is in a unity package, on git hub etc). Due to the nature of this problem, the impact is noticed in a completely different asset/package which makes it a problem harder to find. Furthermore it can cause additional support work for asset store publishers that are only indirectly influenced by this.
I have a few suggestions how to solve this:
Make EditorApplication.update private and instead add 2 methods to the “EditorApplication” class:
public static void SubscribeOnUpdateEvent(Action callback)
{
update += callback;
}
public static void UnsubscribeOnUpdateEvent(Action callback)
{
update -= callback;
}
As Jes28 pointed out, adding the “event” keyword would be the best solution as it won’t break the API but disallows the assignment operator to be used.
As the above solution would be a breaking API change (and you probably don’t want that), you could instead use static code analysis that throws a compile error whenever the assignment (“=”) operator is used to subscribe to an (editor) event.
Use this static code analysis also in the curation process for asset store packages uploaded for older Unity versions.
Thank you very much, looking forward to hear your thoughts about this.
EditorApplication.update is delegate, not event
so making it event will solve issues like this and introduce compiler errors for calling EditorApplication.update = MyUpdateCallback; without + sign
Yes I also noticed that currently Unity is not using the event keyword (just the regular delegate mechanism). So before posting my original post I tried to see if using an event would solve the issue. Maybe I did something wrong but the following test script compiled correctly and showed the same behavior as the “pure delegate” approach currently used by Unity:
using System;
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEditor;
public class TestScript : MonoBehaviour
{
public static event Action OnUpdate;
[MenuItem("Window/Test")]
public static void Test()
{
OnUpdate += Func1;
OnUpdate = Func2;
OnUpdate(); // Call the event
}
private static void Func1()
{
Debug.Log("Func1 called!");
}
private static void Func2()
{
Debug.Log("Func2 called!");
}
}
Thanks for the detailed write up of this problem and for explaining your use case @SoxwareInteractive .
The behaviour that you’re describing is known and the team is pushing for events instead of simple fields (typed as a delegate) whenever possible in new APIs. Unfortunately, changing from field to event is a breaking change and would require additional work on the ApiUpdater side, so this isn’t likely to happen in the foreseeable future.
If you or anyone else has more use cases in which changing from field → event would be important, please add them to this thread as that could help to speed things up.
But is adding the “event” keyword really introducing a breaking change? If all this keyword does is that it prevents the assignment operator from being used, then the assumption that this is a breaking change would only be true if using the assignment operator is a valid use-case scenario. In my opinion, the only time someone uses the assignment operator in his code to register a callback is due to a typing mistake, not because the user really opted for the “lets clear all previously registered callbacks and break every package, asset, user code and engine code (?) that previously subscribed the event” - behavior.
Having the API updater to change the assignment operator to “+=”, would be nice to have but is it really a must? I mean, the API updater is not responsible to fix typing mistakes users made. The compiler error the user gets presented also provides clear instructions on how to fix the issue (simply by adding a “+”).
I understand that you are careful to avoid API breaking stuff, I absolutely second that strategy (especially as an asset store developer who relies on consistent API across multiple Unity versions). But is this case really a “breaking change” in the classical meaning? I mean what valid use case is breaking with this change?
There are at least a couple of scenarios that changing from simple delegate to event is a breaking change (This example show them: https://bit.ly/3oC1T6X)):
Assemblies (when you use the += operator on an event, the compiler will emit a call to add_EventName where as for delegates it will call Delegate.Combine().
Users may be calling Delegate.BeginInvoke(), Delegate.Combine(), etc, or relying on other delegate behavior/features
Also we cannot say for sure that the use of = instead of += is a mistake; it may very well be intentional; we could argue that allowing such scenario may lead to bugs (as you found) (and I agree) but changing it now is a breaking change.
That being said, both breaking changes are a no go, but breaking assemblies (1) is really bad since users may not even be able to recompile them and, if not addressed, it will cause a runtime exceptions.
As I comment above there’s no way we can distinguish the intentional use from the mistakes so we cannot deem the assignment as a mistake.
Also, even if we introduce support in ApiUpdater this change will change behavior whenever the assignment operator was used (we would add the handler to the event instead of overwriting the list).
That being said, if we encounter many more cases in which update delegate is being misused we may consider changing it.
Given that this update delegate is shared among every editor script that could possibly exist in a project, I can’t imagine a scenario where using = would lead to anything but bugs. Even if someone is using it “intentionally”, they’re definitely not using it correctly, and it’s okay to force them to change that IMO.
Perhaps the correct answer here is to deprecate EditorApplication.update and make a new, event-based version?
Changing behavior that projects depends on (intentionally or not) is something we should avoid (it is impossible to know how many projects depends on this behavior)
My point was that simply changing from field → event is a breaking change
Anyway, Indeed, the long term solution is to obsolete update and introduce a new event
that being said:
I am not sure if there is any requirement for update to be a field
Initially, usages of update would be marked as a warning and later as an error.
Probably we would need to trigger both so there’s a risk code registering for both and getting called twice (maybe it is possible to detect this and emit a warning)
@Vagaus : Thank you very much for sharing all this detailed insights into the decision process regarding this situation.
Maybe introduce this change as a preview (in one of the next alpha releases) plus communication on the alpha forum and/or a blog post? That way people can give it a try and if they really have use-cases where a field would be a requirement, they can let you know (and as it’s still in preview/alpha you are able to revert to a different approach).
I would say that’s the users fault. I mean, as in that scenario there would be already a warning mentioning that his code is using the deprecated update field, so it should be clear to the user that he is still using the deprecated callback mechanism too.
My point was not about users requirements, but instead, a Unity requirement.
Unfortunately being a user fault (or not) does not alleviate the issue.
Users would get a warning when compiling code assessing the field; what about assemblies? Maybe we can check., at runtime, and detect that the same method will be invoked by both the delegate and the event
Anyway, I am not even sure this would be a big problem or not.
