Last Comment Bug 718425 - Make all Tilt animations less choppy
: Make all Tilt animations less choppy
Status: RESOLVED FIXED
[tilt]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
: Patrick Brosset <:pbro>
Mentors:
Depends on: 718303
Blocks: 715163
  Show dependency treegraph
 
Reported: 2012-01-16 07:11 PST by Victor Porof [:vporof][:vp]
Modified: 2012-01-28 16:20 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.76 KB, patch)
2012-01-16 07:22 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (4.62 KB, patch)
2012-01-16 14:00 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (5.09 KB, patch)
2012-01-19 06:43 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-01-16 07:11:06 PST
This means intro/outro transitions, reset, move etc. They're sometimes "jumpy", or clumsy anyway.
Comment 1 Victor Porof [:vporof][:vp] 2012-01-16 07:22:17 PST
Created attachment 588874 [details] [diff] [review]
v1

Let's see if this survives try.
https://tbpl.mozilla.org/?tree=Try&rev=188276f2a4f4
Comment 2 Victor Porof [:vporof][:vp] 2012-01-16 14:00:21 PST
Created attachment 588996 [details] [diff] [review]
v2

I had to use requestLongerTimeout. I'm not happy, but I had to!
https://tbpl.mozilla.org/?tree=Try&rev=9e8a72a684de
Comment 3 Victor Porof [:vporof][:vp] 2012-01-19 06:43:34 PST
Created attachment 589855 [details] [diff] [review]
v3

Fixed a minor problem happening when close was called before the reset sequence finished.
Comment 4 Rob Campbell [:rc] (:robcee) 2012-01-24 12:38:01 PST
Comment on attachment 589855 [details] [diff] [review]
v3

+    if ("function" === typeof this.onResetStart) {
+      this.onResetStart();
+      this.onResetStart = null;
+    }
+

there's that pattern again. Where did you get that?

If I'm reading this patch correctly, you're setting up a reset function on an interval allowing the animation to "glide" to to the final orientation. Should improve fluidity, I think.
Comment 5 Victor Porof [:vporof][:vp] 2012-01-24 22:58:36 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Comment on attachment 589855 [details] [diff] [review]
> v3
> 
> +    if ("function" === typeof this.onResetStart) {
> +      this.onResetStart();
> +      this.onResetStart = null;
> +    }
> +
> 
> there's that pattern again. Where did you get that?
> 

How would you test something that you don't know when it ends, and by avoiding creating a notification who no one will use except you? This is what onResetFinish does. I also added onResetStart to exactly know when this happens (and if this happens) in tests. 

> If I'm reading this patch correctly, you're setting up a reset function on
> an interval allowing the animation to "glide" to to the final orientation.
> Should improve fluidity, I think.

I'm not creating the interval here, I'm only changing how it works. The interval was there before.
Comment 6 Cedric Vivier [:cedricv] 2012-01-25 04:24:44 PST
Shouldn't we NOT use setInterval at all but rather window.window.mozAnimationStartTime for pure interpolated goodness at 60FPS ?
Comment 7 Cedric Vivier [:cedricv] 2012-01-25 04:31:56 PST
Following up on that, it seems to me the current solution will probably not solve choppiness problems on slow hardware that won't be able to keep up with one (or multiple) 60FPS setInterval.

Ideally, all active animations should be interpolated against the animation start time and the frame's animationStartTime - so that interpolations looks right regardless of the current FPS.
Comment 8 Victor Porof [:vporof][:vp] 2012-01-25 04:44:55 PST
(In reply to Cedric Vivier [:cedricv] from comment #6)
> Shouldn't we NOT use setInterval at all but rather
> window.window.mozAnimationStartTime for pure interpolated goodness at 60FPS ?

This bug does NOT refer to the interval problem, it only changes the way some animations (particularly the arcball reset) work *internally*. So it does not touch the already existing interval idea.

However, I fully agree, we should use mozAnimation for all animations. A separate bug seems suited for this (especially with the already massive queue on top of this patch).
Comment 9 Victor Porof [:vporof][:vp] 2012-01-25 05:28:56 PST
To be perfectly clear, here is an example of what this bug is supposed to fix:

* open a rather large page (this bug could work)
* scroll it to bottom and inspect an element (like this reply)
* open in tilt
* pan using the mouse (right click + drag) to the top
* press the 3D View button, not ESCAPE (because bug 719039 hasn't landed yet)

The outro animation jumps to the bottom, then back up a bit, and then finally transitions to the flat page. Clumsy, like I said in comment #0 :)


Filed bug 720992.
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-25 11:47:00 PST
https://hg.mozilla.org/integration/fx-team/rev/b8fce88d3a66
Comment 11 Tim Taubert [:ttaubert] 2012-01-25 23:57:34 PST
https://hg.mozilla.org/mozilla-central/rev/b8fce88d3a66
Comment 12 Rob Campbell [:rc] (:robcee) 2012-01-27 07:35:05 PST
Comment on attachment 589855 [details] [diff] [review]
v3

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: Users will experience jittery transitions in and out of 3D view if we don't take it
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): Low.

Note You need to log in before you can comment on or make changes to this bug.