Update ClampingScrollSimulation to better match Android #77497

grouma · 2021-03-07T04:37:43Z

I've noticed that the Android Flutter scroll logic appears to be slightly off. After a scroll slows down to a sufficient amount, items appear to "snap" into place. It's as if the inertial scrolling clamps down and micro movements are not allowed. Note the difference is very subtle but present across Flutter apps, e.g. Stadia, gPay etc. It appears that the underlying implementation is an approximation of the Android logic which may explain the behavior.

Since this has been driving me mad I have decided to translate the 2-dimensional logic from Scroller.java to 1 dimension and reuse in Flutter. The behavior should now be identical assuming equivalent DPI and friction settings.

flutter-dashboard · 2021-03-07T04:37:45Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

grouma · 2021-03-08T22:08:16Z

I created a diff between the output (x, dx) with the new logic and existing logic. The data on the left is the new logic. You can see the velocity slows down to 0 as the scroll comes to a completion. The existing logic on the other hand slows down and then starts to ramp back up at the very end, likely due to the cubic function.

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

Hixie · 2021-03-08T22:14:55Z

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

+    return 9.80665 *
+        39.37 *
+        friction *
+        1.0 * // Assume display density of 1.0.


how much of a difference does this make? if it matters, we should be able to find a way to get the pixel density here...

There is an API in Android but I couldn't find one in Flutter. We have deviceRatio which is slightly different. For what it's worth, this hasn't changed from the previous logic, just the formula is now explicit.

although, actually, maybe 1.0 is the right number here? since we operate in logical pixels not physical pixels in the framework. In which case the comment should reflect that -- it's not an assumption, it's a reality.

Hixie · 2021-03-08T22:16:18Z

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

+      _splineTime[i] = coef * ((1.0 - y) * p1 + y * p2) + y * y * y;
+    }
+    _splinePosition[_nbSamples] = _splineTime[_nbSamples] = 1.0;
+    _isInitialized = true;


can we do this statically and just fill in the data? (rather than doing it at runtime on first use)

Do you mean statically through code, like in the JAVA implementation? If so, no I don't think Dart offers anything similar hence the somewhat awkward implementation.

If you actually meant writing an external program to create the static data, we could do that. It would obviously not be as easily configured liked the Android implementation.

There is no "static initialization" time in Dart. All code that runs should do so as a result of invoking main().

The options here are either to compute it on first use, or to add a hook somewhere in the framework to cause it to be computed early.

I meant offline (as in have a program in the dev/ directory that runs this code and generates the constants and you can just paste them into this file). Looks like this code isn't configurable by developers, so that wouldn't reduce the overall configurability for our developers, but would be more efficient for users.

Hixie · 2021-03-08T22:17:02Z

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

  /// Creates a scroll physics simulation that matches Android scrolling.
  ClampingScrollSimulation({
    required this.position,
    required this.velocity,


is the "some simplifications have been made" bit still true?

Yes. The logic in Scroller.java is 2-dimensional and has logic for non "fling" scrolls so there are simplifications still.

Hixie · 2021-03-08T22:17:21Z

what's the performance like?

Hixie · 2021-03-08T22:19:08Z

I'll defer to @HansMuller or @goderbauer for a detailed review.
(review might be easier once the spurious reformatting is fixed though)

grouma · 2021-03-08T23:51:59Z

Removed the unnecessary diffs.

what's the performance like?

Performance should be more or less equivalent after the spline data is initialized as there are roughly the same number of operations. I doubt the initialization has a measurable impact on the performance but it'd be good to know how best to verify that assumption.

grouma · 2021-03-09T04:39:44Z

packages/flutter/test/widgets/scroll_simulation_test.dart

  });
+
+
+  test('ClampingScrollSimulation velocity eventually reaches zero', () {


Note these new tests fail with the old logic. As noted in the my previous comment, velocity ramps back up near the end of the simulation.

goderbauer

Looks like some existing tests are failing. Did you take a look at those?

goderbauer · 2021-03-09T01:04:10Z

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

 import 'dart:math' as math;

 import 'package:flutter/foundation.dart';
+import 'package:flutter/material.dart';


files in the widgets directory can't import material.

I think this was an artifact of getting the deviceRatio in a pervious implementation. I'm surprised there isn't an unused import lint or issue.

(material reexports most of widgets so it doesn't get flagged as unused. redundant imports is a lint we haven't deployed yet though I understand work is ongoing.)

packages/flutter/test/widgets/scroll_simulation_test.dart

grouma · 2021-03-11T21:49:36Z

This should be ready for review PTAL.

goderbauer · 2021-03-11T22:25:52Z

Looks like there are still some test failures.

goderbauer · 2021-03-12T19:51:14Z

dev/tools/generate_android_spline_data.dart

+const double _endTension = 1.0;
+const double _inflexion = 0.35;
+
+// Generated spline data used in ClampingScrollSimulation.


Can you add a little more context to this doc? E.g. where's the math in here coming from? What should be done with the result after changing anything here?

goderbauer · 2021-03-12T19:56:06Z

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

+  }
+}
+
+class _NBSample {


what does the NB stand for? Maybe document or just spell it out?

To be honest I'm not sure. I think it might just happen to be the variable name of choice for the underlying math equation that the original developer likely referenced. I kept it here as I wanted it to be immediately obvious how this logic is translated from Scroller.java.

goderbauer · 2021-03-12T19:58:26Z

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

+
+  static const int _nbSamples = 100;
+
+  // Generated from dev/tools/generate_android_spline_data.dart.


Ideally, we would have some kind of a test to verify that the numbers reproduced here match the result of that calculation to ensure that we update things here if the calculation math ever changes.

A test may be helpful but I don't think it provides a ton of value. Updating the comment in the file as you suggested should make things much more clear.

goderbauer · 2021-03-12T20:01:26Z

Overall, this looks good. Thanks for taking the time to update it! I am going to trigger a google3 presubmit to see if it breaks any tests there (like it broke some of our framework tests).

…oid-scroll

goderbauer

LGTM

Piinks · 2021-09-09T23:14:35Z

This has caused a bad regression in Android ballistic scrolling. Described in #83632 with good repros, flinging hard and fast on the scrollable causes the ballistics activity to run off much further than it should.

There are some mentions of this in #16371 as well.

Piinks · 2021-09-09T23:19:16Z

packages/flutter/test/widgets/list_wheel_scroll_view_test.dart

        find.byType(ListWheelScrollView),
        const Offset(0.0, -50.0),
-        800.0,
+        100.0,


@grouma Can you recall why this fling needed to be reduced so much? This is probably the same regression that has been reported for flings going way too far.

Unfortunately I do not. I recall that a number of tests had to have their parameters updated to keep the "goldens" valid. The main intent behind this change was to fix the jitter users would experience as the scroll slowed down. I added a test for that invalid behavior, see ClampingScrollSimulation velocity eventually reaches zero.

Piinks · 2021-09-09T23:30:54Z

Chatted with @goderbauer and we are going to revert this, unfortunately it has been in for a while so it can't be done automatically. I should be able to do this manually tomorrow and add some extra tests while I am at it.

[WIP] Update Android Scroll Physics to better match Android

b8921c3

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 7, 2021

google-cla bot added the cla: yes label Mar 7, 2021

ensure initial conditions are stable

0a00fa2

grouma changed the title ~~[WIP] Update Android Scroll Physics to better match Android~~ Update Android Scroll Physics to better match Android Mar 7, 2021

Merge branch 'master' into android-scroll

3d53ec1

grouma requested review from HansMuller and Hixie March 7, 2021 05:24

grouma changed the title ~~Update Android Scroll Physics to better match Android~~ Update ClampingScrollSimulation to better match Android Mar 7, 2021

Hixie reviewed Mar 8, 2021

View reviewed changes

packages/flutter/lib/src/widgets/scroll_simulation.dart Outdated Show resolved Hide resolved

Hixie reviewed Mar 8, 2021

View reviewed changes

grouma added 2 commits March 8, 2021 15:02

undo dartfmt changes and update comment

109ce6b

remove import change

9b14032

grouma requested a review from goderbauer March 8, 2021 23:54

grouma added 2 commits March 8, 2021 20:27

Merge branch 'master' into android-scroll

3e522b8

add final velocity test

913d5d1

grouma commented Mar 9, 2021

View reviewed changes

goderbauer reviewed Mar 10, 2021

View reviewed changes

grouma added 2 commits March 10, 2021 12:52

fix unused import and new line

8f1fc0b

Merge branch 'master' into android-scroll

9bea1d8

fix hardcoded scroll constants in tests

1e22b1f

grouma added 4 commits March 11, 2021 16:49

fix integration test

bbe9aa1

generate spline data ahead of time

fa8a255

revert dartfmt changes

5d78a5b

Update license header

81fd6e3

goderbauer reviewed Mar 12, 2021

View reviewed changes

grouma added 2 commits March 12, 2021 12:19

update comment

4839e13

Merge branch 'android-scroll' of github.com:flutter/flutter into andr…

c0f1625

…oid-scroll

goderbauer approved these changes Mar 12, 2021

View reviewed changes

goderbauer added the waiting for tree to go green label Mar 12, 2021

fluttergithubbot merged commit 3f62915 into master Mar 13, 2021

fluttergithubbot deleted the android-scroll branch March 13, 2021 00:23

nt4f04uNd mentioned this pull request Mar 29, 2021

Android ListView.builder scroll doesn't end smoothly #41060

Closed

nt4f04uNd mentioned this pull request Aug 28, 2021

Android scroll speed deceleration rate doesn't match native #16371

Closed

Piinks mentioned this pull request Sep 9, 2021

Scroll view regains velocity as it scrolls #83632

Closed

Piinks reviewed Sep 9, 2021

View reviewed changes

Piinks mentioned this pull request Sep 10, 2021

Revert clamping scroll simulation changes #89885

Merged

8 tasks

grouma mentioned this pull request Sep 14, 2021

[IN PROGRESS] Revert "Revert clamping scroll simulation changes" #89896

Closed

grouma mentioned this pull request Oct 23, 2021

Adjust ClampingScrollSimulation Parameters #92338

Closed

nt4f04uNd mentioned this pull request Dec 18, 2021

Ballistic simulations may be created too frequently #11599

Open

MxSoul mentioned this pull request Jan 12, 2022

Fix restart ballistic animation while child size change #96512

Closed

8 tasks

MxSoul mentioned this pull request Mar 25, 2022

Update animation simulation to fix restart ballistic animation #100133

Closed

8 tasks

Piinks mentioned this pull request Jul 15, 2022

Update Ballistic animation & ClampingScrollSimulation #107735

Merged

8 tasks

gnprice mentioned this pull request Feb 10, 2023

Allow scroll physics that doesn't obey physical metaphor #120480

Closed

8 tasks

		});


		test('ClampingScrollSimulation velocity eventually reaches zero', () {


		static const int _nbSamples = 100;

		// Generated from dev/tools/generate_android_spline_data.dart.

Update ClampingScrollSimulation to better match Android #77497

Update ClampingScrollSimulation to better match Android #77497

Uh oh!

Conversation

grouma commented Mar 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Mar 7, 2021

Uh oh!

grouma commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

grouma Mar 8, 2021 • 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!

Hixie commented Mar 8, 2021

Uh oh!

Hixie commented Mar 8, 2021

Uh oh!

grouma commented Mar 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

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!

Uh oh!

grouma commented Mar 11, 2021

Uh oh!

goderbauer commented Mar 11, 2021

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 Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

grouma commented Mar 7, 2021 •

edited

Loading

grouma commented Mar 8, 2021 •

edited

Loading

grouma Mar 8, 2021 •

edited

Loading

goderbauer commented Mar 12, 2021 •

edited

Loading

Piinks commented Sep 9, 2021 •

edited

Loading