[Impeller] Make matrix image filter work as expected #36193
Conversation
bdero
left a comment
There was a problem hiding this comment.
Thanks! Just minor comments. I think this solves all of the lingering problems -- including the fact the matrix is only supposed to apply to the geometry after the filter is rendered rather than before.
| return std::nullopt; | ||
| } | ||
|
|
||
| auto transform = GetTransform(entity.GetTransformation()).Invert(); |
There was a problem hiding this comment.
FilterContents already uses GetTransform to apply the local matrix before passing the entity into RenderFilter and GetFilterCoverage, so the entity transform should be used directly rather than passing them through GetTransform in these methods.
| auto transform = GetTransform(entity.GetTransformation()).Invert(); | ||
| transform = matrix_ * transform; | ||
| transform = GetTransform(entity.GetTransformation()) * transform; | ||
| snapshot->transform = transform * snapshot->transform; |
There was a problem hiding this comment.
Here and below: Can we throw this arithmetic on one line to make the multiplication order easier to read?
snapshot->transform = entity.GetTransformation() * matrix_ * entity.GetTransformation().Invert() * snapshot->transform;
| const Matrix& effect_transform, | ||
| const Rect& coverage) const { | ||
| return inputs[0]->GetSnapshot(renderer, entity); | ||
| auto snapshot = inputs[0]->GetSnapshot(renderer, entity); |
There was a problem hiding this comment.
The prior implementation rendered out the child filter at the final on-screen bounds resolution. This implementation no longer does so -- which I believe is correct: https://fiddle.skia.org/c/6cbb551ab36d06f163db8693972be954. I'm not sure if you intended to fix this particular issue, but thought I'd mention that you incidentally fixed it. :)
fix flutter/flutter#111737
without patch
with patch
Pre-launch Checklist
writing and running engine tests.
///).