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

Select correct ticker function during initialization, not only when document visibility changes #725

Merged
merged 1 commit into from
Dec 2, 2016

Conversation

iwsfg
Copy link
Contributor

@iwsfg iwsfg commented Nov 30, 2016

Fixes #611

@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 1, 2016

ugh, it didn't work. I'm pretty sure it did when I tested it year ago

@iwsfg iwsfg closed this Dec 1, 2016
@Rycochet
Copy link
Collaborator

Rycochet commented Dec 1, 2016

Was finishing up getting the pause in and eyeing this up next, any idea why it doesn't fix it?

@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 1, 2016

I'm not sure yet. Animation starts playing in the background as it supposed to, but instead of stopping at the end it plays it backwards to initial state with the same speed, then forward again and so on. I was thinking it might be because of tick() call which is unnecessary upon initialization, but that's not it.

I'll take another look this evening (in like 6 hours)

@Rycochet
Copy link
Collaborator

Rycochet commented Dec 1, 2016

I run a D&D game on Thursday evenings, so won't be able to look till tomorrow anyway - and trying to update QUnit to v2 to improve the testing right now ;-)

@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 2, 2016

Hi. Diidn't had time to take a look into this yesterday, so I'm just getting started. Something has changed indeed. I tried to roll back to ce90611 (year ago), apply the patch and it did the thing. Also works with 1.3.1, but breaks on 1.3.2 release.

I also noticed another new behavior with 1.3.2, which is not mentioned in release notes: all running animations are getting fast-forwarded to the end with the next tick after tab is moved to background.

Here's JSFiddle: 1.3.1 and 1.3.2. I don't know if it's intended or not. I can definitely see how this could be a desired behavior, but personally I would consider this a breaking change.

Thought, I would let you know asap. Will keep you updated.

@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 2, 2016

I forgot what kind of value rAF callback receives. This comment has been updated to tell the truth.


Caused by 130d2e6. It mixes together Date.now() values (time since to Jan 1, 1970) with time performance.now() returned by native requestAnimationFrame (time since page has been loaded):

I dropped console log there and this is what happens:

timestamp attr: 1600.3140000029816, currentTime: 1600.3140000029816
timestamp attr: 1616.9799999988754, currentTime: 1616.9799999988754
timestamp attr: 1633.6460000020452, currentTime: 1633.6460000020452
timestamp attr: true, currentTime: 1480664299903
first animation finished
second animation finished

And if animation is started while page was in the background and used setTimeout() instead of rAF - we go from bigger to smaller numbers, so animation thinks it should play for the next ~47 years.

I'm going to do few things:

  • Update rAFShim so it would count time from the moment it's been created rather than using Date.now() as is and if available - native performance.now()
  • Reuse setTimeout implementation from the rAFShim in "visibilitychange" event handler
  • At this point changes from 130d2e6 probably will not be needed and code of the tick function could be simplified.

Expect another pull request landing in few minutes. I'm too lazy to create an issue for this bug though, so if you use them for tracking various problems - you might want to create one yourself.

@Rycochet
Copy link
Collaborator

Rycochet commented Dec 2, 2016

Not going to roll that back - but definitely needs fixing - if it's in the background then it should use the "normal" time, and not the high resolution time.

Rycochet added a commit that referenced this pull request Dec 2, 2016
@Rycochet
Copy link
Collaborator

Rycochet commented Dec 2, 2016

Hopefully this fixes it - can't have a "perfect" timer as the start time isn't known, but looks to be within a couple of ms of the expected time to me - which should hopefully let this PR work properly again :-)

@Rycochet Rycochet reopened this Dec 2, 2016
@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 2, 2016

Hopefully this fixes it - can't have a "perfect" timer as the start time isn't known

Here was my take on this. Instead of creating local polifyll for performance.now() I updated rAFShim. It uses performance.now() when present, so we can safely interchange it with native implementation of requestAnimationFrame and timestamp returned to callback will be on the same scale.

When performance.now() is not available - native version of requestAnimationFrame probably isn't present as well, so we shouldn't care, but timeCurrent is measured from the moment code definyning polifill was executed.

With this

var timeCurrent = Velocity.timestamp && timestamp !== true ? timestamp : performance.now();

could be simplified to

var timeCurrent = timestamp;

....

Now thinking of what I did with timestamps when performance API is not available, that might lead to problems in environments where window.requestAnimationFrame was monkey patched. Because verison from another polifyll will probably use just new Date().getTime() returned to the callback.

With all this in mind, I think you should update that line to simply

var timeCurrent = performance.now();

Only this way we know for sure returned values will be count from the same origin in every situation.

@iwsfg
Copy link
Contributor Author

iwsfg commented Dec 2, 2016

which should hopefully let this PR work properly again :-)

Can confirm - it works properly as is with current master. It's up to you now

@Rycochet
Copy link
Collaborator

Rycochet commented Dec 2, 2016

Checking on MDN, performance.now() is available somewhat later than requestAnimationFrame (but both quite a few years ago) - so either way works.

If you look at #684 (specifically the graphs in it) you can see that performance.now() is definitely not good enough for normal use (it's barely better than Date.now()) - so it does need to use the rAF timestamp when available. The possibility of a "broken" performance.now() polyfill is there - but as Velocity is planning on dropping the legacy browsers that don't support it at some point I think it's ok to use.

Saying that, it might be worth improving the new polyfill code to try and detect broken API access - maybe check for decimal places, and if none then assume broken?

Not quite sure how we can make this part of the test suite either...

@Rycochet Rycochet merged commit 579c4d7 into julianshapiro:master Dec 2, 2016
@iwsfg iwsfg deleted the fix-ticker-on-load branch December 2, 2016 11:06
Rycochet added a commit that referenced this pull request Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants