Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@flar
Copy link
Contributor

@flar flar commented Oct 22, 2020

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 TransformMethod enums (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 the Clip enum used for the clipBehavior properties of the clip layers in that it suggest how to apply the operation. I'll note that I never liked the name Clip for 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 enum Transform since that is identical to the name of the widget where it will be used, so the naming had to diverge from the Clip model.

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

@flar
Copy link
Contributor Author

flar commented Oct 22, 2020

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 ?

@flar flar force-pushed the transform-method branch from 60ee502 to 0441a8c Compare October 22, 2020 09:22
@flar
Copy link
Contributor Author

flar commented Oct 22, 2020

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.

@liyuqian
Copy link
Contributor

A small suggestion for naming: we could use enum Cache {none, bitmap, /* maybe mipmap in the future */} which looks similar to enum Clip {...}.

BTW, I'm bad at naming and the naming of enum Clip, especially Clip.hardEdge, was suggested by Ian if I remember correctly. So we could schedule a short chat with Ian, or maybe utilize the API meeting to get some better suggestions.

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);
Copy link
Contributor

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?

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Nov 12, 2020
@flar
Copy link
Contributor Author

flar commented Apr 28, 2021

This will be solved more simply in the Framework by flutter/flutter#76742

@flar flar closed this Apr 28, 2021
ds84182 added a commit to ds84182/engine that referenced this pull request Feb 13, 2022
Additionally allows specifying sampling options and paint.
@ds84182 ds84182 mentioned this pull request Feb 13, 2022
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Work in progress (WIP) Not ready (yet) for review!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants