Making owner event listener management for components less of a chore

[Autsider666]

Context

Having to manually manage all the listeners/subscriptions from listeners applied during onAdd and removed during onRemove takes a lot of time (in total at least) and can easilly be done wrong, causing all kinds of unexpected side-effects.

Proposal

I've experimented with a few setups so far and this one is the easiest to use so far, but there's a lot of room for improvement.

There are a few points where my example would need some TLC:

• It's build upon the assumption of the owner always being an Actor. I guess it's 99% of the time correct for most people, but maybe split the on function into onActor and onEntity?
• The on function name is technically consistent with the event handling everywhere else, but it implies the existence of off and emit and it's not fully communicating the functionality of "add listener during onAdd and remove it during onRemove"
• I thought the addition of event.owner would be useful, but I ended up never using it + it's quite ugly.

export type OwnerHandler<EventType> = (event: EventType & { owner: Actor }) => void;

export abstract class BaseComponent extends Component {
    declare owner?: Actor;

    private readonly callbacks = new Map<EventKey<ActorEvents>, OwnerHandler<ActorEvents[EventKey<ActorEvents>]>[]>();

    private readonly subscriptions: Subscription[] = [];

    onAdd(owner: Actor): void {
        for (const event of this.callbacks.keys() as unknown as EventKey<ActorEvents>[]) {
            const handlers = this.callbacks.get(event) ?? [];
            for (const handler of handlers) {
                const subscription = owner.on(event, event => {
                    event.owner = owner;
                    handler(event);
                });
                this.subscriptions.push(subscription);
            }
        }
    }

    onRemove(): void {
        for (const subscription of this.subscriptions) {
            subscription.close();
        }
    }

    protected on<TEventName extends EventKey<ActorEvents>>(eventName: TEventName, handler: OwnerHandler<ActorEvents[TEventName]>): void {
        if (!this.callbacks.has(eventName)) {
            this.callbacks.set(eventName, []);
        }

        this.callbacks.get(eventName)?.push(handler as OwnerHandler<ActorEvents[EventKey<ActorEvents>]>);
    }
}

An example of how I used this code:

export class HasTargetComponent extends BaseComponent {
    constructor(public readonly target: Actor) {
        super();

        this.on('preupdate', this.onPreUpdate.bind(this));
    }

    private onPreUpdate(): void {
        if (this.target.isKilled()) {
            this.owner?.removeComponent(HasTargetComponent);
        }
    }
}

[mattjennings]

Coincidentally @eonarheim, @jyoung4242 and I were just discussing something like this. Good timing.

I don't think it should assume the owner is an actor, because entity + component = actor. Additionally components may need to emit events themselves so I wouldn't want them to simply alias everything to the owner. But forwarding update events from the entity would be good, since those are on all entities, and providing overridable onXYZ update methods like actors do.

It also would be better to extend or implement EventEmitter rather than just add an on method, as users may want to use once, off, etc. This would allow the component to emit its own events too.

[Autsider666]

It also would be better to extend or implement EventEmitter rather than just add an on method, as users may want to use once, off, etc. This would allow the component to emit its own events too.

You gave me an idea: What if we (de)register ((un)pipe) the component EventEmitter on the EventEmitter of the owner during onAdd/onRemove?

Calling (and maybe, in a later version, replacing?) onAdd/onRemove with component events would make it even better (because consistency)

[eonarheim]

The pattern we've been using other places is composition w/ method forwarding (inconsistently). (The ColliderComponent is an example)

1. Create a new EventEmitter public events = new EventEmitter()
2. Forward the event methods for convenience (Entity does this) https://github.com/excaliburjs/Excalibur/blob/main/src/engine/EntityComponentSystem/Entity.ts#L604-L631

I don't think it should assume the owner is an actor, because entity + component = actor. Additionally components may need to emit events themselves so I wouldn't want them to simply alias everything to the owner. But forwarding update events from the entity would be good, since those are on all entities, and providing overridable onXYZ update methods like actors do.

Totally agree @mattjennings, we are of the same mind here.

You gave me an idea: What if we (de)register ((un)pipe) the component EventEmitter on the EventEmitter of the owner during onAdd/onRemove?

I think this is a totally resonable thing to do. ColliderComponent does this manually to manipulate the shape of the event, but we can probably change this up and pick a single target/shape

[Autsider666]

Hmmm, no clue why, but the code that I posted at the start doesn't do anything in 0.29 for some reason.
Weird...

[github-actions]

This issue hasn't had any recent activity lately and is being marked as stale automatically.