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

Fix SpringSimulation formula for underdamping #165017

Merged
merged 2 commits into from
Mar 21, 2025

Conversation

dkwingsmt
Copy link
Contributor

Fix #163858

Pre-launch Checklist

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

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 11, 2025
@dkwingsmt dkwingsmt requested a review from jonahwilliams March 11, 2025 22:25
Copy link
Member

@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.

RSLGTM

I trust your math :)

@dkwingsmt
Copy link
Contributor Author

Unfortunately this PR has caused some Google internal failures. I'm going to fix them (but probably not very soon since I have a lot on my hand). Also this change will probably count as breaking changes and I'll write a migration guide. Therefore don't expect this PR land very soon.

@ercantomac
Copy link

Unfortunately this PR has caused some Google internal failures. I'm going to fix them (but probably not very soon since I have a lot on my hand). Also this change will probably count as breaking changes and I'll write a migration guide. Therefore don't expect this PR land very soon.

Why will it count as breaking change? I don't see any changes in the API itself

@jonahwilliams
Copy link
Member

@dkwingsmt this should not be a breaking change. yes, updating scroll physics will cause some golden files to change heavily, but fixing this is more important than keeping those goldens the same.

@dkwingsmt
Copy link
Contributor Author

I think the idea is that developers do not expect their apps to behave differently after a version update. Silent changes like this are even worse than compile errors. A migration guide should be provided to let developers know how to calculate new values to achieve the previous effect.

@jonahwilliams
Copy link
Member

No that is absolutely not our policy. We need to be able to update things like scroll physics

@dkwingsmt
Copy link
Contributor Author

By our policy,

Additional test suites that we have been allowed to run but that are not public. (Notably, Google allows us to run several tens of thousands of proprietary tests on each commit.)

This PR breaks a few tests in Google tests. Shouldn't it count?

@jonahwilliams
Copy link
Member

If you changed something unrelated and a bunch of goldens broke, it is a good sign you introduced a bug yes. But in this case, you are intentionally fixing the spring simulation formula - so its expected that golden tests which perform something like a fling will change. Arguably these tests should not depend on the exact physics, because at any rate we must be able to make updates to physics to match the platform.

We are absolutely allowed to update golden files. Unless it is a major change (i.e. light colors become dark) we do not need to flag it.

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Mar 21, 2025
Merged via the queue into flutter:master with commit cc35800 Mar 21, 2025
75 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect damping coefficient in the _UnderdampedSolution class
3 participants