Use toPictureSync for faster zoom page transition #106621
Conversation
flutter-dashboard
bot
added
f: material design
|
FYI @dnfield , this is awesome |
| @@ -250,18 +253,137 @@ class _ZoomPageTransition extends StatelessWidget { | |||
| } | |||
| } | |||
|
|
|||
| class _ZoomEnterTransition extends StatelessWidget { | |||
| abstract class _RadicallyAwesomeRenderObject extends RenderProxyBox { | |||
There was a problem hiding this comment.
|
Ahh shit, web |
|
Updated numbers with pixel 6 benchmark, the improvement is even more dramatic, partially due to a new benchmark I wrote which isolates the page transition better |
|
The fidelity of this is a little off, but promising results so far. Hopefully fixing the fidelity doesn't tank the performance... |
flutter-dashboard
bot
added
the
team
| // found in the LICENSE file. | ||
|
|
||
| import 'package:flutter_driver/flutter_driver.dart'; | ||
| import 'package:test/test.dart' hide TypeMatcher, isInstanceOf; |
There was a problem hiding this comment.
Btw I am curious why isInstanceOf is hidden?
There was a problem hiding this comment.
Don't remember, its just a copy paste from the other test
There was a problem hiding this comment.
I believe it causes type definition clashes because flutter_test defined this before it was availalbe in package:test or something.
|
I also investigated total frame count after @dnfield raised some concerns about potentially hidden costs of toGpuImage. I found that with toGpuImage, the attached benchmark pumps about 208 frames. With the old version, we get about 65 frames - on a pixel 6. |
| final Rect src = area; | ||
| final double newWidth = src.width * scale / pixelRatio; | ||
| final double newHeight = src.height * scale / pixelRatio; | ||
| final double leftOffset = (src.width / pixelRatio - newWidth) / 2; | ||
| final double topOffset = (src.height / pixelRatio - newHeight) / 2; | ||
| final Rect dst = Rect.fromLTWH(src.left + leftOffset, src.top + topOffset, newWidth, newHeight); | ||
| context.canvas.drawImageRect(image, src, dst, paint); |
There was a problem hiding this comment.
I think this math can be a little confusing if you're not thinking about dp vs physical pixels. I think this could be more easily done by putting a scale on the canvas itself and just using drawImage
|
I'm observing some odd GC behavior, it seems to trigger consistently when calling toGpuImage |
|
Filled #106698 |
| } | ||
| final bool updateImage = delegate.willPaint(animation); | ||
| if (_childImage == null && updateImage) { | ||
| _childImage = PaintingContext.paintAndDetachToGpuImage( |
There was a problem hiding this comment.
Will we have trouble if the children is changing during animation? For example, there may be a loading indicator that is animating during this zoom page transition. Or there may be a splash effect happening because user has just tapped a button. Since zoom page transition is a very general transition, we may not be able to assume all things like loading indicator or splash effect never happen to be a child in the subtree.
In those cases, the content (i.e. childImage) actually should be updated, but we are caching it and never update it except when animation status changes.
Therefore, users may see the loading indicator or splash "freezed" during this zoom IMHO.
There was a problem hiding this comment.
That is a price I am willing to pay
There was a problem hiding this comment.
I agree. But maybe we should have some flags allowing users to customize it? Or maybe some documentation saying this is expected behavior - or people may file a bug about it
There was a problem hiding this comment.
@goderbauer did we ever change route transitions to turn the ticker mode off, or was that just in customer: money's app?
There was a problem hiding this comment.
I suspect that would not have improved raster performance at all for zoom page, given that the scale transform would invalidate all raster caches
There was a problem hiding this comment.
It was for the opacity one. Animation was busting the raster caching there
There was a problem hiding this comment.
makes sense. We could apply this sort of fix to the other route transitions but since the current zoom page transition doesn't work with any raster cache heuristics, then changing this transition will have an oversized impact.
Not clear to me if we have other route transitions that are as heavily used
There was a problem hiding this comment.
did we ever change route transitions to turn the ticker mode off, or was that just in customer: money's app?
We did something like that for customer:money for some transition types.
There was a problem hiding this comment.
That is a price I am willing to pay
I am going back and forth on this. On the one hand, this is also how iOS handles transitions, I believe. On the other hand, this was a cool feature of flutter that we could do animations while transitioning...
At the very least, we need to document this behavior somewhere. I also wonder if we need a flag to opt out of this behavior?
One example where you absolutely want animations to run during transitions are Hero animations. It's the whole point of a Hero that they fly around while you transition between routes. How does this change affect Heros? (Maybe they are unaffected, because the Hero is placed in a separate Overlay?)
Also, there are transitions where when you trigger them manually (e.g. by swiping back) things actually animate on the screen (e.g. a back-arrow animates back to a Hamburger Menu button). Are any of these use cases impacted?
Rather than using transform layers, snap off a texture of the current/next route using toGpuImage, and then draw the scale/opacity operations using these images while the animation is ongoing.
Edit: Updated because I realized I was accidentally drawing one of the layers twice + dpr
Pixel 6
Wembly
Fixes #104680
Fixes #100972