-
Notifications
You must be signed in to change notification settings - Fork 6k
add a bitmap/render hint to transform layers #22045
Conversation
|
I may need to adjust the web methods to include the new parameter, but the same question as the previous PR remains - whether or not we can implement this on web or if it should just be ignored there... @yjbanov ? |
60ee502 to
0441a8c
Compare
|
It looks like the CanvasKit implementation for Flutter Web has a RasterCache, but it is only currently implemented for picture objects so it would have to be upgraded to also cache layers to be able to implement this solution. |
|
A small suggestion for naming: we could use BTW, I'm bad at naming and the naming of |
| SkPath child_path; | ||
| child_path.addRect(5.0f, 6.0f, 20.5f, 21.5f); | ||
| SkRect cull_rect = SkRect::MakeXYWH(2.0f, 2.0f, 14.0f, 14.0f); | ||
| SkMatrix initial_transform = SkMatrix::Translate(-0.5f, -0.5f); |
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.
Shall we make a more complicated unit test to make sure all the matrix math is correct? For example, let initial_transform and layer_transform both have offset, scale, and rotation in them?
|
This will be solved more simply in the Framework by flutter/flutter#76742 |
Additionally allows specifying sampling options and paint.
This PR offers essentially identical performance for animated transforms to a prior PR (#21943) but uses a different mechanism to trigger the performance tradeoff. Please read the description of that PR for an overall description of the problem and the benefits of having the caching being performed by the Transform layers themselves.
These engine changes will eventually enable fixing flutter/flutter#24627
An additional PR for the Framework repo will be required to use these changes, but the new property I added here should be backwards compatible to existing Frameworks as it defaults to the compatibility value (
TransformMethod.render).In this solution a "method" hint is added to the transform layers to indicate whether they should use the transform for rendering the child, or to transform the bitmap representation of the child.
See the
TransformMethodenums (one in Dart and one in C++) for the reference for the API. I would especially like feedback on the naming of the enum and its values. This is logically similar to theClipenum used for theclipBehaviorproperties of the clip layers in that it suggest how to apply the operation. I'll note that I never liked the nameClipfor the behavior enum for the clipping since it sounds like it is a definition of the clip rather than a suggestion on how to apply it. In this case, it would be inappropriate to name this new enumTransformsince that is identical to the name of the widget where it will be used, so the naming had to diverge from theClipmodel.In the previous PR, the developer could specify a custom transform for caching the child, but in this PR the transform for caching will always be the transform inherited from the ancestor layers. The developer can still achieve control over the transform that the child is cached with by decomposing the transform being animated into a static part applied to the child, and a dynamic part applied as a bitmap to the statically transformed child by nesting two transform layers and only applying the bitmap hint to the outer transform. That nesting technique will not likely be used often except for cases like animating a scale from A to B where the developer may want to cache the child at the larger of (A,B) and then have the outer animating transform be the ratio difference (animating from A/B->1.0, or 1.0->B/A).
The performance is nearly identical to the previous PR, but it seems that I more often get lucky and have the performance of the "cached Transform" animation be closer to ~6.5ms, though the difference is within the margin of error since if I play with the test app for a bit, then sometimes it will perform closer to the ~7ms that I saw with the prior solution. I don't think this is significant other than to point out that this solution performs no worse than the previous solution. If it really is better in the long run, then that would have to be established by collecting benchmark data over time.
Since the API has changed, there is a new test app required which can be seen at this gist: https://gist.github.com/flar/41dbec4b5a610c61c40fa77ce2156b56