1066888 - Overscroll physics improvements

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Gordon Brander :gordonb]

Given the new overscroll effect (bug 1057578), we would like to slate some improvements to the effect that rely on changes to the underlying physics.

Here's how the current physics work (I think):

* User flings scrollable area, resulting in a skate.
* Left-over energy causes the scroll to skate past the natural scroll bounds.
* A special friction value is applied to the skate once it passes the threshold of normal scroll bounds.
* When skate reaches close to 0 velocity, a spring is applied, causing it to snap back to natural scroll bounds.

The odd balance between linear damping (friction) and exponential Hooke force (spring) means that it's difficult to get an effect that works well for both high-velocity flings and low-velocity "bumps". Since we switched to a new stretch effect, this imbalance causes a "gooey" look.

Here's the proposed change:

* Instead of applying a special friction value, maintain the same friction throughout
* When a skate has extra velocity, apply a spring to halt it and a spring to snap it back. These would probably be the same spring, but maybe having 2 springs would feel better? Not sure.

This would also allow us to add a bit of oscillation, a natural physical characteristic for a stretchy material.

The hope is to get this into a 2.2 release.

Comment 1

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

Not sure if we have a set of flags or meta-bug tracking 2.2 (or potential 2.2) issues yet. If we do, we should apply that to this bug.

Comment 2

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

We had a discussion with Gordon today about this to get some more details; Botond is working on this now.

Comment 3

[Botond Ballo [:botond]]

Attachment: Unify the physics for overscroll-fling and snap-back

This patch unifies the overscroll phase of a fling animation and the overscroll snap-back animation into a single overscroll animation which is governed by spring physics, and which allows for oscillations.

It's a bit hacky and it definitely needs some tuning, but it does seem to work.

Notes for Kats:

  - To implement the support for oscillations I had to introduce
    a notion of "underscroll". 

    Previously, mOverscroll < 0 meant the axis was overscrolled at
    its origin (i.e. at the top or left), while mOverscroll > 0 meant 
    the axis was overscrolled at its composition end (i.e. at the
    bottom or right). The snap-back animation would reduce mOverscroll
    to 0, and then stop.

    Now that we allow the snap-back to "overshoot" and oscillate,
    mOverscroll can change sign over the course of the animation.
    For example, suppose we're overscrolled at the top, so
    initially mOverscroll < 0. As we oscillate, we can get
    mOverscroll > 0, but we need to distinguish this case from
    the case where mOverscroll > 0 because we're overscrolling
    at the bottom. To allow us to distinguish, I introduced
    the mInUnderscroll flag.

    It's probably helpful to read the table in Axis.h that describes
    what the combinations of mOverscroll and mInUnderscroll mean
    before looking at the changes to Axis::SampleOverscrollAnimation 
    or APZC::GetOverscrollTransform.

    This seems a bit hacky, but it's the best I could come up with
    at the moment. An alternative I considered was having two
    overscroll values, mOverscrollAtOrigin and mOverscrollAtEnd,
    but that seemed to require a lot of duplication of code.
    Suggestions for improvement are always welcome.

  - To apply the spring friction to the velocity in an
    exponential way similar to the fling friction, I had to
    separate out the change to the velocity as a result of the
    spring force, and the change as a result of the friction.
    Please let me know if the outcome is reasonable; it's
    possible I missed something here.

  - We should update our gtests in accordance with these changes.
    Up to you if we should wait on that before landing this.

Notes for Kats and Gordon:

  - I kept the "spring friction" and the "fling friction" as
    separate prefs because I found they needed to have disparate
    values to get reasonable behaviour. With the same value,
    we'd either get fling stopping very quickly (high value),
    or too many oscillations (low value).

    This makes sense to me intuitively: we want flings to
    "skate" relatively smoothly (low friction), while we want
    the spring to oscillate few times (higher friction).

    I know this is not fully in line with "using the same
    friction throughout"; it's possible that I'm missing
    something.

Notes for Gordon:

  - I played around with the prefs enough to bring them into
    the realm of reasonability, but you'll doubtless want
    to tweak it more. Here's a summary of the prefs:

      - apz.overscroll.spring_stiffness controls the spring force,
        same as before.

      - apz.overscroll.spring_friction controls the spring friction,
        same as before. I kept this separate from apz.fling_friction
        as explained above.

      - apz.overscroll.stop_distance_threshold and
        apz.overscroll.stop_velocity_threshold are the thresholds
        such that when we go below both, we stop oscillating.

      - I removed the "spring mass" pref, because it was redundant.
        A mass of 'm' can be achieved by dividing both the
        spring_stiffness and the spring_friction by 'm'.

Comment 4

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

Comment on attachment 8513958 [details] [diff] [review]
Unify the physics for overscroll-fling and snap-back

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

LGTM!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +449,5 @@
> +    // Drop any velocity on axes where we don't have room to scroll anyways.
> +    // This ensures that we don't take the 'overscroll' path in Sample()
> +    // on account of one axis which can't scroll having a velocity.
> +    ReentrantMonitorAutoEnter lock(mApzc.mMonitor);
> +    {

The lock should be inside the { I think. Or you don't need the extra {}

Comment 5

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

Botond, seems like at least some of comment 3 should end up in the code documentation (don't know which class, but you'd know), using doxygen formatting...

Comment 6

[Botond Ballo [:botond]]

Attachment: Unify the physics for overscroll-fling and snap-back

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> @@ +449,5 @@
> > +    // Drop any velocity on axes where we don't have room to scroll anyways.
> > +    // This ensures that we don't take the 'overscroll' path in Sample()
> > +    // on account of one axis which can't scroll having a velocity.
> > +    ReentrantMonitorAutoEnter lock(mApzc.mMonitor);
> > +    {
> 
> The lock should be inside the { I think. Or you don't need the extra {}

Good catch! Fixed, carrying r+.

Comment 7

[Botond Ballo [:botond]]

Attachment: Update gtests

Had to adjust the overscrolling gtests to keep them passing.

Comment 8

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3635174edf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f14db066b4

Comment 9

[Botond Ballo [:botond]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> Botond, seems like at least some of comment 3 should end up in the code
> documentation (don't know which class, but you'd know), using doxygen
> formatting...

Most of it's already documented in comments, though not in doxygen formatting. I'll expand the comments a bit an doxify them in a follow-up commit.

Comment 10

[Botond Ballo [:botond]]

Attachment: Documentation tweaks

This patch doxifies the explanations of GetOverscroll() and IsInUnderscroll() in the new physics model, and adds a bit of explanation about the prefs.

I didn't doxify the documentation of the prefs because they're not attached to any code entity, so I'm not sure what doxygen would do with them; I'll file a follow-up bug for this.

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8515135 [details] [diff] [review]
Documentation tweaks

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

::: gfx/layers/apz/src/Axis.h
@@ +113,5 @@
> +   *-------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------
> +   *       negative    |      false        | The axis is overscrolled at its origin. A stretch is applied with the content fixed in place at the origin.
> +   *       positive    |      false        | The axis is overscrolled at its composition end. A stretch is applied with the content fixed in place at the composition end.
> +   *       positive    |      true         | The axis is underscrolled at its origin. A compression is applied with the content fixed in place at the origin.
> +   *       negative    |      true         | The axis is underscrolled at its composition end. A compression is applied with the content fixed in place at the composition end.

Note: the reason for the long lines here is that markdown tables are required to be one line per row (http://stackoverflow.com/questions/19950648/how-to-write-lists-inside-a-markdown-table/19953345#19953345).

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I didn't doxify the documentation of the prefs because they're not attached
> to any code entity, so I'm not sure what doxygen would do with them; I'll
> file a follow-up bug for this.

Filed bug 1092262.

Comment 13

[Botond Ballo [:botond]]

Landed the doc patch with DONTBUILD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/347dcfc5d36e

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c3635174edf
https://hg.mozilla.org/mozilla-central/rev/a0f14db066b4

Comment 15

[Chris Lord [:cwiiis]]

Just leaving this comment here, will follow up on it properly on Monday (what with comments on a closed bug not being all that helpful), but what I immediately notice with this new physics is that's it's really annoying having to wait for the bounce animation to finish before you can scroll.

You should immediately be able to scroll in the direction opposite to the overscroll, imo.

Comment 16

[Botond Ballo [:botond]]

Hey Gordon, the new physics code has now landed, and is ready for you to play around with it. Please see my notes addressed to you in comment 3.

Let me know what you think :)

Comment 17

[Botond Ballo [:botond]]

(In reply to Chris Lord [:cwiiis] from comment #15)
> Just leaving this comment here, will follow up on it properly on Monday
> (what with comments on a closed bug not being all that helpful), but what I
> immediately notice with this new physics is that's it's really annoying
> having to wait for the bounce animation to finish before you can scroll.
> 
> You should immediately be able to scroll in the direction opposite to the
> overscroll, imo.

The animation is not finalized yet; I implemented the physics model, but Gordon is still going to tweak the parameters. One possible outcome of this tweaking is that the overall duration of the animation is reduced, and this issue becomes less noticeable.

I suggest we have another look once the parameters have been tweaked; if this is still an issue, we can consider handling input events in an overscrolled state.

Comment 18

[Mason Chang [Inactive] [:mchang]]

(In reply to Chris Lord [:cwiiis] from comment #15)
> Just leaving this comment here, will follow up on it properly on Monday
> (what with comments on a closed bug not being all that helpful), but what I
> immediately notice with this new physics is that's it's really annoying
> having to wait for the bounce animation to finish before you can scroll.
> 
> You should immediately be able to scroll in the direction opposite to the
> overscroll, imo.

See bug 1042103.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/347dcfc5d36e

Comment 20

[Gordon Brander :gordonb]

I'm working on tuning the overscroll settings to get the right feel. In the mean time, here are some "pretty good" settings that will cut down on the length of the overscroll animation until we fix/look at bug 1042103.

    user_pref("apz.overscroll.spring_friction", "0.02");
    user_pref("apz.overscroll.spring_stiffness", "0.002");
    user_pref("apz.overscroll.stretch_factor", "0.15");

Note I will probably revise these further, but I feel like they get us closer to the intended effect. If we're comfortable with landing multiple pref changes, go for it.

Comment 21

[Gordon Brander :gordonb]

Also, I'm seeing a bug with the overscroll effect:

The "tug" effect works correctly:

- Open scrollable view (email is a good example because it's easy to spot)
- Scroll to top, then tug scrollable view "beyond" top and release.
- Scrolling layer stretches, then bounces. Top of scrolling layer remains flush with edge of scrollable viewport.

But the fling overscroll effect does not:

- Pan down a ways from top
- Scroll quickly to gain momentum
- Bug: scrolling layer skates, hits top. Top of scrolling layer *does not* remain flush with top of scrollable viewport. Instead, a white gap is revealed during animation.

Expected:
Animations for skate and tug should match.

Comment 22

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

We should use a follow-up bug for the issue you describe above, otherwise it'll get pretty confusing.

Comment 23

[Gordon Brander :gordonb]

Filed bug 1097196.

Comment 24

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

Thanks!

Comment 25

[Gordon Brander :gordonb]

Let's land the following prefs for now:

    user_pref("apz.overscroll.spring_friction", "0.02");
    user_pref("apz.overscroll.spring_stiffness", "0.002");
    user_pref("apz.overscroll.stretch_factor", "0.15");

These prefs represent the feel we are going for: bouncy but not gooey. This assumes we can land bug 1042103 so that the slightly longer bounciness doesn't interfere with touch events.

If we can't land bug 1042103, then "plan B" is to revisit these settings and "tune down" the spring, making it less bouncy and making the animation duration shorter.

Comment 26

[Botond Ballo [:botond]]

Attachment: Tweak prefs as per comment 20

Comment 27

[Botond Ballo [:botond]]

Attachment: Only enter an overscroll animation if the overscrolling pref is enabled

Couple of small follow-ups.

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8523268 [details] [diff] [review]
Only enter an overscroll animation if the overscrolling pref is enabled

:dvander posted the same patch in a separate bug, bug 1099442. Let's go with that.

Comment 29

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25dfe9824b6f

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25dfe9824b6f

Comment 31

[Chris Lord [:cwiiis]]

Now that Bug 1100680 and bug 1042103 have been fixed, should we re-evaluate the default values?

Comment 32

[Gordon Brander :gordonb]

Yes we should re-tune the overscroll physics.

I attempted to re-tune these values today, but something in the edit-prefs.sh script seems to have changed and I get the following error:

    Pulling preferences: /data/b2g/mozilla/*.default/prefs.js
    remote object '/data/b2g/mozilla/*.default/prefs.js' does not exist

Kats, sorry to bother you with this, but do you know what might have changed?

Comment 33

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

As far as I know the script should still work; I justed it just the other day. Can you run "adb shell ls -l /data/b2g/mozilla" and paste the output?

Comment 34

[Gordon Brander :gordonb]

The new set of prefs, prioritized for bounciness and fun:

user_pref("apz.fling_friction", "0.0019");
user_pref("apz.overscroll.spring_friction", "0.015");
user_pref("apz.overscroll.spring_stiffness", "0.0018");
user_pref("apz.overscroll.stretch_factor", "0.35");

Jaime or Francis, could you give me a second opinion on this? How does it feel?

Comment 35

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

We'll need a new bug to land these changes, so I went ahead and filed bug 1136311 for it. Feel free to comment here or there; once we're good to go I'll use that bug to land the patch.

Comment 36

[Francis Djabri [:djabber]]

Hi Gordon, this feels good to me. The springiness feels a bit tighter and more responsive than before.