Timeout Handler not using the correct currentTime

[ZainManji]

For the way I want to use Shifty, I want to be able to seek to any point in a Tweenable's duration, but I never really want to seek to the end of the duration since the stop() function will get called in TimeoutHandler and I don't want to stop the tween.

As a result, what I tried to do is check if the millisecond I'm providing the seek function is >= the Tweenable object's duration, and if it is, change the millisecond to a value just smaller than the duration to ensure that stop() never gets called. However, even after this check, it seemed that stop() would still get called because, the timeoutHandler_isEnded variable in timeoutHandler() would turn true because of the way currentTime was being calculated.

@SBoudrias and I noticed that in seek() when creating the timestamp to pass to timeoutHandler, you call now(), and then when creating the currentTime variable in TimeoutHandler() you call now() again. As a result of calling now() a second time, it isn't the same as the now() call in seek(). As a result there can be a mismatch when calculating currentTime and timeoutHandler_isEnded, which can result in an inaccurate value of the timeoutHandler_offset and can result in stop() being called if you didn't want to.

I made a simple fix in the following link. https://github.com/ZainManji/shifty/blob/master/src/shifty.core.js

You just need to call now() once and pass a reference to it.

[jeremyckahn]

Thanks for reporting this. I'd be concerned that your proposed fix might break regular tweens — can you make a Pull Request so I can pull it down and test out your patch? We can iterate from there.

[SBoudrias]

@jeremyckahn Basically the thing is that there is a couple ms delay between both now() call.

This mean there's a risk a tween will stop without really being at the end of the tween animation. I'm pretty sure usually this doesn't cause any noticeable issues for usual play animations, but it is a precision error that will occurs in tight animations (or depending on the rounding/duration).

I'd be surprised if this fix break anything, it really is just fixing the precision issue by keeping the same "current" time reference for every tween step.

@ZainManji didn't sent a PR as this fix should be normalized across methods calling the timeoutHandler and it'd need unit test. The bug should be easy to reproduce if you setup an animation on a very short time scale.

On the long time run, it might be better to consider moving from timestamp to the context of frame and decouple the tween logic from time (FWIW, this is how Greensock JS api is working). But this is certainly a major refactoring and is not suitable as a simple fix. Keeping a stable current time across a step iteration seems like the easiest way to fix the precision issue.

[jeremyckahn]

Ok, I spent some time playing with @ZainManji's patch — indeed, it prevents regular tweens from terminating. I made some changes that correct that issue, which you can see here. The unit tests pass now, but as @SBoudrias suggested, this deserves a unit test. I didn't experience the initial problem myself, so I'm having trouble coming up with a unit test that would be representative of the bug that this patch fixes. Would one of you be able to test my patch, as well as provide a unit test that you think would be effective?

Thanks!

[SBoudrias]

Hey @jeremyckahn here's an example failure we have: https://github.com/SBoudrias/shifty/compare/failing-test-%2360

Given there can be a minimal delay between both call to now(), the difference between the animation being nearly ended and ended is lost.

[jeremyckahn]

Ok, this looks good. I just released 1.3.10. Thanks for your help, guys!

[jeremyckahn]

@SBoudrias I just noticed this, but the test you provided doesn't actually make any assertions. It's currently throwing an error:

Uncaught Error: stop shouldn't be called

I should have caught this before. Is ea344c2 actually fixing your problem? Should the test be updated?

[SBoudrias]

Yeah the fix have been fixing our issue.

[jeremyckahn]

Hmm. I didn't experience this issue myself, so I'm not sure what an effective test might be. When you have some time, can you try correcting the test to not throw an uncaught error and make an assertion?

[SBoudrias]

Oh yeah, I can take care of that. I'll try to tackle this one tomorrow.