[framework] remove forced compositing from opacity #102687
Conversation
flutter-dashboard
bot
added
the
framework
|
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. |
|
actually the goldens I'm seeing are making me think this isn't working correctly, investigating |
|
running a few of these locally on windows and chrome they look identical |
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
The skia diffs do look odd: Colors are getting lighter/darker (transparency not getting applied correctly?) and the material.material.refresh_progress_indicator looks very low quality now...
| /// [RenderObject.alwaysNeedsCompositing] property to return true. That informs | ||
| /// ancestor render objects that this render object will include a composited | ||
| /// layer, which, for example, causes them to use composited clips. | ||
| @Deprecated( |
There was a problem hiding this comment.
add this also to the documentation above.
| /// is called synchronously during the call to [pushOpacityLayer]. | ||
| /// | ||
| /// {@macro flutter.rendering.PaintingContext.pushClipRect.oldLayer} | ||
| OpacityLayer? pushOpacityLayer(bool needsCompositing, Size size, Offset offset, int alpha, PaintingContextCallback painter, { OpacityLayer? oldLayer }) { |
There was a problem hiding this comment.
The name seems misleading since it doesn't always push a layer...
We should at least document the behavior.
There was a problem hiding this comment.
Yeah I don't know what to do for naming, since the old name is taken
There was a problem hiding this comment.
Doesn't pushTransform do similar things? It may or may not push a layer.
There was a problem hiding this comment.
Doesn't pushTransform do similar things? It may or may not push a layer.
Yeah, that's why it isn't called pushTransformLayer :) Ideally, we call this pushOpacity, but that name is already taken by the old method.
| /// is called synchronously during the call to [pushOpacityLayer]. | ||
| /// | ||
| /// {@macro flutter.rendering.PaintingContext.pushClipRect.oldLayer} | ||
| OpacityLayer? pushOpacityLayer(bool needsCompositing, Size size, Offset offset, int alpha, PaintingContextCallback painter, { OpacityLayer? oldLayer }) { |
There was a problem hiding this comment.
Add some tests for the new behavior of this method to verify when it creates a layer and when it doesn't, etc.?
| if (alpha == 0) { | ||
| return null; | ||
| } | ||
| canvas.saveLayer(offset & size, Paint()..color = Color(alpha << 24)); // TODO(ianh): Expose the alpha somewhere. |
There was a problem hiding this comment.
use one of the Color constructros where you can pass in the alpha value directly without the need for the magic << 24?
| if (alpha == 0) { | ||
| return null; | ||
| } | ||
| canvas.saveLayer(offset & size, Paint()..color = Color(alpha << 24)); // TODO(ianh): Expose the alpha somewhere. |
There was a problem hiding this comment.
Also, is the TODO still relevant? Can you access the alpha in a test from the paint object after the fact?
There was a problem hiding this comment.
No longer relevant, removed
|
Also skip saveLayer at 255 alpha |
|
I'm going to do a g3 run to see what those scubas do |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
|
Running google3 goldens I don't see any issues, its like opacity doesn't work but only on skia gold |
|
I agree that we only want the forced compositing and isolated children on animating things. Some apps might drive opacity animations directly themselves using an Opacity widget, but they should look to use an animated widget instead for multiple reasons (for one thing, fewer rebuilds of their tree). |
| @Deprecated( | ||
| 'Use pushOpacityLayer instead to avoid forced compositing. ' | ||
| 'This feature was deprecated after v2.13.0-0.0.pre.' | ||
| ) |
There was a problem hiding this comment.
What about adding a {needsCompositing = false} parameter to this method and it only will add a layer if it needs compositing?
There was a problem hiding this comment.
It's backwards from the way the other methods work, though, most have pushThingy() that take a positional needsCompositing...
There was a problem hiding this comment.
How much pain would we have if we just changed this method to have the positional parameter? Are these methods used directly in a lot of apps?
There was a problem hiding this comment.
I don't see any uses in google3 or in flutter/tests, which technically means we could change it with just a breaking change announcement.
There was a problem hiding this comment.
we could call it pushAlpha instead, which accurately describes the value it takes...
There was a problem hiding this comment.
Alpha is fine, but it has lower resolution than opacity. Eventually these things are stored as floats in both Skia and Impeller because GPUs tend to take float values for their shaders.
There was a problem hiding this comment.
sorry, I'm just talking about the name. The Paint/saveLayer API can only accept an alpha
There was a problem hiding this comment.
...currently...
We'll be supporting higher precision color info in the engine (SkPaint/Impeller Paint both already do so, with the Impeller object only taking float components - DlPaint currently takes int colors, but I will be enhancing it to allow float colors as well soon).
There was a problem hiding this comment.
time for Color2
There was a problem hiding this comment.
We may actually be able to swap out the backing implementing and add new APIs to handle this.
Agreed. The docs already push people strongly away from Opacity if they want to animate. |
|
Yeah, use an animated opacity widget instead of opacity to animate is fairly straightforward advice |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
|
So I reproduced the skia gold issues locally. This is due to #48417 To make this landable I will force compositing only on the web |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
| /// (as described at [Layer]). | ||
| int? get alpha => _alpha; | ||
| int? _alpha; | ||
| @Deprecated( |
There was a problem hiding this comment.
Note in the doc comment here (and maybe on the constructor as well) that providing an alpha is deprecated and opacity should be used instead.
| assert(value != null); | ||
| if (value != _alpha) { | ||
| if (value == 255 || _alpha == 255) { | ||
| final double valueAsOpacity = value! / 255.0; |
There was a problem hiding this comment.
Maybe have a private static method for this conversion since it is happening in (at least two places) and to encapsulate the magic number 255 a little bit.
|
|
||
| /// The opacity to composite with child layers. | ||
| /// | ||
| /// The opacity is expressed as a double from 0.0 to 1.0, where 00 is fully |
| return layer; | ||
| } | ||
|
|
||
| if (!kReleaseMode) { |
There was a problem hiding this comment.
Unless we changed it (or I am misremembering), I thought elsewhere debugDisableOpacityLayers is only honored in debug mode and not profile? (we should probably be consistent about this...)
There was a problem hiding this comment.
Yeah, OpacityLayer's addToScene only honors this when asserts are enabled, i.e. debug mode. Let's make this consistent.
| } | ||
|
|
||
| if (!kReleaseMode) { | ||
| if (debugDisableOpacityLayers) { |
There was a problem hiding this comment.
Shouldn't this be placed before the if branch to push the layer above to disable those opacity effects as well?
There was a problem hiding this comment.
Ah, those are disabled in the OpacityLayer's addToScene. Never mind then. Maybe add a comment about that here, though since it does look strange that we seemingly disable it in one code path, but not the other...
| return; | ||
| } | ||
| super.paint(context, offset); | ||
| layer = context.pushOpacity(needsCompositing, offset, _opacity, super.paint, oldLayer: layer as OpacityLayer?, size: size); |
There was a problem hiding this comment.
nit: maybe add some line breaks to this to make it a little easier to read (add trailing comma and then one property per line)
…d_opacity_compositing
| double? opacity, | ||
| super.offset, | ||
| }) : _alpha = alpha; | ||
| }) : _opacity = opacity ?? (alpha != null ? _alphaToOpacity(alpha) : null); |
There was a problem hiding this comment.
we should assert that we're not passing both (i.e. that at least one of them is null)
| builder.pop(); | ||
| } | ||
|
|
||
| @override |
There was a problem hiding this comment.
why all the work to switch to using a double if the engine just takes an int?
There was a problem hiding this comment.
Well there is also: flutter/engine#33035
There was a problem hiding this comment.
Yes, we need to be driving towards deeper components just from the perspective of round off errors on parameters that will be multiplied a lot during the rendering pipeline, and >8-bit color components on screens is already happening.
| pushLayer(layer, painter, Offset.zero); | ||
| return layer; | ||
| /// {@macro flutter.rendering.PaintingContext.pushClipRect.oldLayer} | ||
| OpacityLayer? pushOpacity(bool needsCompositing, Offset offset, double opacity, PaintingContextCallback painter, { OpacityLayer? oldLayer, Size? size }) { |
There was a problem hiding this comment.
can we move this into the Opacity render object? (we might not be able to, i forget the constraints, but in general the more we can push out of PaintingContext the simpler it gets to eventually remove it entirely)
There was a problem hiding this comment.
this is used in a few different places, I think the flow layout and a cupertino widget. The code itself isn't that complicated so I could repeat it, or I could move it into a toplevel helper method in proxy_box. WDUT?
| return null; | ||
| } | ||
| // Due to https://github.com/flutter/flutter/issues/48417 this will always need to be | ||
| // composited on the web. |
There was a problem hiding this comment.
if we could easily detect canvaskit mode, this would only be an issue on HTML renderer. @yjbanov for the feasibility of either fixing that or giving us a better flag
There was a problem hiding this comment.
from discussion offline, this isn't fixable in html mode.
There was a problem hiding this comment.
(not fixable with the current renderer)
…d_opacity_compositing
…opacity_compositing
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Generally LGTM, but I was a bit confused over the null-ness handling of the parameters so I left questions to be answered.
| }) : assert( | ||
| (alpha == null && opacity == null) || | ||
| (alpha == null && opacity != null) || | ||
| (alpha != null && opacity == null), |
There was a problem hiding this comment.
Couldn't this be rewritten as alpha == null || opacity == null?
Also, shouldn't they be required to specify at least one?
There was a problem hiding this comment.
Looking below, you can set either to a non-null, but there is no way to have both be null using setters, so there should not be a way to have both arguments null in the constructor, no?
There was a problem hiding this comment.
I see that it used to allow a null alpha so maybe this is for code that constructs first and sets later?
Also, my original comment on this thread stands - the 3 tests can be combined into one.
| ) | ||
| int? get alpha => _opacity == null ? null : _alphaFromOpacity(_opacity!); | ||
| set alpha(int? value) { | ||
| assert(value != null); |
There was a problem hiding this comment.
Why isn't this done by changing the parameter to a non-null type?
There was a problem hiding this comment.
The language lints having getters and setters of a different type. I think we could work around this by making both non-nullable but changing the backing storage to have a late modifier so we get around that.
| double? get opacity => _opacity; | ||
| double? _opacity; | ||
| set opacity(double? value) { | ||
| assert(value != null); |
There was a problem hiding this comment.
Should this just use a non-nullable type instead?
There was a problem hiding this comment.
I looked into changing this by using late, but then that doesn't allow whether the field has been set to be observed so we can't compare the new value to the old in the setter safely. We could default to 0.0 or 1.0 but that would make current misuse of the API that throws an exception instead silently do ... something else.
There is likely a way we can re-arrange this API to work better but I think it would be best as part of a separate effort, so that we can cover all of the layer types
There was a problem hiding this comment.
I meant as an argument type. The internal fields could be nullable if that makes things easier, but having an "assert(argument != null)" as the first line in a method indicates that arg should probably have been expressed as non-nullable...
| builder.pop(); | ||
| } | ||
|
|
||
| @override |
There was a problem hiding this comment.
Yes, we need to be driving towards deeper components just from the perspective of round off errors on parameters that will be multiplied a lot during the rendering pipeline, and >8-bit color components on screens is already happening.
…d_opacity_compositing
|
I believe that I've addressed all feedback and will plan on landing this on green CI assuming no other problems.... |
| alpha == null || opacity == null, | ||
| 'Only one of alpha and opacity should be provided.' |
There was a problem hiding this comment.
nitty-nit: indent these two lines by a little more?
|
Based on our experience trying to update the physical model layer, I'm going to make this one opt in |
…d_opacity_compositing
…illiams/flutter into remove_forced_opacity_compositing
|
I'm currently waiting on a g3 frob run to confirm this has a smaller number of breakages |
|
|
||
| /// This is an experimental flag which should not be used in production and will | ||
| /// be removed without warning. | ||
| bool forceOpacityCompositing = true; |
There was a problem hiding this comment.
Mark this as deprecated so people don't accidentally use it since the plan is to presumably remove this once migration is complete? That's what we usually do with these kind of flags that assist with migration.
There was a problem hiding this comment.
Also, can this just be a static on PaintingContext instead of a global to not expose this in the global namespace?
|
this has gotten stale, will reopen once we've resolved the scuba issues |
4 participants
Fixes #102179
Removes the force compositing and repaint boundary status of the stand alone opacity widget. In practice, i found the conditional repaint boundary based on child a bit hard to implement and I think this change will be more impactful. We could revisit conditional repaint boundaries in the future, or instead lean towards separating out animating and non-animating render objects more distinctly.