[Impeller] Reland of Simplify Advanced Blend color filters with a foreground color. #40927
Conversation
| auto dst_uvs = maybe_dst_uvs.value(); | ||
|
|
||
| auto size = rect_.size; | ||
| VertexBufferBuilder<VS::PerVertexData> vtx_builder; |
There was a problem hiding this comment.
From reading render_filter, it appears that the coverage rect we are given already has the CTM applied. So we have two choices here: either directly use the values for the coordinates (and don't apply it to the MVP - or invert the transform and likewise.
Of course, if you don't invert the transform and fix the coordinates, then the UVs come out wrong and that's why I've "Fixed" them below.
I'm not sure exactly why this worked before
There was a problem hiding this comment.
So there is some difference in a snapshot that is created by a contents, and a snapshot that is forwarded through from an entity pass. I'm tempted just to say, for this particular case, we should just set the UVs using the coverage rect, since that coverage rect already includes the CTM it is correct for both entity subpasses and for contents. But I'm still not really sure how I should be using the snapshot GetCoverageUVs directly which is concerning.
There was a problem hiding this comment.
I guess more of a question for @bdero, but what is the correct way to use snapshot.GetCoverageUVs ?
|
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. |
|
cc @bdero to take a closer look. We should hold on landing this (if we are) till past the branch cut. |
| // This does not use the snapshot transform, since this value will | ||
| // be different depending on whether the filter was applied via a | ||
| // saveLayer or directly to a particular contents. By construction, | ||
| // the provided rectangle should exactly map to the texture coordinates. |
There was a problem hiding this comment.
The transform of the snapshot needs to either be absorbed or deferred, otherwise this won't render correctly when the FilterInput decides to defer the transform and returns a non-identity matrix (TextureFilterInput or ContentsFilterInput holding a TextureContents sometimes defer their transforms to avoid unnecessary extra passes).
You can either absorb the snapshot transform by applying it to the positions (take the snapshot texture bounds and multiply by the transform) or draw the coverage rectangle and transform the texture coordinates instead (GetCoverageUVs computes the correctly transformed UVs for this).
Coverage rects and snapshot transforms computed by FilterInputs are already in screen space relative to the EntityPass texture position, and so the only difference between rendering into an inline subpass (current behavior of the blend filter) vs Contents::Render is that stuff rendered in a subpass needs to be shifted by the negative coverage origin (assuming the texture size is optimal), and then shifted back by pre-translating the transform in the returned Entity.
There was a problem hiding this comment.
Got it, I took another look at this and found that I was more or less applying the origin offset twice since I was still appending the transform shift to the entity transform - which explained the difference.
This now uses getSnapshotUVs and renders correctly in both scenarios.
| // to convert this back to a snapshot so it can be passed on. This generally | ||
| // implies that there is another filter about to run, so we'd perform this | ||
| // operation anyway. | ||
| auto potential_opacity = alpha.value_or(1.0) * dst_snapshot->opacity; |
There was a problem hiding this comment.
This seems like a reasonable thing to do?
There was a problem hiding this comment.
Yeah I think this makes sense, although it'll probably just end up getting absorbed right away due to no filter being chained most of the time. Maybe we can come up with a mechanism to reconcile this later.
There was a problem hiding this comment.
LGTM.
Could we add a couple of simple goldens (non-interactive playgrounds) that hit these new foreground paths by blending a translated+rotated image input? Maybe one with absorbed alpha and one without? We should lightly cover all of the paths here eventually, but might as well cover new branches as they get added.
| // to convert this back to a snapshot so it can be passed on. This generally | ||
| // implies that there is another filter about to run, so we'd perform this | ||
| // operation anyway. | ||
| auto potential_opacity = alpha.value_or(1.0) * dst_snapshot->opacity; |
There was a problem hiding this comment.
Yeah I think this makes sense, although it'll probably just end up getting absorbed right away due to no filter being chained most of the time. Maybe we can come up with a mechanism to reconcile this later.
|
Yes, for this and the other filters I'll go through and add some goldens first. |
|
Golden file changes are available for triage from new commit, Click here to view. |
|
I think a 5 pixel diff is probably fine |
jonahwilliams
added
the
autosubmit
…with a foreground color. (flutter/engine#40927)
…eground color. (flutter#40927) [Impeller] Reland of Simplify Advanced Blend color filters with a foreground color.
See previous revert: #40914
See original context in #40886
TLDR: the problem appears to be that my previous implementation only worked sort of by accident.