Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The calculation method for _CriticalSolution seems wrong #109675

Closed
LevisonNN opened this issue Aug 17, 2022 · 5 comments · Fixed by #120488
Closed

The calculation method for _CriticalSolution seems wrong #109675

LevisonNN opened this issue Aug 17, 2022 · 5 comments · Fixed by #120488
Labels
a: animation Animation APIs framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version

Comments

@LevisonNN
Copy link

LevisonNN commented Aug 17, 2022

Steps to Reproduce

  1. I was reading the source code of Flutter and found a little mistake in spring_simulation.dart.
  2. It is in line 185. When calculating the critical damping solution, I got a different result.
  3. My calculation steps are as follows: y = (c1 + c2 * t) * e ^(r * t) get: v = c2 * e^(r * t) + (c1 + c2 * t) * r * e ^ (r * t) .
  4. When t = 0, get: y = c1, v = c2 + c1 * r, then c2 = v - c1 * r .
  5. So I think this code should be: "final double c2 = velocity - (r * distance);" , but not "final double c2 = velocity / (r * distance );".
  6. I calculated many times and tried to make sure every step of my calculation was right, always got this result.
  7. Was this a mistake or am I wrong?
  8. Thank you.

Expected results:

Actual results:

Code sample
Logs
@LevisonNN
Copy link
Author

image

@danagbemava-nc danagbemava-nc added the in triage Presently being triaged by the triage team label Aug 18, 2022
@danagbemava-nc danagbemava-nc changed the title The coculation method for _CriticalSolution seems wrong The calculation method for _CriticalSolution seems wrong Aug 18, 2022
@danagbemava-nc
Copy link
Member

@danagbemava-nc danagbemava-nc added framework flutter/packages/flutter repository. See also f: labels. and removed in triage Presently being triaged by the triage team labels Aug 18, 2022
@goderbauer goderbauer added a: animation Animation APIs P2 Important issues not at the top of the work list labels Aug 23, 2022
@gnprice
Copy link
Member

gnprice commented Feb 9, 2023

I agree this is wrong.

I found it as part of investigating #120338, when I went and audited all the simulations the framework uses for scroll physics to try to confirm that each one was consistent with the scroll protocol's invariant #120341 that requires the scroll physics to make at least a certain kind of sense physically. One consequence of this bug is that it does not satisfy that invariant.

Another salient consequence of this bug is that the simulation's actual initial velocity can be very different from the requested initial velocity velocity:

  • Specifically, the initial velocity is c2 + r * distance.
  • With the buggy formula, this is r * distance + velocity / (r * distance).
  • This is a very different formula from velocity, and depending on the numbers involved it can produce very different results.

I have not yet tried to exercise this behavior in the context of an app. I'm not yet sure exactly what effect it has for users.

@gnprice
Copy link
Member

gnprice commented Feb 9, 2023

Looking closer, I think in fact we never actually use _CriticalSolution for any widget in the framework.

As a result, this will not affect any app unless it calls a SpringDescription constructor for itself and manages to create a spring description that _SpringSolution recognizes as critically damped. That happens only when damping * damping - 4 * mass * stiffness is equal to 0.0 — not just within tolerance, but equal as doubles.


There are four SpringDescription constructor call sites in the framework. All are constants.

  • The two found in scroll_physics.dart are both explicitly overdamped, with ratio: 1.1 and ratio: 1.3 respectively.

  • The spring in cupertino/sliding_segmented_control.dart is very close to critically damped. But because its parameters are written to "only" six significant figures, it ends up with a damping ratio of 1.0000007, very slightly overdamped, and so does not use this code.

  • The spring used by default in AnimationController.fling is constructed to be critically damped:

    final SpringDescription _kFlingSpringDescription = SpringDescription.withDampingRatio(
      mass: 1.0,
      stiffness: 500.0,
    );

    It uses that constructor's default damping ratio, which is double ratio = 1.0.

    But this means a SpringDescription.damping field of ratio * 2.0 * math.sqrt(mass * stiffness), which as a double is 44.721359549995796. And plugging that into the above formula gives 2.2737367544323206e-13 ­. Very close to zero, but not equal.

    Instead that value is positive, so the spring is treated as overdamped.

So, left to its own devices, the framework only ever uses _OverdampedSolution, even though two of the four springs are overdamped only due to rounding. It never uses _CriticalSolution, nor _UnderdampedSolution, unless the app creates its own SpringDescription for such a spring.

If an app does create a critically-damped SpringDescription and try to use it, I expect it would see wildly wrong behavior due to this bug.

I have a fix for this issue. I'll send a PR.

gnprice added a commit to gnprice/flutter that referenced this issue Feb 14, 2023
Fixes flutter#120341.

The scroll protocol makes an important assumption about the behavior
of ScrollPhysics implementations, and this requirement hasn't been
clearly documented.  Add documentation for it.

Parts of the text are modelled on similar language at
StatelessWidget.build and StatefulWidget.build.

It does feel a bit uncomfortable to juxtapose this description of a
required invariant with three issues where the framework doesn't
satisfy it.  Fortunately two of them apply by default only in
uncommon cases: flutter#120340 macOS touchpad flinging, and flutter#109675 never.

The third is flutter#120338, affecting default scrolling on Android and
other non-Apple platforms.  I'll send a PR to fix that shortly,
and another for flutter#109675.

As discussed at flutter#120338, it's quite possible we'll remove this
invariant in the future.  But that's been attempted before, and is
complicated: the invariant is a useful one.  Removing it would almost
certainly involve a breaking change for ScrollPhysics subclasses.  So
I think even if we had an immediate plan to remove it, we'd need some
kind of documentation for it, if only to explain the breaking change.
auto-submit bot pushed a commit that referenced this issue Feb 16, 2023
Fixes #120341.

The scroll protocol makes an important assumption about the behavior
of ScrollPhysics implementations, and this requirement hasn't been
clearly documented.  Add documentation for it.

Parts of the text are modelled on similar language at
StatelessWidget.build and StatefulWidget.build.

It does feel a bit uncomfortable to juxtapose this description of a
required invariant with three issues where the framework doesn't
satisfy it.  Fortunately two of them apply by default only in
uncommon cases: #120340 macOS touchpad flinging, and #109675 never.

The third is #120338, affecting default scrolling on Android and
other non-Apple platforms.  I'll send a PR to fix that shortly,
and another for #109675.

As discussed at #120338, it's quite possible we'll remove this
invariant in the future.  But that's been attempted before, and is
complicated: the invariant is a useful one.  Removing it would almost
certainly involve a breaking change for ScrollPhysics subclasses.  So
I think even if we had an immediate plan to remove it, we'd need some
kind of documentation for it, if only to explain the breaking change.
gnprice added a commit to gnprice/flutter that referenced this issue Feb 23, 2023
Fixes flutter#109675.

This formula would produce an initial velocity quite different
from the one specified as an argument.

To update the test, I computed the expected results separately
by using the physical formula.

Happily, the framework by default never ends up actually exercising
this code.  Of the four SpringDescription call sites within the
framework, two are explicitly overdamped; the other two are by
design critically damped, but due to rounding they end up being
treated as (very slightly) overdamped too.  Details here:
  flutter#109675 (comment)

So the only way an app could be affected by this bug is if it called
a SpringDescription constructor itself, and managed to create a spring
description where the distinguishing formula in _SpringSolution comes
out exactly equal to zero.  It's likely nobody has ever shipped such
an app, because the behavior this produces would be so wildly wrong
that it'd be hard to miss when exercised.
auto-submit bot pushed a commit that referenced this issue Feb 23, 2023
Fixes #109675.

This formula would produce an initial velocity quite different
from the one specified as an argument.

To update the test, I computed the expected results separately
by using the physical formula.

Happily, the framework by default never ends up actually exercising
this code.  Of the four SpringDescription call sites within the
framework, two are explicitly overdamped; the other two are by
design critically damped, but due to rounding they end up being
treated as (very slightly) overdamped too.  Details here:
  #109675 (comment)

So the only way an app could be affected by this bug is if it called
a SpringDescription constructor itself, and managed to create a spring
description where the distinguishing formula in _SpringSolution comes
out exactly equal to zero.  It's likely nobody has ever shipped such
an app, because the behavior this produces would be so wildly wrong
that it'd be hard to miss when exercised.

Co-authored-by: Kate Lovett <katelovett@google.com>
@danagbemava-nc danagbemava-nc added the r: fixed Issue is closed as already fixed in a newer version label Feb 27, 2023
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: animation Animation APIs framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list r: fixed Issue is closed as already fixed in a newer version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants