EventDispatcher: remove unwanted references in event objects.

[aardgoose]

Use of EventDispatcher with long lasting event objects can create unwanted references to objects.

For example: the pre allocated _removedEvent in Object3D at

three.js/src/core/Object3D.js

Line 26 in 9c9d05a

var _removedEvent = { type: 'removed' };

can will gain a 'target' reference to a removed Object3D if the object has any event listeners.

drop the reference after use to prevent this.

[Mugen87]

Um, I'm not sure it's a good idea to alter the event object after it was dispatched. Users could work with the event object not just within the event listener. They could save the event and work with it later on. In this case it would be unexpected when target is suddenly null.

[aardgoose]

I see your point, although anyone being called knows what target is (this), in the listener functions. Is target used anywhere within Three? I can't see any obvious uses.

An alternative would be to zap it specifically in the Object3D.remove() method after the dispatch call. In those cases anyone saving the event object could get surprises anyway since it would be shared with other _removed events after that point.

[gkjohnson]

I was thinking the same thing but if that's the case any reused event definitions like this would be problematic because target will get overwritten with every subsequent call. TrackballControls, TransformControls, OrbitControls and I'm sure others all use this same pattern.

These events were originally moved to be reused in order to save on memory allocation (see #17224, #17134 (comment)) but I missed that the object was modified in the dispatchEvent function. I personally am okay with denoting that an event is only valid during the execution of the callback, which would allow us to save memory allocation in quite a few other places.

Setting the target to null in the dispatchEvent function would change the functionality relative to the built relative to the EventTarget.dispatchEvent function, though, so it might be more sensible to set target to null in the remove() function, instead.

[Mugen87]

I've totally forgot that three.js code already reuses event objects...

In this case, I prefer whatever @mrdoob likes. Also note that this class comes from a separate github repo. It could be useful to make changes also there: https://github.com/mrdoob/eventdispatcher.js/

[mrdoob]

Should we replace EventDispatcher with CustomEvent and include the polyfill for IE?

https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent

[gkjohnson]

Should we replace EventDispatcher with CustomEvent and include the polyfill for IE?

I think this is a bit of an orthogonal question because the native version would suffer from the same memory issues from reusing events so we'd have to solve the same question either way:

const _removeEvent = new CustomEvent( 'removed' );

// ...

this.dispatchEvent( _removeEvent );
console.log( _removeEvent.target );

// logs this

And after doing a quick benchmark it looks like the three.js implementation is notably faster than the native one in both firefox and chrome (at least in extreme cases):

const nativeTarget = new EventTarget();
const nativeEvent = new CustomEvent( 'event' );
const threeTarget = new THREE.EventDispatcher();
const threeEvent = { type: 'event' };

for ( let i = 0; i < 10; i ++ ) {

  nativeTarget.addEventListener( 'event', () => {} );
  threeTarget.addEventListener( 'event', () => {} );

}

console.time( 'native dispatch' );
for ( let i = 0; i < 1000; i ++ ) nativeTarget.dispatchEvent( nativeEvent );
console.timeEnd( 'native dispatch' );

console.time( 'three dispatch' );
for ( let i = 0; i < 1000; i ++ ) threeTarget.dispatchEvent( threeEvent );
console.timeEnd( 'three dispatch' );

// chrome
// native dispatch: 31.4951171875ms
// three dispatch: 0.4990234375ms

// firefox
// native dispatch: 7ms - timer ended
// three dispatch: 1ms - timer ended

I think the native event dispatcher is built to handle things that three.js doesn't need or won't use so by using the custom one we can create a leaner and more memory conscious implementation.

[Mugen87]

// native dispatch: 31.4951171875ms
// three dispatch: 0.4990234375ms

😮

[aardgoose]

@mrdoob

For the moment is it acceptable; to zap the retained target references in the static events in Object3D. after the dispatchEvent() has completed?

The static events in the various controls are enclosed by the controllers so those don't pose the same hazard.

At the same time is it worth documenting the events dispatched by Object3D and at the same time document this hazard?

[mrdoob]

Alright, lets see what happens 🤞

[mrdoob]

Thanks!

[taseenb]

This is breaking callbacks that use debounce/throttle. Maybe there should be a way to disable/enable this behaviour?

[gkjohnson]

This is breaking callbacks that use debounce/throttle.

You can work around this by passing in the target to the debounced function instead of the event object:

object.addEventListener( 'add', e => {

  debouncedFn( e.target );

} );

@mrdoob @Mugen87 that event.target is only valid for the duration of the listener callback should probably be listed as a breaking change in the migration guide.

[Mugen87]

Good idea! I have added a note to the migration guide.