Skip to content

Conversation

@HansMuller
Copy link
Contributor

@HansMuller HansMuller commented Aug 18, 2021

Reverts #88409

Caused a performance regression, see #88480.

Unfortunately #88409 also caused some unexpected golden image failures on internal tests, i.e. not just failures that were obviously due to the route change animation (see internal Google issue b/197155815)

Fixes #88480

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Aug 18, 2021
@google-cla google-cla bot added the cla: yes label Aug 18, 2021
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@fluttergithubbot fluttergithubbot merged commit afe58fb into master Aug 19, 2021
@fluttergithubbot fluttergithubbot deleted the revert-88409-reland-new-transition branch August 19, 2021 03:22
@flar
Copy link
Contributor

flar commented Aug 19, 2021

What we would want here is a transition that used raster operations to both zoom and fade the widget. Unfortunately our zoom and our fade widgets operate independently so they would fight over who was creating the raster copy.

Eventually I could see this working via the following technique - an Opacity widget wrapping a raster scale widget with the scale widget caching the transitioning child and the opacity widget simply overriding the opacity of the scale widget's rendering.

We already have raster caching on the scale transition widgets, enabled by the filterQuality parameter on the transition widget.

Unfortunately the Opacity implementation will either cache its child (resulting in double caching in this case), or use a saveLayer (essentially the same thing, but using temporary memory rather than memory that carries forward between frames). There is already an issue (#75697) that has been requesting that the opacity layers forward their opacity values to the children for implementation, but we don't have a solution for that yet. When we do, though, the technique I outline here would work much better than what can be accomplished now.

@AlexV525
Copy link
Member

Unfortunately #88409 also caused some unexpected golden image failures on internal tests, i.e. not just failures that were obviously due to the route change animation (see internal Google issue b/197155815)

Any chances for those golden failures can be fixed recently?

@HansMuller
Copy link
Contributor Author

I haven't had time to investigate the source of those problems yet; will do so as soon as I am able.

@AlexV525
Copy link
Member

@flar Do you think we can reland this again? I saw the default enabled DisplayList has landed.

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

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regressions in Flutter gallery memory metrics on Android

5 participants