EventHandle and option to disable EventHandler chaining.

[Maksims]

This PR introduces EventHandle for easier events management.

Problem:

Event management with limited lifetime of objects - can be cumbersome, especially as there is no easy way to detach events unless you have all the references to arguments that this event was created with.
Also, anonymous functions as a callback on event that needs to be removed after - is a "no no!" with a current implementation.
So when you need to subscribe to an event in a script, and manage that subscription, you need to add very similar lines to 3 places: initialize, destroy and swap. And when you have many events, it looks like copy-pasted code all around with small modifications.

Another major problem is that current EventHandler implementation returns this when creating an event using on and once, which historically was called "chaining", but it is not chaining as it does not offer any logic of chaining events, their order of execution and rules of execution.

So implementing EventHandle requires breaking change, which PlayCanvas avoids. As chaining has not been seen used in practice, this PR implements it in a breaking manner, to mitigate issues if someone (extremely unlikely) used chaining, this implementation makes a on and once method for backwards compatibility. So this code will still work:

app.on('evtOne', () => {
    // one
}).on('evtTwo', () => {
    // two
});

Notes:

Engine does not have a single use of "chaining" in its codebase or examples.
I've tested this PR on dozens of existing projects (old and new, small and big), and have not found a single use of "chaining".
This also been discussed on forums and on github: #5341 #4910
During tests, engine uses somewhere references for event handle (old object) after it has been removed.

Solution:

Proposed implementation changes the behavior of on and once on EventHandler, by returning EventHandle. Which developer can use to detach event by simply calling handle.off().

New APIs:

// pc.EventHandler
let evt = obj.on('event', () => { }); // returns EventHandle

// pc.EventHandle
evt.removed; // true if this event has been removed
evt.off(); // function to simply remove an event

Examples:

ScriptType remove event when script is destroyed or swapped:

Script.prototype.initialize = function () {
    this.evt = this.app.on('event', this._onEvent, this);

    this.once('destroy', () => {
        this.evt.off();
    });
};

Script.prototype.swap = function(old) {
    old.evt.off();
    this.evt = this.app.on('event', this._onEvent, this);
};

Script.prototype._onEvent = function() {
    // event
};

ScriptType with many events:

Script.prototype.initialize = function () {
    this.bindEvents();
    this.once('destroy', this.detachEvents); // make sure we detach events on destroy
};

// bind all events
Script.prototype.bindEvents = function() {
    this.detachEvents();
    if (!this.events) this.events = [];
    this.events.push(this.app.on('eventA', this._onEvent, this));
    this.events.push(this.app.on('eventB', this._onEvent, this));
    this.events.push(this.app.on('eventC', this._onEvent, this));
    this.events.push(this.app.on('eventD', this._onEvent, this));
};

// detach all linked events
Script.prototype.detachEvents = function() {
    if (!this.events?.length) return;
    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events.length = 0;
};

Script.prototype.swap = function(old) {
    old.detachEvents(); // detach events from old script instance
    this.bindEvents();
};

Script.prototype._onEvent = function() {
    // event
};

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[kungfooman]

            const handles = this._callbacks[name];
            if (handles) {
                for (let i = 0; i < handles.length; i++) {
                    handles[i].destroy();
                }
                this._callbacks[name] = [];
            }

Shouldn't this._callbacks[name] = []; be just handler.length = 0;? Is there any kind of reference comparison magic somewhere else, which demands this?

Why do we produce Array GC?

Here too:

    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events = [];

[kungfooman]

Shouldn't a destroyed handle off itself at least?

[Maksims]

destroy is a private method of EventHandle, and cleans its data, and does not modify data outside of its class.

[kungfooman]

Every other destroy method we have cleans up its events in other objects, usually in the form of:

    destroy() {
        // remove any outstanding listeners
        this._registry.off("load", this._onLoad);
        this._registry.off("error", this._onError);
        // ...
    }

Why is this a special case?

It also doesn't really do anything at all right now, is it a draft?

[Maksims]

Every other destroy method we have cleans up its events in other objects, usually in the form of:

    destroy() {
        // remove any outstanding listeners
        this._registry.off("load", this._onLoad);
        this._registry.off("error", this._onError);
        // ...
    }

Why is this a special case?

It also doesn't really do anything at all right now, is it a draft?

Events are "detached" rather than "destroyed". This PR does not change that and follows an existing pattern.
I've added clear references to destroy, like this.callback = null and other things, but tests did not like that. As the current implementation does not clear such references from event objects, their properties are accessed after during tests. So such change would have the potential to break.

[Maksims]

            const handles = this._callbacks[name];
            if (handles) {
                for (let i = 0; i < handles.length; i++) {
                    handles[i].destroy();
                }
                this._callbacks[name] = [];
            }

Shouldn't this._callbacks[name] = []; be just handler.length = 0;? Is there any kind of reference comparison magic somewhere else, which demands this?

Why do we produce Array GC?

Here too:

    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events = [];

Focus of this PR is to with minimal logic changes implement EventHandle. If there is an optimisation of EventHandler methods - good option for another PR.

[kungfooman]

Focus of this PR is to with minimal logic changes implement EventHandle. If there is an optimisation of EventHandler methods - good option for another PR.

Good point, what about the code that specifically you added?

    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events = [];

 * @example
 * // store an array of event handles
 * let events = [ ];
 *
 * events.push(objA.on('testA', () => { }));
 * events.push(objB.on('testB', () => { }));
 *
 * // when needed, remove all events
 * events.forEach((evt) => {
 *     evt.off();
 * });
 * events = [ ];

Usually we handle it like this in the engine:

    destroy() {
        const meshInstances = this.meshInstances;
        for (let i = 0; i < meshInstances.length; i++) {
            meshInstances[i].destroy();
        }
        this.meshInstances.length = 0;
    }

Because why create extra GC for operations that don't require it? We use temporary variables everywhere aswell to not call Vec3#clone for example, so the examples could reflect that, because other devs will take it as... example.

[Maksims]

Focus of this PR is to with minimal logic changes implement EventHandle. If there is an optimisation of EventHandler methods - good option for another PR.

Good point, what about the code that specifically you added?

    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events = [];

What would be an alternative way of unsubscribing from a list of events?

 * @example
 * // store an array of event handles
 * let events = [ ];
 *
 * events.push(objA.on('testA', () => { }));
 * events.push(objB.on('testB', () => { }));
 *
 * // when needed, remove all events
 * events.forEach((evt) => {
 *     evt.off();
 * });
 * events = [ ];

Usually we handle it like this in the engine:

    destroy() {
        const meshInstances = this.meshInstances;
        for (let i = 0; i < meshInstances.length; i++) {
            meshInstances[i].destroy();
        }
        this.meshInstances.length = 0;
    }

Because why create extra GC for operations that don't require it? We use temporary variables everywhere aswell to not call Vec3#clone for example, so the examples could reflect that, because other devs will take it as... example.

I've updated the example to use .length = 0. Good point.

Events are "detached" and long-lived objects. The usual pattern of working with them is to have them for a long time, and sometimes remove them. Associated memory with each event is small. So re-using event handles is not considered here. Vectors and similar math objects can be created on each frame, so this can lead to a significant GC stall, and we do encourage re-using them, as they can be re-used. EventHandle - cannot be re-used, like many other things within the engine.

[Maksims]

Updated this PR with a recent EventHandler refactor (#5501).

[kungfooman]

Do we really need all this extra getter code which kinda does nothing? Why not simple and short:

    /**
     * True if event has been removed.
     * @type {boolean}
     * @readonly
     */
    removed = false;

If I get your intention right, you just want to have it readable and not writable?

[kungfooman]

Sorry, I just realized we still lack proper types to express this idiom: microsoft/TypeScript#37487

My favorite would be this, which is a pattern all over the engine:

~~

    destroy() {
        this.handler?.off(this.name, this.callback, this.scope);
        this.handler = null;
    }

~~

So we don't introduce removed/_removed at all.

Edit: outdated / do-not-use / etc.

[kungfooman]

Events are "detached" and long-lived objects. The usual pattern of working with them is to have them for a long time, and sometimes remove them. Associated memory with each event is small. So re-using event handles is not considered here.

Right, so why give them an off method? It's not like we will on them again? off'ing them is destroy'ing them, so destroy is all that should be required.

My main motivation to get rid of the off method is to simplify the API here. I dislike to have different return values based on some setting like pc.EventHandler.chaining = false; if it's not 100% necessary.

I think we could simply return always and only the EventHandle, BUT the EventHandle has extra deprecation methods to generate warning's for on/off/once method's.

So for the rare case that someone wrote something like:

this._registry
  .off("load", this._onLoad)
  .off("error", this._onError);

They can continue using that code until we decide it had enough deprecation time.

[Maksims]

Right, so why give them an off method? It's not like we will on them again? off'ing them is destroy'ing them, so destroy is all that should be required.

That is the current naming convention when working with events. We "off" them.
Happy to hear if PlayCanvas developers are happy to have two different names for the same logical operation.

const evt = app.on('event', onEvent);

app.off('event', onEvent);
evt.off(); // alternative

// or

app.off('event', onEvent);
evt.destroy(); // alternative

I dislike to have different return values based on some setting like pc.EventHandler.chaining = false; if it's not 100% necessary.

I absolutely agree here. But, the engine has a promise to the users that it does not break public APIs, and ensures backwards compatibility. Even with this case, where I personally never seen chaining used in dozens of projects I've worked on.

I think we could simply return always and only the EventHandle, BUT the EventHandle has extra deprecation methods to generate warning's for on/off/once method's.

It will be impossible to have the same behavior, as currently the returned value from EventHandler.on - is the EventHandler itself. So the user can access any properties/methods of EventHandler (and objects that inherit it, e.g. Entity).

Also, off still returns EventHandler, so provided example:

this._registry
  .off("load", this._onLoad)
  .off("error", this._onError);

Will work with this PR.

[kungfooman]

I absolutely agree here. But, the engine has a promise to the users that it does not break public APIs, and ensures backwards compatibility. Even with this case, where I personally never seen chaining used in dozens of projects I've worked on.

Okay, lets be real here: breaking things is an established procedure, that's why we note it in releases:

The Plane#constructor change alone affected me and at least two others (who happened to speak up in the PR). Who knows how many others.

While this PR is basically obfuscating API by trying to bend over backwards for something that affects... no one?

In my personal fork I ripped the Event chaining out quite some time ago, never experienced any problems in projects I play around with.

[Maksims]

@willeastcott, @slimbuck, @mvaligursky what you guys think, should we go with breaking change by returning EventHandle from on and once by default? With very-very little chance that someone wrote with chaining. So we provide a global flag that they can use to switch to the old-way?

That would be definitely amazing, it just goes against a rule of "never break a public API".

I went conservative with this PR, but am happy to go with breaking (unlikely affecting anyone) change.

[willeastcott]

To be honest, @Maksims, I never knew you could chain these calls and I never have in all the years I've been working with PlayCanvas. I don't recall ever seeing anyone else do that in their code (which doesn't mean they don't do it!).

In this particular instance, where I believe very, very few projects will be affected, I'd rather go with a clean API and a mini-breakage. 😄 What do you think @mvaligursky?

[mvaligursky]

@Maksims - I agree with @willeastcott , that's a main reason to break existing API. I'd say go for it as well, considering this will benefit many users with some inconvenience to almost none.
Extra points are given if there's a way to have Debug.deprecated message to warn the user something no longer works. Or at least I prefer to have an exception in code to happen if we cannot detect it - pointing the user to the change, instead of silently hiding it behind a different behaviour.

[kungfooman]

EventHandle could have on/off/once/fire methods which simply point out their deprecation and do nothing else.

So we provide a global flag that they can use to switch to the old-way?

This still needs input, I would rather completely drop static chaining = true; which just makes the types crazy (return values based on variables... which no one uses).

I don't want to write code like !(ret instanceof pc.EventHandler) because pc.EventHandle is not exported, just to fix types for a case that I want 100%.

[Maksims]

I've updated PR with returning EventHandle by default.
Also (@mvaligursky) added on and once methods to EventHandle, so it is technically backwards compatible with such workflow, except it will warn with Debug.deprecated.

[willeastcott]

Should we hide the constructor from the docs? Presumably end users would never construct the EventHandle themselves and just use instances returned from on and once?

[willeastcott]

Suggested change

	Debug.deprecated('Using chaning with EventHandler.once is deprecated, subscribe to an event from EventHandler directly instead.');
	Debug.deprecated('Using chaining with EventHandler.once is deprecated, subscribe to an event from EventHandler directly instead.');

[willeastcott]

Suggested change

	* @returns {EventHandle} - can be used for removing event in the future.
	* @returns {EventHandle} Can be used for removing event in the future.

[willeastcott]

OK, nice job here. I'm approving with a few minor fixes and suggestions. But I do kinda wish there were some unit tests to go along with this PR. 😉

[kungfooman]

I'm also very happy about the outcome, I would just like to point out the last thing that I'm still not happy with: EventHandle#destroy

I have a function that I'm using in dozens of places:

/**
 * Call each element's destroy method and empty array afterwards.
 * @param {{destroy: Function}[]} arr 
 */
export function destroyArray(arr) {
    const n = arr.length;
    for (let i = 0; i < n; i++) {
        arr[i].destroy();
    }
    arr.length = 0;
}

When I call that on an array, I expect that no resource is leaked any longer. But current EventHandle#destroy is leaking the event, because Event#destroy is only marking the EventHandle as removed. Marking something as removed is not destroying it, it's simple as that.

You can check every destroy method we have in the engine: https://github.com./search?q=repo%3Aplaycanvas%2Fengine%20destroy()%20%7B&type=code

They all clean up their events in their destroy method and I would like to keep that intuition.

Instead of something like this:

// detach all linked events
Script.prototype.detachEvents = function() {
    if (!this.events?.length) return;
    for(let i = 0; i < this.events.length; i++) {
        this.events[i].off();
    }
    this.events.length = 0;
};

I just write everywhere:

// detach all linked events
Script.prototype.detachEvents = function() {
    if (!this.events?.length) return;
    destroyArray(this.events);
};

Collapsing 4 lines into 1...

So my last change proposal is:

And the four lines of callbacks[i].destroy(); turn into callbacks[i]._removed = true;

In C++ there is a concept of "friend classes", but in JS it's simply @internal (public to "friend classes").

    /**
     * True if event has been removed.
     * @type {boolean}
     * @internal
     */
    _removed = false;

And then you can drop the getter:

    /**
     * True if event has been removed.
     * @type {boolean}
     */
    get removed() {
        return this._removed;
    }

Last but not least:

These changes don't break anything, the unit tests work etc, it's a semantic fix to ensure that destroying means destroying and not marking something as removed, while leaking its resources.

[Maksims]

I have a function that I'm using in dozens of places:

I've updated that function for you:

export function destroyArray(arr) {
    const n = arr.length;
    for (let i = 0; i < n; i++) {
        if (arr[i] instanceof pc.EventHandle) {
            arr[i].off();
        } else {
            arr[i].destroy();
        }
    }
    arr.length = 0;
}

The initial intent was to clean up references within the destroy method, but currently, the engine has places where such references are used after the callback has been removed.
Updated this PR and removed destroy method altogether.

[kungfooman]

@Maksims Thank you a lot, it looks very good!

Can we just please have a simple destroy method? 😅

This is a common pattern:

engine/src/framework/asset/asset-localized.js

Lines 217 to 221 in 864b4bd

	destroy() {
	this.defaultAsset = null;
	this._app.i18n.off('set:locale', this._onSetLocale, this);
	this.off();
	}

We could apply that here aswell:

    destroy() {
        this.off();
    }

Yes, I could update destroyArray, but the point is that it shouldn't know any internals - it shall just work following a simple idiom. And the type {destroy: Function}[] is also wrong now.

Anyway, if you add that or not, I'm very happy about the state 👍 Good job! 💯

[willeastcott]

I just synched with @Maksims and he says he's happy to merge this in its current state. So merging... 😄 Such a great contribution. Thank you all!

[jbromberg]

Just a heads up. This PR breaks the current playcanvas tween add on when you tween like I have in the code snippet below since the event handlers no longer return this but they return the new EventHandle.

    const params = {
      opacity: 0.5,
      scale: 0,
      yPosition: 0,
    };

    textEntity.entity.setPosition(position);
    textEntity.entity.setLocalScale(0, 0, 0);

    textEntity.entity
      .tween(params)
      .to({ opacity: 1, scale: 1.25 }, (0.1 / 0.24) * duration, CubicOut)
      .chain(
        textEntity.entity
          .tween(params)
          .to({ scale: 1 }, (0.06 / 0.24) * duration, CubicOut)
          .on('update', () => {
            textEntity.entity.setLocalScale(params.scale, params.scale, params.scale);
          }),
        textEntity.entity
          .tween(params)
          .to({ yPosition: 0.5 }, (0.02 / 0.24) * duration)
          .on('update', () => textEntity.entity.setPosition(
            position.x,
            position.y + params.yPosition,
            position.z,
          )),
        textEntity.entity
          .tween(params)
          .to({ yPosition: 0.75, opacity: 0 }, (0.06 / 0.24) * duration)
          .on('update', () => {
            textEntity.entity.element.opacity = params.opacity;
            outlineColor.a = params.opacity;
            textEntity.entity.element.outlineColor = outlineColor;
            textEntity.entity.element.shadowColor = outlineColor;
            textEntity.entity.setPosition(
              position.x,
              position.y + params.yPosition,
              position.z,
            );
          })
          .on('complete', () => {
            textEntity.active = false;
            textEntity.entity.enabled = false;
          }),
      )
      .on('update', () => {
        textEntity.entity.element.opacity = params.opacity;
        outlineColor.a = params.opacity;
        textEntity.entity.element.outlineColor = outlineColor;
        textEntity.entity.element.shadowColor = outlineColor;
        textEntity.entity.setLocalScale(params.scale, params.scale, params.scale);
      })
      .start();

[mvaligursky]

@Maksims - any thoughts on the comment above?

[kungfooman]

I wanted to look into this all day too, but working on something else... quick ad-hoc solution should be a prototype patch and return this again

[Maksims]

@Maksims - any thoughts on the comment above?

Small patch of tween library chain method to use scope of EventHandle - will do the trick.

[kungfooman]

We are also mixing "illegal" ES5 patterns with ES6 in playcanvas-tween:

https://github.com./playcanvas/playcanvas-tween/blob/3e0eb3b40b5d1155efa1a27d2e1d33590f9cabe3/tween.js#L1

We are not coming around the fact that we have to have some breakage in old ways of doing things, because it's simply not possible any longer - ES6 is here since 2015 and we didn't catch up too much in PC libraries (tween / spine lib).

The upside is that we can get a somewhat simpler typing experience too by following standard ES6 ways (compared to namespace merging in e.g. playcanvas-tween). E.g. instead of pc.SineOut we could introduce a default pcTween.SineOut for ES5 users?

And ES6 users could use whatever, like import { SineOut } from 'playcanvas-tween';

If we already need to update two things, we can just overlap it so developers only need to update once and are "good to go" again.

[Maksims]

With a second glance at a tween library recommended ways of doing things, and it heavily relies on chaining. This is the case where not forcing breaking change, but having it behind the flag would help.

Updating the tween library with its own methods like .update(callback) and .complete(callback) - will help, but will require a migration by its users.

We are also mixing "illegal" ES5 patterns with ES6 in playcanvas-tween:

This is outside of this PR's scope, please open a separate issue or propose a PR in a related repo.

UPDATE:
I've created a PR to accommodate this breaking change: playcanvas/playcanvas-tween#42