Closed Bug 1066888 Opened 10 years ago Closed 10 years ago

Overscroll physics improvements

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
feature-b2g 2.2+
Tracking Status
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: gbrander, Assigned: botond)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
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.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
feature-b2g: --- → 2.2?
feature-b2g: 2.2? → 2.2+
We had a discussion with Gordon today about this to get some more details; Botond is working on this now.
Assignee: nobody → botond
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'.
Attachment #8513958 - Flags: review?(bugmail.mozilla)
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 {}
Attachment #8513958 - Flags: review?(bugmail.mozilla) → review+
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...
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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+.
Attachment #8513958 - Attachment is obsolete: true
Attachment #8515051 - Flags: review+
Attached patch Update gtestsSplinter Review
Had to adjust the overscrolling gtests to keep them passing.
Attachment #8515052 - Flags: review?(bugmail.mozilla)
Attachment #8515052 - Flags: review?(bugmail.mozilla) → review+
(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.
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.
Attachment #8515135 - Flags: review?(bugmail.mozilla)
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).
(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.
Attachment #8515135 - Flags: review?(bugmail.mozilla) → review+
Landed the doc patch with DONTBUILD:

https://hg.mozilla.org/integration/mozilla-inbound/rev/347dcfc5d36e
https://hg.mozilla.org/mozilla-central/rev/6c3635174edf
https://hg.mozilla.org/mozilla-central/rev/a0f14db066b4
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.
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 :)
Flags: needinfo?(gbrander)
(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.
(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.
See Also: → 1042103
No longer blocks: 1093298
Depends on: 1093298
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.
Flags: needinfo?(gbrander) → needinfo?(botond)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We should use a follow-up bug for the issue you describe above, otherwise it'll get pretty confusing.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Filed bug 1097196.
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.
Flags: needinfo?(botond)
Attachment #8523267 - Flags: review?(bugmail.mozilla)
Couple of small follow-ups.
Attachment #8523268 - Flags: review?(bugmail.mozilla)
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.
Attachment #8523268 - Attachment is obsolete: true
Attachment #8523268 - Flags: review?(bugmail.mozilla)
Attachment #8523267 - Flags: review?(bugmail.mozilla) → review+
Now that Bug 1100680 and bug 1042103 have been fixed, should we re-evaluate the default values?
Flags: needinfo?(gbrander)
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?
Flags: needinfo?(gbrander) → needinfo?(bugmail.mozilla)
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?
Flags: needinfo?(bugmail.mozilla) → needinfo?(gbrander)
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?
Flags: needinfo?(jachen)
Flags: needinfo?(gbrander)
Flags: needinfo?(fdjabri)
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.
Hi Gordon, this feels good to me. The springiness feels a bit tighter and more responsive than before.
Flags: needinfo?(jachen)
Flags: needinfo?(fdjabri)
You need to log in before you can comment on or make changes to this bug.