Added ScriptType listen and unlisten API for easier Event subscription handling

[yaustar]

Fixes #4910

Test project with the debug version of the engine already included. It has buttons to change scene (destroying entities) and to disable/enable entities with scripts that are listening for mouse and touch events.

https://playcanvas.com/project/1078046/overview/listen-api-test-project

PR includes automated tests as well

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

[yaustar]

This a slow function but I don't expect this to be called very often/at all and the number of subscribed events is generally expected to be quite low generally.

Didn't seem to be worth the extra complexity to make it faster at this point.

[kungfooman]

@typedef is too good to be allowed to use, you need to remove it and mess around with any instead. 😉

[yaustar]

I did wonder if I should use a typedef or a new class definition 🤔

[kungfooman]

Proper way by example:

engine/src/framework/anim/state-graph/anim-state-graph.js

Line 17 in 6d4766e

* @param {object} data - The JSON data to create the AnimStateGraph from.

(while removing neat IntelliSense)

[kungfooman]

Would be nice to see this merged, saves a ton of time/nerves handling events 👍

Also required to rather easily fix Seemore scene change issue as described in #5409 (comment)

Good work as always 💯

[yaustar]

Anyone got time to review this one for 1.65? :)

[Maksims]

Just a comment related to this issue: I've made a small override of on and once and another method of event handler, to return object with off method, and in a large projects with long legacy we have not encountered any issue.

And it made it a breeze to manage events that way.

[kungfooman]

Just a comment related to this issue: I've made a small override of on and once and another method of event handler, to return object with off method

Interesting, can you share full example? Is it only an empty object otherwise? If so, what do you think about just returning a function which - when called - removes the event?

Just a little bit like React useEffect with a different life-cycle/context:

  useEffect(() => {
    function handleStatusChange(status) {
      setIsOnline(status.isOnline);
    }
    ChatAPI.subscribeToFriendStatus(props.friend.id, handleStatusChange);
    // Specify how to clean up after this effect:
    return function cleanup() {
      ChatAPI.unsubscribeFromFriendStatus(props.friend.id, handleStatusChange);
    };
  });

[Maksims]

Example of usage:

let evt = entity.on('trigger', () => { });

// sometime after
evt.off();

[willeastcott]

@Maksims That is what the Editor does in various places. As an example:

const events = [];

events.push(editor.on('someevent', ...));
events.push(editor.on('otherevent', ...));

events.push(panel.on('clear', () => {
    for (const event of events) {
        event.unbind();
    }
    events.length = 0;
});

[Maksims]

@Maksims That is what the Editor does in various places. As an example:

const events = [];

events.push(editor.on('someevent', ...));
events.push(editor.on('otherevent', ...));

events.push(panel.on('clear', () => {
    for (const event of events) {
        event.unbind();
    }
    events.length = 0;
});

Indeed.
As I've designed Editor's events API based on exactly this pain of managing events. So within Editor's context - it proved to be a great and flexible approach.

[yaustar]

Just a comment related to this issue: I've made a small override of on and once and another method of event handler, to return object with off method, and in a large projects with long legacy we have not encountered any issue.

With that in mind, do we want to go ahead with this PR or explore other possibilities?

If so, what is being purposed here? Going with @Maksims comment on returning an object with an off method requires the user to still handle this manually in a destroy/disable/enable scenario.

I wanted to avoid this as that's what people tend to forget/not know how to handle.

Example: https://forum.playcanvas.com/t/event-not-working-after-changescene/28944

[kungfooman]

With that in mind, do we want to go ahead with this PR or explore other possibilities?

Personally I want this merged and not explore 100 possibilities which eventually just get stuck in a rut. I already use it and then I have to patch every PR using this if I want to load for example Seemore to test bug fixes against non-trivial scenes.

You already made sure that this is extensible in future, so nothing is lost - and if someone feels like extending upon this, that can be better done in individual PR's.

[Maksims]

Managing events - is somewhat advance topic. It is required in more complex scenarios, as ~80% of events don't need to be managed.

PlayCanvas does not break existing public APIs, so adding an API - should be very considered.
Also, it is important to avoid "multi-path" in API design - to prevent branching during learning. If it is possible to achieve same thing different ways - it is harder to learn such bloated APIs.

Unfortunately this particular issue - has no easy solution.
Proposed API stands out to me personally as a utility library, rather than a first-class API of PlayCanvas way of doing things.

It also does not handle a case where event is already unbound (using off), but will be still in a list unless it is unbound only using unlisten method. This leads to GC being unable to free things that are reachable from the event function scope in some cases.

[kungfooman]

It also does not handle a case where event is already unbound (using off), but will be still in a list unless it is unbound only using unlisten method. This leads to GC being unable to free things that are reachable from the event function scope in some cases.

Practically the unlisten method isn't necessary because the point is to call ScriptType#listen once and then being able to forget about it. Because pretty much everyone forgets (or has forgotten) to off their on's.

When the entity (or only the script component) is deleted, the GC gets rid of every ScriptType instance including of course ScriptType#_listeners.

[Maksims]

It also does not handle a case where event is already unbound (using off), but will be still in a list unless it is unbound only using unlisten method. This leads to GC being unable to free things that are reachable from the event function scope in some cases.

Practically the unlisten method isn't necessary because the point is to call ScriptType#listen once and then being able to forget about it. Because pretty much everyone forgets (or has forgotten) to off their on's.

When the entity (or only the script component) is deleted, the GC gets rid of every ScriptType instance including of course ScriptType#_listeners.

This is only one of the cases for cleaning events.
There are many cases where re-binding events is necessary during lifetime of script component. E.g. bind on enable and unbind on disable. If it would be created using listen method and then unbound using off, it will lead to stalled references that will not be GC'ed, and developer will not know about it without knowing the details of the implementation of API.

So either it should handle remove of events that were created using listen or the alternative API design would be preferred.

[yaustar]

This API also unbinds the underlying event during disable and enable.

You are right in its a utility function where the developer can write one line to listen for events that automatically unbinds on disable and destroy and rebind on enable.

Throughout the years I've done support and moderated the forums, the amount of boilerplate code needed to manage events and number of topics created around destroying entities and changing scenes because of dangling events is high.

It's been complained about enough by developers from studios and beginners that it warrants a solution like this where its one call, managed and part of the core API. Not an extra script that is added.

[Maksims]

This API also unbinds the underlying event during disable and enable.

What if it is not desired (some events should be called during disabled state)? But unbind on destroy is desired?
This covers very specific use case. And does not cover few different scenarios, this will lead to still a need to develop own solutions when this does not cover.

Sorry for being critical here, I just believe there is a simpler building block API can be designed that is more flexible and less narrow, while providing a tools to cover different behaviours and needs of a developer.

[yaustar]

Then they can use the 'raw' events that currently exist. If there is a solution that doesn't require the developer to have manually listen for the destroy/enable/disable events, then I be happy to hear it but haven't seen any suggestions that cover that use case.

While this is a specific use case, it's a very common one. Arguably the most common use case.

[Maksims]

So, an combine API of returning events handle with listen utility, with handling case of removing event handler from listen list - probably would be a better solution here.
Also, provide an optional mechanic to handle enable/disable when using listen.
Such way developers would use common way to create events, as well as have a utility that does not have hidden behaviours that can bite.

Something like:

this.linkEvent(app.on('event', () => { }));

We are linking an event to this script instance.
Also, we can link at any point, and unlink using the same event handle.
So it covers the main purpose of this API. Without introducing new events API (alternative way of subscribing).

[yaustar]

If we go this route, we would have to be okay breaking API for the on/off functions on the EventHandler as they currently set up for chaining

I assume you are thinking along the lines of what is done with observer and add EventHandle for the engine https://github.com./playcanvas/playcanvas-observer/blob/main/src/event-handle.js#L1

[kungfooman]

Lets say I have a script like this, two on's and two listen's:

HoloVideoPlayer.prototype.initialize = function () {
    this.on("attr:url", this.reload, this);
    this.on('destroy', this.cleanUp, this);
    this.listen(this.app.mouse, "mousemove", this.mousemove, this);
    this.listen(this.app.mouse, "mousedown", this.mousedown, this);
    // allocate all kinds of things down the line...
};

I work to ensure that all of the operations done by initialize are undone by cleanUp. Pretty much allocating resources, textures, mesh instances, meshes, entities, events on self and others:

HoloVideoPlayer.prototype.reload = function () {
    this.cleanUp();
    this.initialize();
}

Now something becomes obvious:

HoloVideoPlayer.prototype.cleanUp = function () {
    this.off();
    this.unlisten(this.app.mouse, "mousemove", this.mousemove, this);
    this.unlisten(this.app.mouse, "mousedown", this.mousedown, this);
    // delete other stuff...
}

EventHandler#off is way easier to work with than ScriptType#unlisten. If we had 20 of each, we would still have only one this.off(), but 20 unlisten's.

Considering that we want to make it a quick/easy interface, maybe we need to improve unlisten?

[Maksims]

Here is an example of how I believe such API could work:

Script.prototype.initialize = function () {
    this.bindEvents();
};

// handling swap becomes easier
Script.prototype.swap = function (old) {
    // old script events on script instance are removed, so no need to remove them
    // old script linked events are unsubscribed automatically too
    this.bindEvents();
};

Script.prototype.bindEvents = function() {
    this.on('attr:property', this.onProperty);

    // should trigger only when this script is enabled, and be ignored when this script is disabled
    this.linkEvent(this.app.mouse.on('mousedown', this.onMouseDown, this), true);

    // should trigger while this script is either enabled or disabled
    // also, if `otherEntity` gets destroyed, it will lead to `off` on that event,
    // which should automatically be removed from this script linked events
    this.linkEvent(otherEntity.on('someEvent', () => { }));

    // alternative way of writing:
    let evt = anotherEntity.on('someEvent', () => { });
    this.linkEvent(evt);
};

The API would be:

// these are changes to existing API
EventHandler.on(); // returns event handle
EventHandler.once(); // returns event handle

// new APIs
class EventHandle // handle of the event
EventHandle.off(); // unbind an event
EventHandle.silenced // if true, event will not be triggered

ScriptType.linkEvent(eventHandle, [silenceWhenDisable]); // method to link event to script instance life. Optionally can be silenced when script instance is disabled. This method returns eventHandle.
ScriptType.unlinkEvents(); // unbind (off) all linked events, this method is ran automatically when script instance is destroyed

// EventHandler.off() - should ensure linked event handles on script instances to be removed

Such API and implementation, will eliminate hanging references of linked event handler that were off'ed elsewhere, also will handle cases where source of the event removes such event (e.g. entity that we subscribe to is destroyed). Additionally it allows developers to have any complexity for event handle's life. And it does not introduces alternative way of making events, which means integrating this approach to existing code, is as easy as adding this.linkEvent( in front of existing lines of code.

The only problem with such approach, is that it requires a breaking change - EventHandler to return EventHandle instance instead of scope of which it was called. Which I have not ever seen to be used by other developers. So to handle such transition, we can create a global project setting, that will define the behaviour, and by default for all existing projects or if not defined will default to old way. For new projects will be enabled by default. For existing projects it will be possible to switch in settings.

Potential breaking impact of such change on existing project is extremely tiny. As nor the engine uses "chaining" (which is not chaining) in its code base, nor I personally seen any other project used it. And if project setting path is used, no breaking impact will be.

[yaustar]

Do we have an example elsewhere in the engine where creation of an object is 'linked' in a similar way that we need to be considerate of pattern wise?

[Maksims]

Do we have an example elsewhere in the engine where creation of an object is 'linked' in a similar way that we need to be considerate of pattern wise?

I believe this pattern would be new to the engine API design.

[kungfooman]

Lets say we really need to get rid of the mouse (could be anything really, any entity):

function destroyMouse() {
    const { mouse } = pc.app;
    if (mouse) {
        // For lack of Mouse#destroy:
        mouse.fire("destroy");
        mouse.disableContextMenu();
        mouse.detach();
        mouse.off();
    }
    delete pc.app.mouse;
}
destroyMouse();

Right now we would be stuck with it in ScriptType#_listeners:

[kungfooman]

Suggested change

	this._listeners.push({
	eventHandler.on("destroy", () => {
	this.unlisten(eventHandler, name, callback, scope);
	});
	this._listeners.push({

[yaustar]

That would have to assume that all objects have a destroy event which is not guaranteed

[kungfooman]

That would have to assume that all objects have a destroy event which is not guaranteed

Good point! Should we try to remove object references or accept that these objects will remain active in memory then?

As with everything, we have multiple possibilities:

1. Make sure to actually add destroy methods on obvious objects (good style anyway?)
2. Save some kind of GUID reference to objects, which isn't the actual object (would require GUID lookups etc.)
3. Simply accept we reference destroyed objects and trust the garbage collector to eventually free up the memory (as it is right now)
   4...) ...any more ideas?

I guess option (3) would work fine in most cases, but this should be a conscious choice and not an oversight decision.

[Maksims]

Check out WeakMap and WeakSet.

[kungfooman]

@Maksims Interesting suggestion, maybe I miss something, but we cannot iterate WeakMap and WeakSet by design, so how would we iterate over all event handlers to enable/disable them when needed?

[Maksims]

Thinking on it more, without at least periodical iteration through listened event handles, or an event on event handler - would be hard to implement self removals.

[kungfooman]

Right, so you basically think that we require EventHandler#destroy aswell, with at least this.fire("destroy");, which Mouse#destroy could reuse via super.destroy (and dozens of other classes)?

[kungfooman]

I inspected the idea of adding a EventHandler#destroy method a bit, which would fire a this.fire('destroy') and three classes which extend EventHandler already have it:

engine/src/platform/sound/manager.js

Lines 117 to 125 in c55ab20

	destroy() {
	this.fire('destroy');
	if (this._context) {
	this._removeUnlockListeners();
	this._context?.close();
	this._context = null;
	}
	}

engine/src/platform/graphics/graphics-device.js

Lines 359 to 369 in c55ab20

	destroy() {
	// fire the destroy event.
	// textures and other device resources may destroy themselves in response.
	this.fire('destroy');
	this.quadVertexBuffer?.destroy();
	this.quadVertexBuffer = null;
	this.dynamicBuffers?.destroy();
	this.dynamicBuffers = null;
	}

This one feels odd, kinda semantically wrong - remove implies out of hierarchy, but still "working"? Destroying implies you shouldn't touch any properties any longer, because the object is... well... destroyed:

engine/src/framework/xr/xr-plane.js

Lines 94 to 96 in c55ab20

	destroy() {
	this.fire('remove');
	}

At the same time, EventHandler.destroy could always call this.off()? A quick look over the code base looks like many classes do that already, so we could even reduce the amount of code, pretty much removing duplicate code here and there 🤔

[Maksims]

XrPlane is "remove", because it is not destroyed, as its data is still available to the developer. But it is removed from tracking by the underlying system.

[yaustar]

Closing PR as it's not clear of the direction that this should go in. Can alway reopen later down the line.

[kungfooman]

Closing PR as it's not clear of the direction that this should go in. Can alway reopen later down the line.

I think we can just forget about the references in _listeners, it's not even a bug and rather simple to debug using Chrome's Heap Snapshot tool... and to put it bluntly, no one else cares about that, the engine is full of examples.

I saw for example exactly one case trying to get rid of the this.app reference:

engine/src/framework/script/script-registry.js

Lines 24 to 27 in 6f82ce8

	destroy() {
	this.app = null;
	this.off();
	}

No other class would ever consider that.

But forgetting to actually remove event listeners is causing bugs.

When I implemented this myself, I implemented some other stuff and came across this event off'ing bug aswell:

engine/src/framework/components/camera/system.js

Lines 33 to 47 in 6f82ce8

	constructor(app) {
	super(app);
	this.id = 'camera';
	this.ComponentType = CameraComponent;
	this.DataType = CameraComponentData;
	this.schema = _schema;
	this.on('beforeremove', this.onBeforeRemove, this);
	this.app.on('prerender', this.onAppPrerender, this);
	this.app.systems.on('update', this.onUpdate, this);
	}

engine/src/framework/components/camera/system.js

Lines 170 to 174 in 6f82ce8

	destroy() {
	super.destroy();
	this.app.systems.off('update', this.onUpdate, this);
	}

Two little code snippets of the same file, anyone wants to guess the bug here?

And this example brings me to another point: instead of in ScriptType, we should add this in EventHandler, because "events are easy to forget for everyone".

EventHandler#unlisten:

    /**
     * Removes a listener for an event on an EventHandler that was added by {@link ScriptType#listen}.
     *
     * @param {EventHandler} [eventHandler] - EventHandler that was originally listened to.
     * @param {string} [name] - Name of the event that was originally listened to.
     * @param {HandleEventCallback} [callback] - Function that was used as the callback when the event was originally
     * listened to.
     * @param {object} [scope] - Object that was used as the scope when the event was originally listened to
     * @example
     * this.unlisten(this.app.mouse, 'mousemove', this.onMouseMove, this);
     * @see {@link ScriptType#listen} to listen to events on an EventHandler.
     */
    unlisten(eventHandler, name, callback, scope) {
        for (let i = 0; i < this._listeners.length; ++i) {
          const l = this._listeners[i];
          if (
            (!eventHandler || l.eventHandler === eventHandler) &&
            (!name         || l.name         === name        ) &&
            (!callback     || l.callback     === callback    ) &&
            (!scope        || l.scope        === scope       )
          ) {
            this._listeners.splice(i, 1);
            l.eventHandler.off(l.name, l.callback, l.scope);
          }
        }
    }

With this change, we can simply call this.unlisten() to remove all remote events, which is the equivalent for the common this.off() for local events.

Anyway, that's my current direction I'm thinking in, would be happy to see more discussion around this... IMO this is rather important to have and I couldn't find another idea as appealing as listen/unlisten for non-this event handling.

[kungfooman]

I think I found a legit example for listenEvenWhenDisabled:

engine/src/scene/shader-lib/program-library.js

Lines 33 to 56 in e5c6903

	constructor(device, standardMaterial) {
	this._device = device;
	this._generators = {};
	this._isClearingCache = false;
	this._precached = false;
	// Unique non-cached programs collection to dump and update game shaders cache
	this._programsCollection = [];
	this._defaultStdMatOption = new StandardMaterialOptions();
	this._defaultStdMatOptionMin = new StandardMaterialOptions();
	standardMaterial.shaderOptBuilder.updateRef(
	this._defaultStdMatOption, {}, standardMaterial, null, [], SHADER_FORWARD, null);
	standardMaterial.shaderOptBuilder.updateMinRef(
	this._defaultStdMatOptionMin, {}, standardMaterial, null, [], SHADER_SHADOW, null);
	device.on('destroy:shader', (shader) => {
	this.removeFromCache(shader);
	});
	}
	destroy() {
	this.clearCache();
	}

Not directly applicable here (since it's not a script), but when it comes to resource handling, we woud want to destroy shaders or other resources.

In the script I also don't see a off call, might be an event leak?

And as usual, we don't remove device from the instance, "leaking" just as much as listen/unlisten would do - so I don't see a strong argument against it.

[Maksims]

Made a PR to push "chaining" away, that will lead to better APIs and events management in the future.
#5481