tween.js icon indicating copy to clipboard operation
tween.js copied to clipboard

if stop each tween after in onComplete() callback it went wrong.

Open lesstommy opened this issue 8 years ago • 6 comments

I try to generate 6 tweens at one time, and stop each tween in onComplete callback. like the code below. but i only saw 3 complete massage printed at the console window.

I tried looked into your code, if i use TWEEN.remove(tween) replace the tween.stop(). it can also cauwe this issue.

It seems, the _tween.indexOf(tween) can not find the right one. every time it return the value '0'.

        .onComplete(function(){
            //todo on complete tween.stop() cant use here
            tLog('complete');
           self.tween.stop();
        });

lesstommy avatar Apr 15 '16 09:04 lesstommy

There's no reason to call the stop() function in the onComplete callback. At that point the tween is already "stopped." See if removing that line of code solves your problem. Otherwise, please post the rest of your tweening code so that we can try to figure out what's wrong.

mikebolt avatar Apr 16 '16 04:04 mikebolt

the issue is solved. when it's 'completed' it removes the tween from tween list _tweens; but shouldn't when i try to stop a tween which have already stopped, it just return a error code, not stop another tween? i log it, seems when i use stop(), it removed the item in _tweens[0].

lesstommy avatar Apr 17 '16 17:04 lesstommy

I have a possible solution in my branch: https://github.com/mikebolt/tween.js/commit/ab7da07bb83643c9919af20c4ee558d49ee345bf

The only things that needs to be changed are the update functions. Instead of removing the tween within the global update function, remove it within the instance's update function, immediately before calling the onComplete callback.

However, I realized that this would be a slight performance hit in the standard scenario, because whenever a tween completes normally it will have to remove itself from the array, and it doesn't know its own index in that array, and having it keeping track of that index would not be any better.

So I changed the _tweens variable to be a kind of a hash set instead of an array. All of the operations can now be performed in constant time in any situation, except for getAll, which took a performance hit, but I don't think that's a big deal.

I'm not submitting a pull request yet because it broke the tests. I still need to go through those tests carefully and figure out if my code broke something or if the tests need to be fixed. I think it's a little of both.

mikebolt avatar Apr 17 '16 22:04 mikebolt

@mikebolt this didn't get into master, what are your thoughts now? It's not the first time someone suggests changing the array of tweens into a hash. I wonder if that would help other issues.

sole avatar Oct 03 '16 11:10 sole

@sole Arrays works perfectly, why need stop in onComplete, if elapsed === 1 tween auto removes from tweens array? Or maybe i wrong.

dalisoft avatar Dec 12 '16 04:12 dalisoft

Hello. I found this thread after hitting a similar issue - calling stop inside onComplete. It was a nested call, so not obvious.

It may be worth considering, instead of doing the following near the end of this.update: return false; Doing this: return !_isPlaying;

That way, if a bad client calls stop inside onComplete, Tween.js doesn't remove another random tween from it's array.

Note that that might be a little dangerous. I think that if someone calls start() on a tween twice, it'll be in the array twice, then calling stop() would set _isPlaying to false and the subsequent onComplete would return true and the second copy would never be removed from the array. Note that I didn't test this, so it's just a guess. To avoid this case, instead of returning !_isPlaying, you can push that check to Tween.update, maybe like this:

let curTween = _tweens[i];
if (curTween.update(time) || preserve) {
    i++;
} else if (curTween == _tweens[i]) {
    _tweens.splice(i, 1);
}

or

let curTween = _tweens[i];
if (curTween.update(time) || preserve) {
    i++;
} else {
    this.remove(curTween). 
}

ianhoh avatar Jan 21 '22 22:01 ianhoh

The code has changed a big since 2016. If still any problem, please feel free to open a new issue including a reproduction example that we can run.

trusktr avatar Apr 23 '23 20:04 trusktr