718425 - Make all Tilt animations less choppy

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

This means intro/outro transitions, reset, move etc. They're sometimes "jumpy", or clumsy anyway.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Let's see if this survives try.
https://tbpl.mozilla.org/?tree=Try&rev=188276f2a4f4

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v3

Fixed a minor problem happening when close was called before the reset sequence finished.

Comment 4

[Rob Campbell [:rc] (:robcee)]

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]]

(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]]

Shouldn't we NOT use setInterval at all but rather window.window.mozAnimationStartTime for pure interpolated goodness at 60FPS ?

Comment 7

[Cedric Vivier [:cedricv]]

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]]

(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]]

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)]

https://hg.mozilla.org/integration/fx-team/rev/b8fce88d3a66

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b8fce88d3a66

Comment 12

[Rob Campbell [:rc] (:robcee)]

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.