-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix buggy formula for critically-damped springs #120488
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
Conversation
94ea3c9 to
cd77631
Compare
|
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. |
Piinks
left a comment
There was a problem hiding this 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!
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks for the testing and reviews! I see the "Google testing" check is pending. I'll rebase in hopes of unwedging that. |
cd77631 to
da3b805
Compare
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.
da3b805 to
6b80c8e
Compare
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.