-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
EventDispatcher: remove unwanted references in event objects. #18564
Conversation
@@ -76,6 +76,8 @@ Object.assign( EventDispatcher.prototype, { | |||
|
|||
} | |||
|
|||
event.target = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
Should we replace https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent |
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. |
😮 |
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? |
Alright, lets see what happens 🤞 |
Thanks! |
This is breaking callbacks that use debounce/throttle. Maybe there should be a way to disable/enable this behaviour? |
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 |
Good idea! I have added a note to the migration guide. |
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
drop the reference after use to prevent this.