API to make registering events in scriptType easier

[yaustar]

A common issue with developers new to PlayCanvas is ensuring that event listeners to objects like the mouse are cleaned up properly when the scriptType is destroyed (eg destroying the Entity or changing scenes)

Users either don't know and/or assume it's done automatically. And to do it correctly requires some boilerplate. eg:

// initialize code called once per entity
BoxPlacement.prototype.initialize = function () {
    if (this.app.touch) {
        this.app.touch.on('touchstart', this.onTouchStart, this);
    } 
    
    this.app.mouse.on('mousedown', this.onMouseDown, this);

    this.on('destroy', function () {
        if (this.app.touch) {
            this.app.touch.off('touchstart', this.onTouchStart, this);
        } 

        this.app.mouse.off('mousedown', this.onMouseDown, this);       
    }, this);
};

Could we introduce some API to make this easier/automatic? Such as a function that automatically removes the listener when the scriptType is destroyed? It would turn the above code into:

BoxPlacement.prototype.initialize = function () {
    if (this.app.touch) {
        this.listen(this.app.touch, 'touchstart', this.onTouchStart, this);
    } 
    
    this.listen(this.app.mouse, 'mousedown', this.onMouseDown, this);
};

And would also work with anonymous functions too

[Maksims]

If event methods such as on/once would return event handler instead of self, it would be easier to manage events also. Similar to the way it is implemented in the Editor.

[yaustar]

So it would be more like?

    this.mouseEventHandler = this.app.mouse.on('mousedown', this.onMouseDown, this);

    this.on('destroy', function () {
        this.mouseEventHandler.off();
    }, this);

[querielo]

Maybe, something like this:

this.handlers.mouseDown = this.app.mouse.on('mousedown', this.onMouseDown, this);

And execute the next code on the side of the engine while destroying a component instance:

for (const handlerName of Object.keys(this.handlers)) {
  const handler = this.handlers[handlerName];
  if (typeof handler.off === "function") {
    handler.off();
  }
}

So, a user don't destroy anything manually and just do the first line (this.handlers.mouseDown = this.app.mouse.on).

[kungfooman]

To be even more pedantic, a "good" script should look at least somewhat like this:

class DebugGamepadFlyCamera extends pc.ScriptType {
    // ...
    initialize() {
        this.hookEvents();
        this.on('destroy', this.onDestroy   , this);
        this.on("enable" , this.hookEvents  , this);
        this.on("disable", this.unhookEvents, this);
    }
    onDestroy() {
        this.unhookEvents();
        // whatever else needs to be done...
        this.app.timeScale = this._defaultTimeScale;
    }
    hookEvents() {
        console.log("hookEvents");
        this.app.mouse.on('mousemove', this._onMouseMove, this);
        this.app.on('framerender', this._update, this);
    }
    unhookEvents() {
        console.log("unhookEvents");
        this.app.off('framerender', this._update, this);
        this.app.mouse.off('mousemove', this._onMouseMove, this);
    }
    // ...
}

A script has no business reacting to mouse/keyboard/touch as long it is disabled.

The shipped OrbitCamera also reacts to input when it's disabled, so I guess it's fair to say that everyone is forgetting these things 😅

engine/scripts/camera/orbit-camera.js

Lines 382 to 398 in 4c16f9a

	this.app.mouse.on(pc.EVENT_MOUSEDOWN, this.onMouseDown, this);
	this.app.mouse.on(pc.EVENT_MOUSEUP, this.onMouseUp, this);
	this.app.mouse.on(pc.EVENT_MOUSEMOVE, this.onMouseMove, this);
	this.app.mouse.on(pc.EVENT_MOUSEWHEEL, this.onMouseWheel, this);
	// Listen to when the mouse travels out of the window
	window.addEventListener('mouseout', onMouseOut, false);
	// Remove the listeners so if this entity is destroyed
	this.on('destroy', function () {
	this.app.mouse.off(pc.EVENT_MOUSEDOWN, this.onMouseDown, this);
	this.app.mouse.off(pc.EVENT_MOUSEUP, this.onMouseUp, this);
	this.app.mouse.off(pc.EVENT_MOUSEMOVE, this.onMouseMove, this);
	this.app.mouse.off(pc.EVENT_MOUSEWHEEL, this.onMouseWheel, this);
	window.removeEventListener('mouseout', onMouseOut, false);
	});

But yes, I think you are on the right track with your suggestion (just that it should also automatically handle enabling/disabling)

[yaustar]

I guess the problem here is flexibility as there will be cases that even disabled, some users will want to listen to events 🤔

The way I've been doing it more recently so far is:

// initialize code called once per entity
UiManager.prototype.initialize = function () {
    this.setEvents('on');

    this.on('destroy', function () {
        this.setEvents('off');
    });
};

UiManager.prototype.setEvents = function (offOn) {
    this.app.graphicsDevice[offOn]('resizecanvas', this.onResize, this);
};

And setEvents can also be called when the script is enabled/disabled events too

[querielo]

It looks like emitting destroy doesn't remove all listeners (see EventHandler). We almost always have some callbacks that are connected to destroyed component/EventEmmiter. Even here

this.on('destroy', function handleDestroy() {
    this.setEvents('off');
}, this);

we have handleDestroy connected to the "destroy" event (until garbage collection). It is required to use once or turn it off manually. Or I miss something.

this.once('destroy', function handleDestroy() {
    this.setEvents('off');
}, this)

// OR

this.on('destroy', function handleDestroy() {
    this.setEvents('off');

    this.off('destroy', handleDestroy, this);
}, this);

My point is that it's easy to forget to turn some handlers off.

Maybe add removing handlers into EventEmmiter? Since we move callback ownership to EventEmmiter when we call on/once

class EventHandler {
    // . . .
    constructor() {
        // . . .
        
        this.handlers = { };
        
        this.once("destroy", handleDestroy() {
            this._callbacks = {}
            this._callbackActive = {}
        
            for (const handlerName of Object.keys(this.handlers)) {
                const handler = this.handlers[handlerName];
                if (typeof handler.off === "function") {
                    handler.off();
                }
            }

        }, this);
    }
    // . . .
}

So, a user just need to assign a new handler to this.handles[handlerName]

this.handlers.mouseDown = this.app.mouse.on('mousedown', this.onMouseDown, this);
// OR
this.addHandler(this.app.mouse.on('mousedown', this.onMouseDown, this));

[LeXXik]

As a note, I would vote for disabled scripts to not be listening for events. If there is an action that needs to be called on event, that action should be on an active script. A disabled script should have no business with the running app. Use cases, when a developer enables a disabled entity (via its script listening to it) is a bad pattern. It often introduces hard to debug / unexpected bugs, rather than gives any particular benefit. Just like disabled models should not affect the rendering pipeline, a disabled script should not affect the application.

[kungfooman]

As a note, I would argue to allow disabled scripts be listening for events.

@LeXXik I guess you mean s/allow/forbid? Otherwise the rest doesn't make sense to me.

We are discussing overcomplicating the "easy" interface for the sake of a few users who may want to listen to events while disabled.

IMO until some user comes up with a convincing/compelling use-case, there shouldn't be a method like listenEvenWhenDisabled.

And if some use-case is really so compelling, the user can still just use this.on('destroy', ...) with the setEvents that @yaustar posted, it would be no more than a one-liner:

this.on('destroy', () => this.setEvents('off'));`

[yaustar]

@querielo

we have handleDestroy connected to the "destroy" event (until garbage collection). It is required to use once or turn it off manually. Or I miss something.

this.once('destroy', function handleDestroy() {
    this.setEvents('off');
}, this)

// OR

this.on('destroy', function handleDestroy() {
    this.setEvents('off');

    this.off('destroy', handleDestroy, this);
}, this);

You don't need to remove the destroy listener as the object we are listening to (itself) is being destroyed so no new events will be emitted from it and there's no external reference to the function or scriptType to be kept in memory

Maybe add removing handlers into EventEmmiter? Since we move callback ownership to EventEmmiter when we call on/once

This is interesting 🤔 . Wondering if this should be on ScriptType or be wrapped in an API

[LeXXik]

@kungfooman right, English is hard 😅

[Maksims]

So it would be more like?

    this.mouseEventHandler = this.app.mouse.on('mousedown', this.onMouseDown, this);

    this.on('destroy', function () {
        this.mouseEventHandler.off();
    }, this);

Yep, those handlers can be stored in an array. To take dynamic subscription into an account, special event handler manager class can be created, which basically stores a list of handlers, and when they are off'ed, will forget about them. And when needed one call to events.off() would off its events. If object is provided to constructor, it will subscribe to its destroy event.

this.events = pc.EventHandlers(this);
this.events.add(entity.on('state', this.onEntityState, this));

When this entity is destroyed, handler will call off to all its events if any left.

[yaustar]

Ah, I see. Sounds similar to @querielo suggestion

[kungfooman]

I have seen this style in Angular, paraphrased:

class EventSink {
    events = [];
    set sink(value) {
        this.events.push(value);
    }
}
const eventSink = new EventSink();

And then it could be used like:

self.sink = pc.app.mouse.on("mousemove", (e) => console.log('mousemove', e.x, e.y));
self.sink = pc.app.mouse.on("mousedown", (e) => console.log('mousedown', e.x, e.y));

This prevents push/add methods or random-but-should-be-unique key object property assignment (just to make it shorter).

To take dynamic subscription into an account, special event handler manager class can be created, which basically stores a list of handlers, and when they are off'ed, will forget about them.

@Maksims You can't forget about the events, since you need to reenable every event between enable/disable toggling.

[kungfooman]

I tried to take in all ideas, but I cannot find a solution that looks nicer than @yaustar's initial ScriptType#listen, so I implemented it myself, just to test around with it:

ScriptTypeListen.js

import * as pc from 'playcanvas';
export class ScriptTypeListen extends pc.ScriptType {
    _sunk = [];
    constructor(args) {
        super(args);
        this.on("enable" , () => this._listenSetEvents('on' ), this);
        this.on("disable", () => this._listenSetEvents('off'), this);
        this.on('destroy', () => this._listenSetEvents('off'), this);
    }
    /**
     * @param {pc.EventHandler         } eventHandler - Whatever extends pc.EventHandler,
     * like `app` or `app.mouse`.
     * @param {string                  } name - Name of the event, like `mousemove`.
     * @param {pc.callbacks.HandleEvent} callback - The callback.
     * @param {any                     } scope - The scope.
     */
    listen(eventHandler, name, callback, scope = this) {
        this._sunk.push({eventHandler, name, callback, scope});
        //console.log("sink", eventHandler, name, callback, scope);
        eventHandler.on(name, callback, scope);
    }
    /**
     * @param {'on'|'off'} onOrOff - On or off.
     */
    _listenSetEvents(onOrOff) {
        //console.log('setEvents', onOrOff);
        for (const {eventHandler, name, callback, scope} of this._sunk) {
            eventHandler[onOrOff](name, callback, scope);
        }
    }
}

Usage is just like described in the first post.

The only issue I ran into is that PlayCanvas doesn't listen on all "interesting" events that developers use/need, so I think pc.Mouse should handle mouseout aswell (anyone wants to make a PR for that?).

Or you end up with code like this:

            this.listen(this.app.mouse, pc.EVENT_MOUSEDOWN , this.onMouseDown );
            this.listen(this.app.mouse, pc.EVENT_MOUSEUP   , this.onMouseUp   );
            this.listen(this.app.mouse, pc.EVENT_MOUSEMOVE , this.onMouseMove );
            this.listen(this.app.mouse, pc.EVENT_MOUSEWHEEL, this.onMouseWheel);
            // Listen to when the mouse travels out of the window
            window.addEventListener('mouseout', onMouseOut, false);
            // Remove the listeners so if this entity is destroyed
            this.on('destroy', function() {
                window.removeEventListener('mouseout', onMouseOut, false);
            });

And then of course... mouseout still runs even when the OrbitCamera is disabled.

[yaustar]

The only issue I ran into is that PlayCanvas doesn't listen on all "interesting" events that developers use/need, so I think pc.Mouse should handle mouseout aswell (anyone wants to make a PR for that?).

That's a thought 🤔 Maybe it should fire an event for that, would make it nicer

[yaustar]

As a note, I would vote for disabled scripts to not be listening for events.

Been thinking on this and actually, if someone does want a disabled script to be listening for events, they can manage it themselves and not use this API

[yaustar]

Been thinking more on this and I'm personally leaning towards a .listen like API myself in the pc.ScriptType.

Would love to get feedback from other PlayCanvas team members on this when they are free :)

[Maksims]

@Maksims You can't forget about the events, since you need to reenable every event between enable/disable toggling.

Off'ed event, destroys its handle, so it should be removed from the list. Every subscription - creates new handle.

[csubagio]

+1 to support in a base class for this. I've ended up doing the same in my project. Paraphrased below for brevity as the actual base class(es!) in the project have a bunch of other stuff in there.

My problem with this though, is that even in this simple example, I've had to make a few design decisions that happen to work for my project. For example, I assume that the reference to a target must exist, so calls to registerEvent can only happen in and after initialize, but that's hard to enforce and doesn't apply to events registered on this script. Because initialize now provides functionality, I have to choose how to override that in the subclass and it just so happens that calling this as part of the function still makes sense for all my classes. Maybe in other contexts it won't, or you wouldn't be able to lump all "base class" functionality into the same location. There's even a huge assumption baked into swap, that I always happen to design for initialize to make sense here, so I can use it to rewire events, but obviously that's not a Script Type SLA. I've made assumptions that make sense in my project about the relationship with the script's enable, but you could just as easily talk about the enable state of the entity, or even the entity ancestry.

All of this to say, adding this functionality to the Script Type directly feels wrong to me. Lots of game engines have experienced base class bloat and paid the price, and while this seems super convenient to the out of box experience, it feels like keeping ScriptType lean should be sacred.

Possible alternatives:

1. Provide a focused derivative: ScriptTypeAutoEvents, specifically for that purpose. Feels lame though, to extend only for one thing.
2. Provide a "fat" version: ScriptTypeExtended. Default to this, and add lots of built in functionality, letting people eject back to ScriptType when they know why and how. Given the engine is open, they can crib from ScriptTypeExtended to just pull what they want/used. Potentially good mix of "it works out of the box" and "you're not locked into stuff you don't need."
3. Try to figure out how to do it as a mixin, via direct prototype extension. This requires more up front education, both on JS and PC, but is ultimately the more reusable chunk of code.

interface RegisteredEventListener {
  target: pc.EventHandler;
  event: string;
  handler: pc.HandleEventCallback;
  registered: boolean;
}

export class ExtendedScriptType extends pc.ScriptType {
  eventListeners: RegisteredEventListener[] = [];

  constructor(...args: ConstructorParameters<typeof pc.ScriptType>) {
    super(...args);
  }
  
  registerEvent( target: pc.EventHandler, event: string, handler: pc.HandleEventCallback ) {
    this.eventListeners.push({target, event, handler, registered: false});
  }
  
  setRegisteredEvents( enable: boolean ) {
    this.eventListeners.forEach( l => {
      if ( enable && !l.registered ) {
        l.target.on( l.event, l.handler, this );
      } else if ( !enable && l.registered ) {
        l.target.off( l.event, l.handler, this );
      }
      l.registered = enable;
    }) 
  }
  
  initialize(): void { 
    this.on('enable', () => {
      this.setRegisteredEvents(true);
    });
    this.on('disable', () => {
      this.setRegisteredEvents(false);
    });
    this.on('destroy', () => {
      this.setRegisteredEvents(false);
    });
    if (this.enabled) {
      this.setRegisteredEvents(true);       
    }
  }
  
  swap(old: ExtendedScriptType): void {  
    old.setRegisteredEvents(false);
    this.initialize(); 
  }
}

[yaustar]

All of this to say, adding this functionality to the Script Type directly feels wrong to me. Lots of game engines have experienced base class bloat and paid the price, and while this seems super convenient to the out of box experience, it feels like keeping ScriptType lean should be sacred.

Not sure how bad the the bloat here would be here. I was intending to only listen to the enable/disable/etc events if this.listen is called. If it's not called, it is in no worse off situation than it is in now performance wise.

Good shout on swap though, not thought about that 🤔

[kungfooman]

@csubagio Don't get too lost in thinking about SLA or whatever is the current buzzword for "better", it can achieve the opposite aswell. Once you start turning every loop body into a method, you simply get lost in otherwise useless extra methods e.g. or you introduce for example more functional programming and then many developers are also not comfortable with that.

I extended ScriptType because that's how it works for adding functionality while still being able to test PR's that don't have that functionality (yet).

Things being lame is often a good thing, because you can deal with it without overstressing systems (be it your own brain or of colleagues). Usually every attempt of doing something "smart" has a trail of issues and bugs all over GitHub (e.g. transpiling mixins into ES5 etc.)

It probably also adds more strain on the decision making process for every dev at the start of writing every new method: "Should I use pc.ScriptType? Wasn't there something about ScriptTypeListen#listen? Or ScriptTypeAutoEvent#listen? Or ScriptTypeExtended#listen? Huh getting lost"

Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?
"The Elements of Programming Style", 2nd edition, chapter 2

[yaustar]

We've had an internal meeting about this and reached the following conclusion. We will create separate PRs for the following:

• Look for ways to improve reporting when this error/situation happens to help developers debug their issues faster.
• Add an 'off by scope' function on the Event Handler to make it easier to remove all event listeners on single object (example from Donovan).
• Add the 'listen' API to the Script Type proposed earlier in a PR for further review.