Closed Bug 1018255 Opened 10 years ago Closed 9 years ago

Followup tweaks to the overscroll effect physics

Categories

(Core :: Panning and Zooming, defect)

32 Branch
x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: milan, Assigned: botond)

References

Details

Attachments

(6 files, 7 obsolete files)

9.30 KB, patch
Details | Diff | Splinter Review
1.52 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.16 KB, patch
kats
: review+
Details | Diff | Splinter Review
8.30 KB, patch
botond
: review+
Details | Diff | Splinter Review
11.73 KB, patch
botond
: review+
Details | Diff | Splinter Review
8.78 KB, patch
botond
: review+
Details | Diff | Splinter Review
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.
This is basically straight out of Milan's patch.
Assignee: nobody → botond
Calling ApplyOverscrollEffect when we are not overscrolled should be harmless, but it's needless work so we should avoid it.
Attachment #8433701 - Flags: review?(bugmail.mozilla)
This is the "making the Z-effect slighter" change that we discussed.
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 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 ||
Attachment #8433702 - Flags: review?(bugmail.mozilla) → review+
Attachment #8433701 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> You could just use | instead of ||

It somehow feels wrong to use a bitwise operator for a logical purpose.
Updated to expose relevant prefs.
Attachment #8433700 - Attachment is obsolete: true
Attachment #8435181 - Flags: review?(bugmail.mozilla)
Updated to expose relevant prefs.
Attachment #8433703 - Attachment is obsolete: true
Attachment #8435182 - Flags: review?(bugmail.mozilla)
Updated to expose relevant prefs.
Attachment #8433707 - Attachment is obsolete: true
Attachment #8435183 - Flags: review?(bugmail.mozilla)
Fixed a mistake.
Attachment #8435181 - Attachment is obsolete: true
Attachment #8435181 - Flags: review?(bugmail.mozilla)
Attachment #8435187 - Flags: review?(bugmail.mozilla)
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
Attachment #8435187 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8435182 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8435183 - Flags: review?(bugmail.mozilla) → review+
Updated to address review comments. Carrying r+.
Attachment #8435187 - Attachment is obsolete: true
Attachment #8435852 - Flags: review+
Updated to address review comments. Carrying r+.
Attachment #8435182 - Attachment is obsolete: true
Attachment #8435854 - Flags: review+
Updated to address review comments. Carrying r+.
Attachment #8435183 - Attachment is obsolete: true
Attachment #8435856 - Flags: review+
Combined Try push for this bug and bug 1020045: https://tbpl.mozilla.org/?tree=Try&rev=d88bfc86e7f5
You need to log in before you can comment on or make changes to this bug.