Make ColorFilterLayer accept an offset #38055
Conversation
|
Gold has detected about 72 new digest(s) on patchset 2. |
|
Gold has detected about 37 new digest(s) on patchset 3. |
| void ColorFilterLayer::Preroll(PrerollContext* context) { | ||
| auto mutator = context->state_stack.save(); | ||
| mutator.translate(offset_); | ||
|
|
There was a problem hiding this comment.
You need to adjust the paint bounds accordingly. The paint_bounds are measured in the parent coordinate system (which used to be a transform layer so it would be the agent applying the offset to the child's bounds to create its own bounds). But now that this layer is doing its own translation, it needs to adjust the bounds it reports.
There was a problem hiding this comment.
Done, but I noticed that there is a difference in how the image filter layer and this layer behave w.r.t child bounds transformation. I assume that is because matrix filters or blur can increase the size of things beyond the offseT?
There was a problem hiding this comment.
The bounds should always be offset because the translation is always applied in Paint. There is a bug in image_filter_layer in that it doesn't offset the bounds if there is no filter. Do not replicate that bug here the way you have - and fix IFL while you're at it if you please.
The bounds need to match what is rendered (modulo we haven't been replicating the integral transform code into the layer Preroll methods, which has been a topic of discussion with @knopp as we migrate Diff to use the LayerStateStack).
But, this offset is always applied in Paint and should always be applied to the paint bounds in both CFL and IFL.
| void ColorFilterLayer::Preroll(PrerollContext* context) { | ||
| auto mutator = context->state_stack.save(); | ||
| mutator.translate(offset_); | ||
|
|
There was a problem hiding this comment.
The bounds should always be offset because the translation is always applied in Paint. There is a bug in image_filter_layer in that it doesn't offset the bounds if there is no filter. Do not replicate that bug here the way you have - and fix IFL while you're at it if you please.
The bounds need to match what is rendered (modulo we haven't been replicating the integral transform code into the layer Preroll methods, which has been a topic of discussion with @knopp as we migrate Diff to use the LayerStateStack).
But, this offset is always applied in Paint and should always be applied to the paint bounds in both CFL and IFL.
|
I've updated both image and color filter layers to always apply the offset in preroll and added unit tests for both cases, PTAL |
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
A minor fix and then we should work with the IFL PR below to make sure we manage translation during caching correctly. I only skimmed a couple of the files on this pass pending those eventual changes.
flow/layers/color_filter_layer.cc
Outdated
| : 0; | ||
| } else { | ||
| // else - we can apply whatever our children can apply. | ||
| set_paint_bounds(child_bounds); |
There was a problem hiding this comment.
This line is not conditional, why is it applied in both halves of the if?
| FML_DCHECK(needs_painting(context)); | ||
|
|
||
| auto mutator = context.state_stack.save(); | ||
| mutator.translate(offset_); |
There was a problem hiding this comment.
There is a bug in IFL that we are currently fixing where this line here causes the offset to be applied doubly when caching children. See #38567 on our discussion about how the interplay of offset/integralTransform/caching should play out.
There was a problem hiding this comment.
Should I leave this out?
There was a problem hiding this comment.
it sounds like from reading the issue this change here would introduce a raster caching bug. I'll put this on hold until 38567 has a solution
3 participants
Make ColorFilterLayer accept an offset so the framework can make the RO a repaint boundary.
See also: flutter/flutter#101941