1018255 - Followup tweaks to the overscroll effect physics

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We anticipate tweaking the overscroll effect introduced in bug 998025.  This bug will collect the requirements as they show up with testing, perhaps turning into a meta bug.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Random changes tweaking behaviour. This is to record the conversation, the code is not meant for actual use.

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 1 - Stop fling more quickly when overscrolled

This is basically straight out of Milan's patch.

Comment 3

[Botond Ballo [:botond]]

Attachment: Part 2 - Don't bother doing the computations in ApplyOverscrollEffect if we are not overscrolled

Calling ApplyOverscrollEffect when we are not overscrolled should be harmless, but it's needless work so we should avoid it.

Comment 4

[Botond Ballo [:botond]]

Attachment: Part 3 - Fix a short-circuit evaluation bug in OverscrollSnapBackAnimation::Sample

Comment 5

[Botond Ballo [:botond]]

Attachment: Part 4 - Make the translation (in the direction of overscroll) more pronounced than the zoom

This is the "making the Z-effect slighter" change that we discussed.

Comment 6

[Botond Ballo [:botond]]

Attachment: Part 5 - Use spring physics for the snap-back animation

This patch implements the use of spring physics to model the snap-back animation. (This is different from the changes to the snap-back animation in Milan's patch.)

These patches, taken together, implement the physics tweaks that we discussed. I haven't put prefs in place for everything yet, I'd like to give Gordon a chance to pay around with this and see if we have the right models.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8433702 [details] [diff] [review]
Part 3 - Fix a short-circuit evaluation bug in OverscrollSnapBackAnimation::Sample

Review of attachment 8433702 [details] [diff] [review]:
-----------------------------------------------------------------

You could just use | instead of ||

Comment 8

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> You could just use | instead of ||

It somehow feels wrong to use a bitwise operator for a logical purpose.

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 1 - Stop fling more quickly when overscrolled

Updated to expose relevant prefs.

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 4 - Make the translation (in the direction of overscroll) more pronounced than the zoom

Updated to expose relevant prefs.

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 5 - Use spring physics for the snap-back animation

Updated to expose relevant prefs.

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 1 - Stop fling more quickly when overscrolled

Fixed a mistake.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8435187 [details] [diff] [review]
Part 1 - Stop fling more quickly when overscrolled

Review of attachment 8435187 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +955,5 @@
>  pref("apz.subframe.enabled", true);
>  
>  // Overscroll-related settings
>  pref("apz.overscroll.enabled", false);
> +pref("apz.overscroll.fling_friction", "0.01"); 

trailing ws

::: gfx/thebes/gfxPrefs.h
@@ +121,5 @@
>    DECL_GFX_PREF(Once, "apz.max_velocity_queue_size",           APZMaxVelocityQueueSize, uint32_t, 5);
>    DECL_GFX_PREF(Live, "apz.min_skate_speed",                   APZMinSkateSpeed, float, 1.0f);
>    DECL_GFX_PREF(Live, "apz.num_paint_duration_samples",        APZNumPaintDurationSamples, int32_t, 3);
>    DECL_GFX_PREF(Live, "apz.overscroll.enabled",                APZOverscrollEnabled, bool, false);
> +  DECL_GFX_PREF(Live, "apz.overscroll.fling_friction",         APZOverscrollFlingFriction, float, 0.01f); 

trailing ws

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8435182 [details] [diff] [review]
Part 4 - Make the translation (in the direction of overscroll) more pronounced than the zoom

Review of attachment 8435182 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1867,5 @@
>    // composited area, respectively.
>  
>    // The maximum proportion of the composition length which can become blank
>    // space along an axis as we overscroll along that axis.
> +  const float CLAMPING = gfxPrefs::APZOverscrollClamping(); 

trailing ws

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8435183 [details] [diff] [review]
Part 5 - Use spring physics for the snap-back animation

Review of attachment 8435183 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/Axis.cpp
@@ +120,5 @@
>  float Axis::GetOverscroll() const {
>    return mOverscroll;
>  }
>  
>  void Axis::StartSnapBack() {

If this is empty we might as well remove it and put it back if we need it later.

@@ +139,5 @@
> +  //   b is a constant that provides damping (friction)
> +  //   v is the velocity of the point at the end of the spring
> +  // See http://gafferongames.com/game-physics/spring-physics/
> +  const float kSpringStiffness = gfxPrefs::APZOverscrollSnapBackSpringStiffness();
> +  const float kSpringFriction = gfxPrefs::APZOverscrollSnapBackSpringFriction();

In the previous patch you used UPPERCASE for const variables. I don't care whether you do kBlah or UPPERCASE but it would be good to make it consistent one way or the other.

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 1 - Stop fling more quickly when overscrolled

Updated to address review comments. Carrying r+.

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 4 - Make the translation (in the direction of overscroll) more pronounced than the zoom

Updated to address review comments. Carrying r+.

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 5 - Use spring physics for the snap-back animation

Updated to address review comments. Carrying r+.

Comment 19

[Botond Ballo [:botond]]

Combined Try push for this bug and bug 1020045: https://tbpl.mozilla.org/?tree=Try&rev=d88bfc86e7f5

Comment 20

[Botond Ballo [:botond]]

Tweaked some prefs based on the latest feedback from Gordon and Milan, and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9315213aa679
https://hg.mozilla.org/integration/mozilla-inbound/rev/e761f600d11a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1495e351283e
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ce4d3cf94e
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d99630aa73

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9315213aa679
https://hg.mozilla.org/mozilla-central/rev/e761f600d11a
https://hg.mozilla.org/mozilla-central/rev/1495e351283e
https://hg.mozilla.org/mozilla-central/rev/05ce4d3cf94e
https://hg.mozilla.org/mozilla-central/rev/77d99630aa73