Skip to content
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

[BUG] Animation duration is not consistent across browsers #5329

Closed
serhii-yakymuk opened this issue Mar 10, 2018 · 5 comments
Closed

[BUG] Animation duration is not consistent across browsers #5329

serhii-yakymuk opened this issue Mar 10, 2018 · 5 comments
Milestone

Comments

@serhii-yakymuk
Copy link
Contributor

serhii-yakymuk commented Mar 10, 2018

Consider this generic example:

https://jsfiddle.net/8g3g3z3u/1/

Expected Behavior

Animation should last for 10 seconds in any browser.

Current Behavior

Chrome, Firefox: ~17 sec animation
IE, MS Edge: ~8 sec animation

Timings have the same relative values for any duration set.
That is kind of problematic for user expirience in different browsers.

Environment

  • Chart.js version: 2.7.2
  • OS: Windows 10

Possible Solution

Looks like the problem is with frameDuration variable and whole 'drop frames' logic:

frameDuration: 17,

I'm wondering if we can just update addAnimation function to include these lines:

animation.startTime = Date.now();
animation.duration = duration;

And later in advance function use them to calculate currentStep:

animation.currentStep = (Date.now() - animation.startTime) / animation.duration * animation.numSteps;
animation.currentStep = Math.min(animation.currentStep, animation.numSteps);

I'm playing with this version locally, and it has much better timing.
Maybe I'm missing something important though, why is that 'frame drop' logic relevant?

@etimberg
Copy link
Member

This is some of the oldest code in v2 chartjs, so I'll do my best to explain with the caveat that I'm not the original author on this.

The original intention was to allow animations to finish in the correct wall clock time, even if it drawing takes more than the time allotted for a single frame. window.requestAnimationFrame wants to fire at 60fps giving us at most 16.66ms to render & paint the screen. However, it's unlikely that we can actually render in that time so the idea was to skip ('drop') frames in to catch back up to the right point in the wall clock animation.

That being said, looking at the current implementation I see a few bugs:

  • frameDuration should be 16.66
  • animation.currentStep should be an integer instead of a float (ie, needs to be floored when set)

Now, to your proposed solution. I think it's much simpler and I think it achieves the same end result. I would, however, recommend the following change

animation.currentStep = Math.floor((Date.now() - animation.startTime) / animation.duration * animation.numSteps);

To test this I would recommend the following cases be tested. These may need to be manual tests since they may be hard to write for the CI.

Test Case Test Result
Fast Render Rendering capped at 60fps
Slow Render Rendering finishes as close to wall clock time as possible

@simonbrunel thoughts? Did I miss something?

@serhii-yakymuk
Copy link
Contributor Author

serhii-yakymuk commented Mar 11, 2018

@etimberg
I've done some testing for Fast render and Slow render (mocking 100-200ms computation in afterDatasetsDraw hook) - it works as expected.
Fast render sometimes gets the same currentStep twice though, not sure if that's a big problem.
Maybe we can skip render if (currentStep === previousStep) ?

There is a problem in this line btw, duration from animationOptions is not propagated:

Chart.animationService.addAnimation(me, animation, duration, lazy);

Should be:
Chart.animationService.addAnimation(me, animation, duration || animationOptions.duration, lazy);

I can create a fork with my proposed solution, so you guys can play around yourself.

@etimberg
Copy link
Member

@serhii-yakymuk definitely create a branch and open a PR that we can play around with

@simonbrunel
Copy link
Member

Fixed by #5331

@etimberg
Copy link
Member

etimberg commented Jan 3, 2019

Closing per the comment above

@etimberg etimberg closed this as completed Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants