Add a bitmap operation property to transform widgets to enable/control bitmap transforms #76742
Conversation
|
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. 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. |
flutter-dashboard
bot
added
framework
|
What was your conclusion regarding the idea of specializing the various animation widgets around transformation (like ScaleTransition), following a model similar to the opacity-animating widgets, rather than doing it on Transform itself? |
That is an interesting idea that is independent of getting this API right and should be investigated in a separate effort. It did open my eyes to a new implementation for this API, though. Previously I had been working to make the engine transform layers do double duty until I realized that I could simply do the translation in the RenderObject with no changes to the engine required. |
|
I don't think it actually is independent. If we were able to make the transitions use the efficient model, we may be able to avoid making any API changes at all, since we wouldn't necessarily need to expose this (at least, not on Transform, which is generally not directly animated). |
|
I thought you were referring to the implementation detail where animation values were handed down to the RenderObject level so that the widgets didn't need to rebuild as the animation is driven. You seem to be asking about an idea where the bitmap path is chosen automatically by the animation and transition widgets and I don't think that's a good idea for 2 reasons:
|
|
With respect to whether the base Transform widget needs to have this property, it will enable a custom animation that isn't driven by an AnimationController to use this mechanism. It would also allow a developer to create a "blocky zoom effect" if that is what they want their output to look like. Developers have also indicated that they have uses for bitmap transformation that have nothing to do with animation as can be seen by this comment that appeared after the ImageFiltered issue was closed: #13489 (comment) |
|
Ah, that makes sense. |
|
Sorry, I must have missed the discussion that connects the Dirty Region Management design with this PR. How are the two connected? It seems to me that the concern "I want the engine to rasterize my subtree and use a certain sampling quality when compositing" is orthogonal to the concen "I want to apply a transform matrix to my subtree". Have you considered keeping |
|
Sorry, I had the wrong doc link in the description - fixed now. Some of the questions you asked are answered in the corrected document link. In particular, sampling is associated with operations so it would normally go on a widget that performed an action. Also, what coordinate transform would a RasterLayer rasterize at? It is inside the Transform widget and has no idea what the environment was like outside the "action" layer that it is supposed to be turning into a bitmap operation. As we descend the tree we get a Transform that will change the matrix stored in the context, then it will ask the RasterLayer to do its thing and the RasterLayer will - rasterize itself at the indicated matrix? Next frame there is a new matrix and the RasterLayer doesn't really have the information to do much more than just render again under the new matrix. There are ways we could work around this, but it would require a lot more communication between layers to coordinate when most of the system is designed around single-pass depth-first tree traversals. It is easier for the RenderTransform to make the decision as to which type of layer to use depending on whether the Transform widget was given permission to switch to a bitmap operation - almost no mechanism involved in such a case. |
|
This seems fine to me but I'll let @goderbauer or @yjbanov do the actual detailed review. @flar is there a bug filed on making the more efficient transformation animation widgets? Like AnimatedRotation, etc? |
I can check again, but after running through them as I was creating this PR I wasn't sure that there is much missing. ScaleTransition and RotatingTransition both exist to animate scales and rotations. The AnimatedFoo widgets seem to be designed around responding to changes in the child so we have AnimatedSize that responds to a child's size, but not something like AnimatedScale since children don't usually have an observable "scale" (or "rotation") to observe and control through animation (the changes in size can be detected by watching the results of the layout pass and comparing to the former size). When I wrote the doc and when we had a discussion about the designs in it I seemed to feel that there were missing holes, but I think we're pretty much covered for the common cases. |
They're about automatically responding to changes in the widget's properties. See e.g. AnimatedOpacity vs FadeTransition. |
Ah, I see the distinction. I was looking primarily at AnimatedSize which is reacting to a child's size, but others actually maintain a parameter themselves that can be changed atomically by a builder, but result in an animated change at the RenderObject level. So we are missing AnimatedScale which would maintain a scale property of its own and apply it slowly over time, and potentially AnimatedRotate as well. Both animated versions of Transform.scale and Transform.rotate. Potentially AnimatedTransform.rotate/scale. I'll file an issue. |
|
Heads up. Unfortunately, I won't be able to get back to this until next week. |
There was a problem hiding this comment.
We still need to null out the layer if child is null, no?
There was a problem hiding this comment.
We don't need to. It won't be used on this pass, but it might be used if we have a new frame that does have a child. It wasn't originally cleared in the null child case, so that hasn't changed here.
There was a problem hiding this comment.
I've still left this in the latest push because it was present in existing logic so I didn't want to change code I wasn't specifically introducing to fix the issue.
There was a problem hiding this comment.
If this layer it going to unconditionally push a layer, then we need to make sure that we set needsCompositing to true, or that alwaysNeedsCompositing is set to true.
There was a problem hiding this comment.
Adding/removing a child (via adoptChild) will request an update to the compositing bits. It looks like the update method here will look for a child that requires compositing.
What I think may be missing (and it was missing before my modifications) is that if the transform evaluates to a translation then we avoid a layer and that won't be reflected in the value of needsCompositing for this RO or its parents. That issue is not changed here.
There was a problem hiding this comment.
I'm not sure that's the case. Previously we had:
if (childOffset == null) {
layer = context.pushTransform(needsCompositing, ..., oldLayer: layer);
} else {
super.paint(context, offset + childOffset);
layer = null;
}This says that if it's not a simple translation (the if branch), then context.pushTransform will either push a layer or inject a canvas transform, depending on the value of needsCompositing. If it's a simple transform (the else branch), we don't push layers. So there are 3 possible outcomes:
- Not simple translation, needs compositing: push layer
- Not simple translation, needs not compositing: no layer
- Simple translation: delegate to children
There's no situation when needsCompositing == false and we push a layer.
After this PR, we have the following situation: filterQuality != null, needsCompositing == false => push a layer.
It's OK to push a layer when none of the descendants need compositing, but we still need to set the render object's own needsCompositing to true. Otherwise, its ancestors will do wrong things.
There was a problem hiding this comment.
I see. Basically it would only push a transform layer if a child was already planning to composite (push a layer?). So, it was impartial wrt whether it was going to push a layer itself - if no children were planning to, then it didn't. Thus, this layer used to not have any additional requirements as to the compositing bits other than the "standard" pass through from the children.
Now that FQ means we will be pushing a layer, that needs to factor in...
There was a problem hiding this comment.
I have now updated the alwaysNeedsCompositing handling to match what I think you were saying here.
There was a problem hiding this comment.
Yep, the latest iteration looks correct.
flar
force-pushed
the
add-filter-quality-to-transform-widgets
branch
from
b780733 to
1022992
Compare
|
@yjbanov do my clarifications above answer your questions? |
flar
force-pushed
the
add-filter-quality-to-transform-widgets
branch
from
1022992 to
7bc82c6
Compare
|
The |
|
Gold has detected about 1 untriaged digest(s) on patchset 4. |
|
The windows tests appear to be a flake. I'll try rebasing and see if they go away. But the linux failure appears to be due to #45213. Since the HTML renderer does not support ImageFilter.matrix currently I'll either have to solve that issue or find a way to work around it as the FilterQuality property is just a hint and shouldn't cause a web app to fail with that exception. The exception occurs in the constructor so we can't have the html renderer detect the type and work around it - that constructor would have to not throw the exception first. Alternately we could catch the exception when RenderTransform tries to instantiate the filter and back off to the regular transform paths. |
flar
force-pushed
the
add-filter-quality-to-transform-widgets
branch
from
b9b06a7 to
f54dc5b
Compare
|
Gold has detected about 1 untriaged digest(s) on patchset 4. |
|
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. Changes reported for pull request #76742 at sha f54dc5bf959c637b726c094ab3e4e0d21e283d80 |
flar
force-pushed
the
add-filter-quality-to-transform-widgets
branch
from
f54dc5b to
ebe822e
Compare
|
Tests added. The golden tests are no longer asking for triage. It should be good to go pending an approval... |
There was a problem hiding this comment.
nit: hasLength would produce a better failure message, i.e.:
expect(tester.layers.whereType<ImageFilterLayer>(), hasLength(1));
There was a problem hiding this comment.
nit: for testing zero length consider using the isEmpty matcher, also for better error messages (e.g. "expected empty, but found 5 elements")
|
Gold has detected about 1 untriaged digest(s) on patchset 5. |
flar
force-pushed
the
add-filter-quality-to-transform-widgets
branch
from
c749f4f to
09aeb23
Compare
4 participants
This new API is basically the culmination of the API discussions that ensued from this document with one potential modification described below. It introduces a new property on the Transform (and related) Widgets that allows a developer to request that a coordinate transform be substituted with a bitmap transform while at the same time controlling the quality/speed tradeoff of the bitmap operation.
Recently, there has been some discussion about the value of our current bitmap quality hint enumFilterQualitywhich is ongoing at #76737. As a result I think the final form for the API for this particular mechanism should have one more level of abstraction as we may move away from the FilterQuality enum and so tying the choice of transform approach to that one enum might not be the best approach.A slightly more abstract API would introduce a wrapper class, along the lines of:and then thefilterQualityproperty introduced by this PR would morph into atransformMethodproperty with a nearly identical implementation.If we later introduce new "sampling" controls, we could simply add another constructorTransformMethod.bitmapWithSampling(<SamplingType[s]>);After spending some time looking to add a more finely tunable set of Sampling controls to the Flutter API, I decided that the existing enum and the 4 values that it exposes (though poorly named) represented as much control over bitmap sampling as it would make sense to provide. As a result, the API as is already proposed by this PR of adding a (nullable)
FilterQualityproperty to various transform widgets would be enough to provide developers the control they need over the quality and performance of the transform operations.