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
[framework] restore old zoom page transition for benchmarking. #133346
Conversation
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.
Happy to see this!
// that was too slow to run on the Skia backend. This is being benchmarked on | ||
// the Impeller backend so that we can improve performance enough to restore | ||
// the default behavior. | ||
class _ZoomPageTransitionNoCache extends StatelessWidget { |
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.
Is this class needed? Reading the code it looks like the existing class should work so long as you specify allowSnapshotting to be false?
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.
The old implementation uses an ImageFilter.matrix to scale the individual pages without causing text relayout/snapping. The fallback uses a transform layer because its designed to work well if we can't apply an ImageFilter to content, which is common with platform views on iOS/Web.
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 🎉
|
||
|
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.
nit: remove one of the extra blank lines?
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.
Done
|
||
|
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.
same nit here
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.
Done
In the past I switched the implementation of the zoom page transition because the performance of the old transition was terrible, but I'm hopeful that with Impeller we'll be able to identify and fix the issues that made it so slow. In order to evaluate this though, we need to be able to opt into the old transition for benchmarks on CI.
#129742
#121325