[framework] introduce repaint boundary in Opacity widgets #101601
Conversation
flutter-dashboard
bot
added
a: animation
| /// to reuse textures on subsequent frames if the opacity is changing but | ||
| /// the child layer is not. If the child layers are usually changing along | ||
| /// with the opacity, setting this value to `false` will reduce the size of the | ||
| /// layer tree. |
| /// | ||
| /// This value defaults to true. The repaint boundary allows the engine | ||
| /// to reuse textures on subsequent frames if the opacity is changing but | ||
| /// the child layer is not. If the child layers are usually changing along |
There was a problem hiding this comment.
See also my note here: #101597 (comment)
If we decide to keep it on Opacity, we may want to link to AnimatedOpacity and FadeTransition from here and emphasize that those may be better choices if the opacity changes frequently?
There was a problem hiding this comment.
The fact that we need a switch to turn this off makes me wonder if we should actually just omit this feature from the Opacity widget. Did you see a use case where this helped? I would have assumed that this is mostly helpful for animated opacities (and for those one should use AnimatedOpacity or FadeTransition).
There was a problem hiding this comment.
So this optimiztion isn't really about animated opacities, its just easier to see when the opacity is animated. Anytime you repaint anything above an opacity, whether or not the opacity or its children have changed - you will go down a much slower path if the engine thinks the child layers have been updated.
There was a problem hiding this comment.
In this particular case though, I'm not really in love with this flag. I think we should just update the progress spinner to not use an opacity widget and instead paint with opacity.
There was a problem hiding this comment.
I think we should just update the progress spinner to not use an opacity widget and instead paint with opacity.
+1 to that if that's possible.
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| return _Opacity( | ||
| opacity: opacity, | ||
| alwaysIncludeSemantics: alwaysIncludeSemantics, | ||
| child: RepaintBoundary(child: child), |
There was a problem hiding this comment.
uber-nit: maybe format this as:
| child: RepaintBoundary(child: child), | |
| child: RepaintBoundary( | |
| child: child, | |
| ), |
to make it clear to the casual reader (i.e. me right now) where the repaint boundary is getting introduced...
| assert(alwaysIncludeSemantics != null), | ||
| super(key: key, child: child); | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove one extra line of white space
| @@ -1759,7 +1759,7 @@ class _AnimatedOpacityState extends ImplicitlyAnimatedWidgetState<AnimatedOpacit | |||
| return FadeTransition( | |||
| opacity: _opacityAnimation, | |||
| alwaysIncludeSemantics: widget.alwaysIncludeSemantics, | |||
| child: widget.child, | |||
| child: RepaintBoundary(child: widget.child), | |||
| return _FadeTransition( | ||
| opacity: opacity, | ||
| alwaysIncludeSemantics: alwaysIncludeSemantics, | ||
| child: RepaintBoundary(child: child), |
…utter#101601)" This reverts commit 6def159.
Fixes #101597