ded / morpheus

A Brilliant Animator
504 stars 57 forks source link

Revert to previous RAF shim, support window.performance.now() #36

Closed danro closed 11 years ago

danro commented 11 years ago

Tests passing in Chrome 25, but I haven't tried IE10.

This should hopefully add support for the new timers with minimal impact on current performance. And would close #35 as well.

(also added a .jshintrc for Sublime love)

danro commented 11 years ago

Tests passing in IE10.

danro commented 11 years ago

Wait, scratch that- all tests were passing and then I reloaded it a few times. Now the Colors, long hex + short hex are failing. Good old IE.

danro commented 11 years ago

OK phew. Somehow IE10 had switched to "Compatibility View" which caused the breaking.

IE10 tests passing. :hammer:

ded commented 11 years ago

:point_right: :ok_hand:

ded commented 11 years ago

this looks solid

rvagg commented 11 years ago

Thanks muchly for this @danro, saved me going insane with the latest Chrome.

But, FF doesn't appear to use the performance timings for rAF, a +new Date would be a better choice, an even better choice would be to use window.mozAnimationStartTime.

I haven't bothered looking at IE, Opera or Safari at this stage but we need to make sure that if a browser has window.performance that it actually uses that clock as the reference for rAF callbacks; Gecko has it but doesn't use it for rAF. (I'm running Aurora fwiw so window.performance may not be in the release channel yet.)

rvagg commented 11 years ago

This works for Chrome & Aurora: https://github.com/rvagg/morpheus/commit/1c51a1e51ae9bf87e0be83a6ba56f9f71e85b085 Thoughts?

rvagg commented 11 years ago

I'm having a git-derp day today, see this commit instead: https://github.com/rvagg/morpheus/commit/d46f43904eb955888a1a4c1e1a79de65238bf661

danro commented 11 years ago

Ahh thank you @rvagg, I was having my own special firefox moment today w/ morpheus. This explains it.

The tests were passing, so perhaps a new test is in order?

rvagg commented 11 years ago

ok, next derpage.. IE9 has window.performance but not window.performance.now so it completely barfs. Next iteration that gets it working in Chrome dev, Aurora and IE9: https://github.com/rvagg/morpheus/compare/animationstart

danro commented 11 years ago

Another thing we could do to make it even safer moving forward would be to completely ignore the parameter being passed to the RAF callback, and just use your startTime() (maybe that would make it time()) on each run to get the current time. I think any perceived perf losses from the function wrapper would be regained from having more accurate timers in Chrome & FF.

danro commented 11 years ago

@rvagg I'm gonna pull from you and write the code for that if u like?

rvagg commented 11 years ago

Could do, except for Gecko, mozAnimationStartTime is an absolute reference for the current page refresh so it doesn't change, so it works a little different to the others. I think that +new Date will still work for Gecko (for now) so we could just ignore their special property. My OS X VM isn't working at the moment so I can't check this on latest Safari, could someone do some Safari tests on this stuff to see what it's doing? My fear is that Chrome is picking up the use of the window.performance.now() value as its reference while the others are just using an epoch reference value, what happens when others start to implement window.performance the same way that Firefox has? We're going to end up in feature-detection hell.

rvagg commented 11 years ago

And yes, do what you want with my branch, you're welcome to turn it into a PR if you think you can make it sensible across all browsers. I just need something that works right now.

rvagg commented 11 years ago

My code is good in IE7 to IE9, Chrome 23 & 24, Opera, Firefox Aurora & Release, Safari 5.1 (Win) but I can't test anything on OS X and I also don't have IE10 handy right now.

danro commented 11 years ago

Well, in FF 15 it looks like they're supporting window.performance.now() so maybe we just roll with that and ignore their non-standard mozAnimationStartTime ?

Just tested in Safari 6.0.2 and it doesn't have window.performance yet.

rvagg commented 11 years ago

can't use window.performance.now() on FF as it's not used as the base for rAF, only Chrome does that; so far anyway.

danro commented 11 years ago

I'll pull and sketch out what i'm thinking. Stand by..

rvagg commented 11 years ago

in FF, open a Web Console and run this: console.log(window.performance.now(), window.mozAnimationStartTime, +new Date)

/me pulls hair

But this is the combined fault of the spec (for not mandating a window.animationStartTime) and Chrome for going it alone on the window.performance.now() as a base.

rvagg commented 11 years ago

btw, a few alternatives I considered:

  1. leave start=null then if (!start) start = +new Date in the loop
  2. add an additional check in the loop that resets start = +new Date if start < now or delta < 0, or perhaps delta < -10 just to be sure.
  3. ignore the parameter passed to the rAF callback completely.

The problem with (1) is that we lose the first iteration cause we don't have a reference time, I'm not sure how much that matters though, at least we get to ignore these cross-browser issues though. The problem with (2) is just more cycles for the check, perhaps not a big deal though. (3) would presumably mean we lose out on the slight performance gains offered by not having to do a Date each iteration when we have rAF.

Each solution would mostly future-proof any implementation changes; we can guarantee that at some point in the (near?) future the current code is going to break.

Thoughts @ded?

danro commented 11 years ago

Here's what I was thinking for ignoring the RAF callback param.

https://github.com/rvagg/morpheus/pull/1/files

danro commented 11 years ago

Another plus to ignoring the rAF callback param, we could additionally shim in support for webkitNow and such other prefixed nonsense (meaning high res timers for current builds).

danro commented 11 years ago

Edited.

danro commented 11 years ago

Pushed another commit to https://github.com/rvagg/morpheus/pull/1.

I moved the now() call to only occur once per render loop. (meaning only once per however many morpheus tweens are happening on that frame).

This feels like the safest approach to me, and perhaps we can revisit later w/ stress tests to see if it's negatively impacting the old set of rAF browsers. Morpheus could probably use some animation demos anyway.

rvagg commented 11 years ago

I've taken a totally different tack with an uber-feature-detect, heavily documented to save confusion, see code here: https://github.com/rvagg/morpheus/compare/timer, let me know what you think. It's a multi-stage feature-detect that should be robust enough to last for a while.

I've also included a test case that should probably make it in regardless, it'll ensure that an animation is actually happening--or at least it's not just jumping straight to the final state or doing something else weird.

rvagg commented 11 years ago

And I've given you commit access to my fork @danro so feel free to mess around in there. I see you're doing some more interesting work with the possible performance.*now() variations that I haven't touched in my latest.

danro commented 11 years ago

Oh, interesting feature detect work there!

I'm currently on deadline and won't be able to come back to this for a few days, so I may stick with my safe + small edit for now, but I like the feature detect idea on page load.