Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [#2347] EventDispatcher concurrent update issue and removal bug #2350

Merged
merged 3 commits into from
Jun 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

## Breaking Changes

-
- `ex.EventDispatcher` meta events 'subscribe' and 'unsubscribe' were unused and undocumented and have been removed

### Deprecated

Expand Down Expand Up @@ -83,6 +83,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).

### Fixed

- Fixed issue with `ex.EventDispatcher` where removing a handler that didn't already exist would remove another handler by mistake
- Fixed issue with `ex.EventDispatcher` where concurrent modifications of the handler list where handlers would or would not fire correctly and throw
- Fixed issue with `ex.ArcadeSolver` based collisions where colliders were catching on seams when sliding along a floor of multiple colliders. This was by sorting contacts by distance between bodies.
![sorted-collisions](https://user-images.githubusercontent.com/612071/172401390-9e9c3490-3566-47bf-b258-6a7da86a3464.gif)

Expand Down
34 changes: 21 additions & 13 deletions src/engine/EventDispatcher.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { GameEvent, SubscribeEvent, UnsubscribeEvent } from './Events';
import { GameEvent } from './Events';
import { Eventable } from './Interfaces/Evented';

export class EventDispatcher<T = any> implements Eventable {
Expand All @@ -13,12 +13,21 @@ export class EventDispatcher<T = any> implements Eventable {
this._wiredEventDispatchers = [];
}

private _deferedHandlerRemovals: {name: string, handler?: (...args: any[]) => any }[] = [];
private _processDeferredHandlerRemovals() {
for (const eventHandler of this._deferedHandlerRemovals) {
this._removeHandler(eventHandler.name, eventHandler.handler);
}
this._deferedHandlerRemovals.length = 0;
}

/**
* Emits an event for target
* @param eventName The name of the event to publish
* @param event Optionally pass an event data object to the handler
*/
public emit(eventName: string, event: GameEvent<T>) {
this._processDeferredHandlerRemovals();
if (!eventName) {
// key not mapped
return;
Expand All @@ -32,7 +41,6 @@ export class EventDispatcher<T = any> implements Eventable {
if (this._handlers[eventName]) {
i = 0;
len = this._handlers[eventName].length;

for (i; i < len; i++) {
this._handlers[eventName][i](event);
}
Expand All @@ -52,16 +60,13 @@ export class EventDispatcher<T = any> implements Eventable {
* @param handler The handler callback to fire on this event
*/
public on(eventName: string, handler: (event: GameEvent<T>) => void) {
this._processDeferredHandlerRemovals();
eventName = eventName.toLowerCase();

if (!this._handlers[eventName]) {
this._handlers[eventName] = [];
}
this._handlers[eventName].push(handler);

// meta event handlers
if (eventName !== 'unsubscribe' && eventName !== 'subscribe') {
this.emit('subscribe', new SubscribeEvent(eventName, handler));
}
}

/**
Expand All @@ -73,6 +78,10 @@ export class EventDispatcher<T = any> implements Eventable {
* @param handler Optionally the specific handler to unsubscribe
*/
public off(eventName: string, handler?: (event: GameEvent<T>) => void) {
this._deferedHandlerRemovals.push({name: eventName, handler});
}

private _removeHandler(eventName: string, handler?: (event: GameEvent<T>) => void) {
eventName = eventName.toLowerCase();
const eventHandlers = this._handlers[eventName];

Expand All @@ -82,13 +91,11 @@ export class EventDispatcher<T = any> implements Eventable {
this._handlers[eventName].length = 0;
} else {
const index = eventHandlers.indexOf(handler);
this._handlers[eventName].splice(index, 1);
if (index > -1) {
this._handlers[eventName].splice(index, 1);
}
}
}
// meta event handlers
if (eventName !== 'unsubscribe' && eventName !== 'subscribe') {
this.emit('unsubscribe', new UnsubscribeEvent(eventName, handler));
}
}

/**
Expand All @@ -98,9 +105,10 @@ export class EventDispatcher<T = any> implements Eventable {
* @param handler The handler of the event that will be auto unsubscribed
*/
public once(eventName: string, handler: (event: GameEvent<T>) => void) {
this._processDeferredHandlerRemovals();
const metaHandler = (event: GameEvent<T>) => {
const ev = event || new GameEvent();
this.off(eventName, handler);
this.off(eventName, metaHandler);
handler(ev);
};

Expand Down
23 changes: 0 additions & 23 deletions src/engine/Events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ export enum EventTypes {
Button = 'button',
Axis = 'axis',

Subscribe = 'subscribe',
Unsubscribe = 'unsubscribe',

Visible = 'visible',
Hidden = 'hidden',
Start = 'start',
Expand Down Expand Up @@ -364,26 +361,6 @@ export class GamepadAxisEvent extends GameEvent<Input.Gamepad> {
}
}

/**
* Subscribe event thrown when handlers for events other than subscribe are added. Meta event that is received by
* [[EventDispatcher|event dispatchers]].
*/
export class SubscribeEvent<T> extends GameEvent<T> {
constructor(public topic: string, public handler: (event: GameEvent<T>) => void) {
super();
}
}

/**
* Unsubscribe event thrown when handlers for events other than unsubscribe are removed. Meta event that is received by
* [[EventDispatcher|event dispatchers]].
*/
export class UnsubscribeEvent<T> extends GameEvent<T> {
constructor(public topic: string, public handler: (event: GameEvent<T>) => void) {
super();
}
}

/**
* Event received by the [[Engine]] when the browser window is visible on a screen.
*/
Expand Down
46 changes: 45 additions & 1 deletion src/spec/EventSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ describe('An Event Dispatcher', () => {
pubsub.emit('event', null);
expect(eventHistory).toEqual(subscriptions);

pubsub.off('event');
pubsub.off('event'); // clear all handlers
subscriptions.push(subscriptions.shift());

eventHistory = [];
Expand Down Expand Up @@ -116,4 +116,48 @@ describe('An Event Dispatcher', () => {

expect(callCount).toBe(1, 'There should only be one call to the handler with once.');
});

it('will handle remove invalid handler', () => {
const eventSpy1 = jasmine.createSpy('handler');
const eventSpy2 = jasmine.createSpy('handler');
const eventSpy3 = jasmine.createSpy('handler');
const dispatcher = new ex.EventDispatcher();
dispatcher.on('foo', () => eventSpy1());
dispatcher.on('foo', () => eventSpy2());
dispatcher.on('foo', () => eventSpy3());
dispatcher.off('foo', () => 'invalid');
dispatcher.emit('foo', null);

expect(eventSpy1).toHaveBeenCalled();
expect(eventSpy2).toHaveBeenCalled();
expect(eventSpy3).toHaveBeenCalled();
expect(eventSpy1).toHaveBeenCalledBefore(eventSpy2);
expect(eventSpy2).toHaveBeenCalledBefore(eventSpy3);
});

it('once will remove the handler correctly', () => {
const eventSpy1 = jasmine.createSpy('handler');
const eventSpy2 = jasmine.createSpy('handler');
const eventSpy3 = jasmine.createSpy('handler');
const dispatcher = new ex.EventDispatcher();
dispatcher.once('foo', () => eventSpy1());
dispatcher.on('foo', () => eventSpy2());
dispatcher.on('foo', () => eventSpy3());
dispatcher.emit('foo', null);
dispatcher.emit('foo', null);

expect(eventSpy1).toHaveBeenCalledTimes(1);
expect(eventSpy2).toHaveBeenCalledTimes(2);
expect(eventSpy3).toHaveBeenCalledTimes(2);
});

it('should not fail if event handlers change during iteration', () => {
expect(() => {
const dispatcher = new ex.EventDispatcher();
dispatcher.once('foo', () => 'foo1');
dispatcher.on('foo', () => 'foo2');
dispatcher.on('foo', () => 'foo3');
dispatcher.emit('foo', null);
}).not.toThrow();
});
});