Make Scrollable's free scroll initial velocity matches that of iOS #60501

LongCatIsLooong · 2020-06-29T07:52:50Z

Description

This PR makes flinging velocity platform-dependent. We can't reliably correct the flinging velocity on iOS in the friction simulation because the velocity estimation is done differently on iOS. This change brings the initial scroll velocity very close to what scrollViewWillEndDragging(_:withVelocity:targetContentOffset:) reports in the scroll_overlay app, also since the final scroll distance (the delta) of a free scroll solely depends on the initial velocity, this in theory should also make the amount the scrollable scrolls during free scrolling match iOS (it's still not as close as it could be because of pixel snapping on iOS and another minor difference in friction simulation, but from the results below it seems this should be good enough).

Test results (using a modified version of the scroll_overlay app):

test run 1

trigger velocity: 4030.787700, target offset: 2135.000000, scrollOffset delta: 2008.500000
2020-06-29 00:50:58.828681-0700 Runner[5062:3670119] flutter: goBallistic with initial velocity: 4030.6899249427292
2020-06-29 00:50:58.828743-0700 Runner[5062:3670119] flutter: start pixel: 258.6666666666667
2020-06-29 00:51:01.815903-0700 Runner[5062:3669876] velocityY: 0.000000
2020-06-29 00:51:01.849800-0700 Runner[5062:3670119] flutter: becomes Idle. Pixels: 2266.561898725746

In this run scrollViewWillEndDragging(_:withVelocity:targetContentOffset:) reported an initial velocity of 4030.787700 and a scroll distance of 2008.500000, while flutter reported 4030.6899249427292 and 2266.561898725746-258.6666666666667= 2007.89523206 respectively.

test run 2

2020-06-29 01:02:29.504894-0700 Runner[5116:3673262] trigger velocity: 6895.866228, target offset: 3816.500000, scrollOffset delta: 3439.500000
2020-06-29 01:02:29.531646-0700 Runner[5116:3673481] flutter: goBallistic with initial velocity: 6895.867438846153
2020-06-29 01:02:29.531713-0700 Runner[5116:3673481] flutter: start pixel: 455.0
2020-06-29 01:02:32.778637-0700 Runner[5116:3673262] velocityY: 0.000000
2020-06-29 01:02:32.812310-0700 Runner[5116:3673481] flutter: becomes Idle. Pixels: 3893.693637211801

this time it's 6895.866228 vs 6895.867438846153 and 3439.500000 vs 3438.69363721.

Related Issues

Fixes #11772

Related PR

adopted some changes from #59623 for better results.

Tests

I added the following tests:

FreeScrollStartVelocityTracker.getVelocity throws when no points, etc.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

passsy · 2020-07-09T00:28:16Z

Good job! I tested it with the scroll_overlay PR#2 project including the changes in #59623.

xster

The general approach (e.g. final Offset estimatedVelocity = _previousVelocityAt(-2) * 0.6 + _previousVelocityAt(-1) * 0.35 + _previousVelocityAt(0) * 0.05;) lgtm

xster · 2020-07-09T18:02:17Z

packages/flutter/lib/src/gestures/monodrag.dart

Since this will be the first place a user is exposed to the term "velocity tracker", describe what it is or reference another doc

xster · 2020-07-09T18:06:16Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

Maybe briefly describe what's special about iOS scroll velocity to help bootstrap the next maintainer

It's still not too clear what the difference is and why this needs to be a separate class rather than changing VelocityTracker.

You mean the difference in terms of the strategies used? Does

/// A [VelocityTracker] subclass that employs a weighted average strategy to
/// estimate the velocity of the associated pointer, to match iOS scroll view's
/// fling velocity calculation.

sound better?

As opposed to what though. I was looking for a text that saves the next maintainer from seeing thing as redundant, try to merge the 2 classes into one, spend 2 hours and then realized that the approach for the 2 are conceptually different (which the maintainer could have discovered in 10s by reading this text)

Added a simple comparison of the 2 velocity tracker classes.

xster · 2020-07-09T18:06:55Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

Why this name? Either somehow describe the "free scroll start" nature of the velocity tracking in the class doc or just call this ios velocity tracker.

I thought we wanted to avoid having iOS in the name? Would be nice to know that I was mistaken.

xster · 2020-07-09T18:07:38Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

Not sure I understood this comment

also seems like you're always using exactly 3 samples no?

"4 _PointAtTime samples" is what I meant

Horizontal/VerticalDragGestureRecognizer.isFlingGesture has a threshold for VelocityEstimate.offset for it to be a fling:
https://api.flutter.dev/flutter/gestures/VerticalDragGestureRecognizer/isFlingGesture.html

Initially I used 4 samples and that broke some tests so I had to record more points. Will point to VerticalDragGestureRecognizer.isFlingGesture in the comments.

xster · 2020-07-09T18:12:28Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

sounds stronger than what the code seems to be doing. You're just weighing the initial velocity more rather than matching the initial velocity no?

I was trying to say this is the velocity of the scroll view not that of the gesture. Reworded.

xster · 2020-07-09T18:14:15Z

packages/flutter/lib/src/widgets/scroll_configuration.dart

"initial free scrolling speed" is a bit vague

xster · 2020-07-09T18:15:47Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

Please add to the velocity_tracker_bench microbenchmark too

goderbauer · 2020-07-22T21:38:06Z

fly-by comment: The analyzer is complaining.

xster

sorry I was out last few weeks. Thanks for the clean up! LGTM

xster · 2020-07-29T17:15:20Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

It's still not too clear what the difference is and why this needs to be a separate class rather than changing VelocityTracker.

goderbauer · 2020-07-29T21:44:00Z

(PR triage): looks like some nested scrollview tests are failing.

LongCatIsLooong · 2020-07-30T01:36:40Z

Had to use microseconds instead of milliseconds in flingTo to ensure the velocity samples' timestamps are monotonically increasing. Tests should be passing now.

LongCatIsLooong · 2020-07-31T18:34:37Z

@xster @goderbauer should be ready for review now.

xster · 2020-08-03T17:53:26Z

packages/flutter/lib/src/gestures/velocity_tracker.dart

<optional at this point, up to your judgement> can this just be done by composition? i.e. most platforms use a VelocityTracker with a VelocitySampleScorer of [1, 1, 1, 1, ...] and iOS uses a VelocityTracker with a VelocitySampleScorere of [8, 5, 2, 1] or something?

You mean the weighted average can be expressed in terms of a least-squares estimator? Not sure how I can achieve that, could you elaborate?

xster · 2020-08-03T17:54:04Z

LGTM. Up to you at this point if you want to use composition or just have 2 classes.

fluttergithubbot · 2020-08-08T04:26:02Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.
The status or check suite web_smoke_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

goderbauer

LGTM

goderbauer · 2020-08-12T16:27:30Z

packages/flutter/lib/src/gestures/monodrag.dart

    this.dragStartBehavior = DragStartBehavior.start,
+    this.velocityTrackerBuilder = _defaultBuilder,
  }) : assert(dragStartBehavior != null),
+       //this.velocityTrackerBuilder = velocityTrackerBuilder


Remove this?

…lutter#60501)

fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jun 29, 2020

googlebot added the cla: yes label Jun 29, 2020

LongCatIsLooong added the platform-ios iOS applications specifically label Jun 29, 2020

LongCatIsLooong requested a review from xster June 29, 2020 17:53

LongCatIsLooong marked this pull request as ready for review June 29, 2020 17:54

LongCatIsLooong added f: scrolling Viewports, list views, slivers, etc. and removed work in progress; do not review labels Jun 29, 2020

goderbauer mentioned this pull request Jul 8, 2020

Reduce iOS scroll damping for lists with differently sized items #59623

Merged

9 tasks

LongCatIsLooong requested a review from goderbauer July 9, 2020 00:31

xster reviewed Jul 9, 2020

View reviewed changes

LongCatIsLooong force-pushed the ios-scroll-velocity branch from 99b303a to f793210 Compare July 10, 2020 08:59

LongCatIsLooong requested a review from xster July 17, 2020 07:03

xster reviewed Jul 29, 2020

View reviewed changes

LongCatIsLooong force-pushed the ios-scroll-velocity branch 3 times, most recently from 8db3b82 to 0dcec67 Compare July 30, 2020 01:17

LongCatIsLooong force-pushed the ios-scroll-velocity branch from a8d0d75 to b8c6c9c Compare August 1, 2020 02:30

xster reviewed Aug 3, 2020

View reviewed changes

reapply patch onto master

97e257b

LongCatIsLooong force-pushed the ios-scroll-velocity branch from 3820762 to 97e257b Compare August 7, 2020 17:27

Merge remote-tracking branch 'upstream/master' into ios-scroll-velocity

121ab5d

LongCatIsLooong added the waiting for tree to go green label Aug 8, 2020

fluttergithubbot removed the waiting for tree to go green label Aug 8, 2020

Piinks added the a: fidelity Matching the OEM platforms better label Aug 10, 2020

LongCatIsLooong added 4 commits August 11, 2020 12:52

Update monodrag.dart

6868a02

Update velocity_tracker_test.dart

f4ec399

make it non-null

e3b66ae

assert in preparation of nnbd

3523baf

goderbauer approved these changes Aug 12, 2020

View reviewed changes

review

03df3f0

LongCatIsLooong added the waiting for tree to go green label Aug 12, 2020

fluttergithubbot merged commit cea055e into flutter:master Aug 12, 2020

LongCatIsLooong deleted the ios-scroll-velocity branch August 12, 2020 20:36

LongCatIsLooong mentioned this pull request Aug 18, 2020

Restore the original name of the velocity tracker microbenchmark #64060

Merged

9 tasks

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Make Scrollable's free scroll initial velocity matches that of iOS (f…

ad7949f

…lutter#60501)

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021

Make Scrollable's free scroll initial velocity matches that of iOS #60501

Make Scrollable's free scroll initial velocity matches that of iOS #60501

Uh oh!

Conversation

LongCatIsLooong commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test results (using a modified version of the scroll_overlay app):

Related Issues

Related PR

Tests

Checklist

Breaking Change

Uh oh!

passsy commented Jul 9, 2020

Uh oh!

xster left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jul 22, 2020

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jul 29, 2020

Uh oh!

LongCatIsLooong commented Jul 30, 2020

Uh oh!

LongCatIsLooong commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Aug 3, 2020

Uh oh!

fluttergithubbot commented Aug 8, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Jun 29, 2020 •

edited

Loading

xster left a comment •

edited

Loading

LongCatIsLooong Jul 29, 2020 •

edited

Loading