Skip to content

Conversation

@gnprice
Copy link
Member

@gnprice gnprice commented Feb 10, 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.

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 flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 10, 2023
@goderbauer goderbauer requested a review from Piinks February 14, 2023 23:22
@Piinks
Copy link
Contributor

Piinks commented Feb 16, 2023

I agree with your (very detailed) assessment of this and the potential impact! I am running this through some extra testing up front as well.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience while I ran this through additional testing! LGTM!

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gnprice
Copy link
Member Author

gnprice commented Feb 23, 2023

Thanks for the testing and reviews!

I see the "Google testing" check is pending. I'll rebase in hopes of unwedging that.

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.
@Piinks Piinks added a: fidelity Matching the OEM platforms better f: scrolling Viewports, list views, slivers, etc. a: quality A truly polished experience autosubmit Merge PR when tree becomes green via auto submit App labels Feb 23, 2023
@auto-submit auto-submit bot merged commit 9793885 into flutter:master Feb 23, 2023
@gnprice gnprice deleted the pr-spring-fix branch February 23, 2023 19:38
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: fidelity Matching the OEM platforms better a: quality A truly polished experience autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The calculation method for _CriticalSolution seems wrong

3 participants