Jump to content
Search Community

problem with removeEventListener scopes

erikb test
Moderator Tag

Warning: Please note

This thread was started before GSAP 3 was released. Some information, especially the syntax, may be out of date for GSAP 3. Please see the GSAP 3 migration guide and release notes for more information about how to update the code to GSAP 3's syntax. 

Recommended Posts

Discovered some strange behavior in my code.

 

Looks like TweenMax's removeEventListener is not taking the scope of the added listener into account and thereby removing other  scoped callbacks' listeners.  Advise?

 

Looks like the issue is here: 

https://github.com/greensock/GreenSock-JS/blob/master/src/uncompressed/TweenMax.js#L6047

 

See the Pen jrPEGj by jedierikb (@jedierikb) on CodePen

Link to comment
Share on other sites

You can use bind, or not put the callback on your object's prototype (create the callback in the constructor).

 

I think the ticker could use a little overhaul. I have some suggestions on how to handle issues like yours and a couple others. I'm heading out the door, so I'll post them later.

  • Like 3
Link to comment
Share on other sites

Happy to look at any suggestions you've got, Blake. 

 

@erikb, there is no such parameter for the removeEventListener() method. This is the first time anyone has requested something like that, and it should be pretty easy to work around in various ways, like creating your own bound function as you discovered. 

  • Like 2
Link to comment
Share on other sites

  • 1 month later...

I totally forgot about this...  :?

 

To fix @erikb's scope issue, you could check the scope of the listener...

if (list[i].c === callback && list[i].s === scope) {}

Most of my suggestions are based on this code...

See the Pen 67d323d539b9fd2fbfdb82be7ab31f12?editors=0010 by osublake (@osublake) on CodePen

 

First, I was wondering if there was any reason why GSAP doesn't use performance.now over Date.now?

 

For GSAP's EventDispatcher, it would be really nice to be able to add a listener that only gets called once. So the next time that event is called, the listener would be removed. The name of this method could be "once".

Draggable.once("throwupdate", listener);
TweenLite.ticker.once("tick", listener);

It would also be nice if addEventListener and removeEventListener could be aliased with on and off. So this...

TweenLite.ticker.addEventListener("tick", listener);
TweenLite.ticker.removeEventListener(listener);

Could become this...

TweenLite.ticker.on("tick", listener);
TweenLite.ticker.off(listener);

The ticker only has 1 event, so this is even better...

TweenLite.ticker.add(listener);
TweenLite.ticker.remove(listener);
TweenLite.ticker.addOnce(listener);

Another suggestion for the ticker is to have some of the stuff that is already being calculated passed to the listener.

TweenLite.ticker.add(function(deltaTime, frameCount, time) {

});

I was also wondering if you could improve the performance of callbacks by using call instead of apply. If you look at the emit method in my EventEmitter class, you can see how I do it using a switch. It requires a little more code, but seems to be a lot faster. Since callbacks use an array of params, you could use that instead of the arguments.

  • Like 2
Link to comment
Share on other sites

I like a lot of the suggestions, Blake. I think they're more appropriate for the v2.x rewrite, though, rather than shoe-horning them into 1.x. I just feel this need to be very careful about adding more kb to the core TweenLite file especially since so few people seem to care about these kinds of features. I almost never see people tapping into the ticker's event dispatching out in the wild. 

 

I use Date.new instead of performance.now for a few reasons:

  1. Compatibility (only more modern browsers support performance.now)
  2. In most browsers I tested, performance.now() was slower.
  3. I can't imagine that the difference between using performance.now() and Date.now() would be noticeable in any way. 

Do you feel differently? 

 

Oh, and you mentioned using call() instead of apply() for performance reasons - you're just talking about things like onComplete, onUpdate, etc., right? (ticker uses call() already). In my tests, the extra code/logic necessary to do that made it almost the same performance-wise. I didn't see much of a gain at all, and it would definitely bloat file size a fair amount (including in TweenLite which is where it's most painful).  

  • Like 1
Link to comment
Share on other sites

Wouldn't the event dispatching changes also apply to Draggable? So it's not limited to the people who use the ticker.

 

For performance.now, the differences can be noticeable over time as the Date doesn't remain constant. And the microsecond accuracy can be important for some game or physics engines.

 

If a browser has requestAnimationFrame, shouldn't it also have performance.now? I've never looked into that, but I thought that was one of the main reasons for it. The timestamp is same format as requestAnimationFrame, so you don't need to call it on every frame.

var elapsed, lastTime = performance.now();
requestAnimationFrame(animate);

function animate(currentTime) {
  elapsed = currentTime - lastTime;
  requestAnimationFrame(animate);
  lastTime = currentTime;
}

And yes, for the call() and apply() cases, I was only talking about the callbacks like onUpdate. I got the idea about using a switch from a couple different libraries. I thought it was strange at first, but all the code comments claim it's faster. I'm not sure how you judge the cost/benefit for file size, but it really would only be a few more lines of code. Pretty much this, except for setting the scope, param, and callback variables.

function callback() {  
  var len = p ? p.length : 0;    
  switch (len) {
    case 0: fn.call(scope); break;
    case 1: fn.call(scope, p[0]); break;
    case 2: fn.call(scope, p[0], p[1]); break;
    case 3: fn.call(scope, p[0], p[1], p[2]); break;
    case 4: fn.call(scope, p[0], p[1], p[2], p[3]); break;
    default: fn.apply(scope, p);
  }
}

JsPerf is down, so I made a simple test. Not sure how accurate that is as I don't do a lot of benchmarking. Would that be noticeable if you use onUpdate a lot?

See the Pen 06821993420020d18781b6c643615410?editors=0010 by osublake (@osublake) on CodePen

Link to comment
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...