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 Apr 15, 2022

Fixes flutter/flutter#101957

This PR fixes a flaw in how the opacity inheritance flags are computed between layers of the tree. It is still somewhat of a work in progress as I want to revisit all of the layers and check their logic, but at least for the combination of a FadeTransition with a ScaleTransition child, the layer cache no longer re-populates the cache on every frame and the rasterizer times are way down with a complex child.

This PR also adds the inheritance logic to ImageFilterLayer which used to not participate in opacity inheritance, so that is an additional win with this fix, but each part of this PR (both the part that fixes the logic and the new inheritance in ImageFilterLayer) are important to the example above. Fixing either still ends up with problems.

Note that there is one more problem I've discovered before this is a total solution for the Fade/Scale pair and that is that we fumble the coordinates a bit for an animated scale with the filterQuality. The reasons for that are in our cache rendering logic and I'm still investigating those.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

// method will then reset the field to false if it detects any
// incompatible children.
//
// 2. If you need to do change any logic depending on the answer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "neet to do change" is confusingly phrased.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh! Fixed in my local repo pending writing some tests and going over the logic layer by layer.

Copy link
Contributor

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

LGTM

@chinmaygarde chinmaygarde added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 21, 2022
@fluttergithubbot fluttergithubbot merged commit 62054c5 into flutter:main Apr 21, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 21, 2022
@flar
Copy link
Contributor Author

flar commented Apr 21, 2022

Eeek, I was busy adding tests and fixing things. I should have marked this a WIP.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs tests waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine layers that can always inherit opacity may report that they cannot depending on their children

4 participants