Update animation simulation to fix restart ballistic animation by MxSoul · Pull Request #100133 · flutter/flutter

MxSoul · 2022-03-15T12:44:04Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-03-15T12:44:08Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

MxSoul · 2022-03-15T17:51:55Z

@Piinks
I tried a new method to solve the problem.
I don't know if this is the right plan, Maybe you could take a look. If you think this solution is feasible, I will complete the documentation and fix the test.

Here's what I think:

In the previous method:

I need to update the BouncingSimulator/ClampingSimulator properties.
I need to call a specific method in ClampingSimulator/BouncingSimulator.
I have to use type checking to get me to call this method.

In the new method:

My purpose is to not restart the sliding animation
Maybe we can use the new Simulator, just make sure it is generated with the initial position and initial velocity.
Then we update the new Simulator to the animation.

Piinks

Hey @MxSoul thanks for re-working this! This looks like a really nice approach. Do you have tests that validate the expected behavior?

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

…imation

MxSoul · 2022-03-25T08:33:18Z

@Piinks
Yes, Everything is as expected. I record a video and add test. Docs also update.
Please let me know if need me to do anything else. 😄
I record clamping performance only. Bouncing performs consistently before and after modification(validate the expected)

Before	After

Record By Pixel 3a XL

We could see sliding damping is not as great as before. The animation of clamping is same as without size change.
Although the problem of excessive damping has been fixed, there is still a certain gap between the sliding of Clamping and the native curve of Android. The "snap" which described in #77497 has reappeared. So I will try to reland it after this PR merge

Piinks

This is looking good.
@goderbauer can you give a second review?

packages/flutter/lib/src/animation/animation_controller.dart

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

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

goderbauer · 2022-04-06T22:26:45Z

packages/flutter/lib/src/animation/animation_controller.dart


+  /// Update the simulation without restarting the animation.
+  ///
+  /// The current simulation will be replaced with the provided [Simulation].


This could mean that the animation is making a jump if the simulations are very different at the current point in time? Since there is no smooth transition from one simulation to the next one, we should probably document that here.

goderbauer · 2022-04-06T22:27:32Z

packages/flutter/lib/src/animation/animation_controller.dart


+  /// Update the simulation without restarting the animation.
+  ///
+  /// The current simulation will be replaced with the provided [Simulation].


Also, it may be useful to add a little color to this doc describing when this API could be useful (and when not - with links to other API methods that may be more appropriate).

packages/flutter/lib/src/animation/animation_controller.dart

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

goderbauer · 2022-04-06T22:31:41Z

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

+  late double _initVelocity;
+
+  late double _initPosition;


change late to final for both of these?

goderbauer · 2022-04-06T22:33:14Z

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

+    double initVelocity = 0.0,
+    double initPosition = 0.0,


Somewhere we need to document what these parameters mean. Either in the constructor doc, or - if we can make them final as suggested below - you could make them public and document on there what they mean.

I am especially curious to learn why these have the "init" prefix...

goderbauer · 2022-04-06T22:34:28Z

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

+  /// Update the ballistic animation instead of restarting  it, for example as the result of a layout change after a flinging gesture.
+  ///
+  /// The [initVelocity] and [initPosition] refer to the starting values of the new ballistic animation.
+  Simulation? updateBallisticAnimation(double initVelocity, double initPosition);


document: when does this return null? What does that mean?

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

goderbauer · 2022-04-06T22:38:01Z

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

+      _initPosition,
+    );
+    if (newSimulation != null) {
+      _controller.updateSimulation(newSimulation);


How is it guaranteed that this doesn't give as a discontinued animation with a big jump when we switch simulations? (see my other comment about this above)

goderbauer · 2022-04-06T22:51:48Z

Same as #96512

Can you please update the PR description with an actual description of what this is fixing? I tried to follow the chain of links, but it is not clear to me. Is this fixing a perf regression? A fidelity problem? A bit of both?

goderbauer · 2022-04-20T21:36:53Z

@MxSoul Do you still have plans to come back to this PR?

goderbauer · 2022-05-04T21:49:22Z

@MxSoul I saw that you responded to some of the comments, did you maybe forget to push your updated code to this PR?

goderbauer · 2022-05-12T21:49:01Z

Since there hasn't been a response, I am going to close this one for now. If you find the time to work on this again, feel free to reopen this with the feedback from above addressed.

fix restart animation

f68abb3

flutter-dashboard bot added the a: animation Animation APIs label Mar 15, 2022

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Mar 15, 2022

add doc

2b426de

MxSoul changed the title ~~【WIP】fix restart animation~~ Fix restart ballistic animation while child size change when update simulation Mar 15, 2022

MxSoul changed the title ~~Fix restart ballistic animation while child size change when update simulation~~ Fix restart ballistic animation while child size change By update simulation Mar 15, 2022

MxSoul changed the title ~~Fix restart ballistic animation while child size change By update simulation~~ Update animation simulation to fix restart ballistic animation Mar 15, 2022

MxSoul added 2 commits March 16, 2022 00:59

delete print

0bf7e8e

update doc

b381281

goderbauer requested a review from Piinks March 16, 2022 21:38

Piinks reviewed Mar 22, 2022

View reviewed changes

packages/flutter/lib/src/widgets/nested_scroll_view.dart Show resolved Hide resolved

goderbauer mentioned this pull request Mar 23, 2022

Fix restart ballistic animation while child size change #96512

Closed

8 tasks

Merge remote-tracking branch 'origin/master' into update_ballistic_an…

790997f

…imation

MxSoul force-pushed the update_ballistic_animation branch from 03b1d51 to 790997f Compare March 25, 2022 07:39

add doc

d9e04f3

MxSoul mentioned this pull request Mar 25, 2022

[Proposal] Update animation simulation to fix restart ballistic animation which is used by NestedScrollView #100748

Closed

add test

3f8d899

MxSoul requested a review from Piinks March 25, 2022 08:49

Piinks reviewed Apr 5, 2022

View reviewed changes

fix doc

9401c60

goderbauer reviewed Apr 6, 2022

View reviewed changes

goderbauer closed this May 12, 2022

Piinks mentioned this pull request Jul 15, 2022

Update Ballistic animation & ClampingScrollSimulation #107735

Merged

8 tasks

Conversation

MxSoul commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 15, 2022

Uh oh!

MxSoul commented Mar 15, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MxSoul commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Apr 6, 2022

Uh oh!

goderbauer commented Apr 20, 2022

Uh oh!

goderbauer commented May 4, 2022

Uh oh!

goderbauer commented May 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MxSoul commented Mar 15, 2022 •

edited

Loading

MxSoul commented Mar 25, 2022 •

edited

Loading