Support nested clips & clip state restoration #14
Conversation
bdero
force-pushed
the
bdero/nested-clips
branch
from
4ef07bf to
4d9171b
Compare
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Awesome. Thanks! There is a drive by comment about a separate optimization but please do not rework this patch to account for that API issue. We'll fix it separately.
| // Clip restoration pipeline. | ||
| { | ||
| auto clip_pipeline_descriptor = | ||
| clip_pipelines_[{}]->WaitAndGet()->GetDescriptor(); |
There was a problem hiding this comment.
Just a drive by comment that you absolutely should not account for here because the right™ fix is account for the issue with having to wait for a pipeline future just to get its descriptor it to rework pipeline futures.
A restore pipeline is a variant of the clip pipeline which is a variant of the solid fill pipeline. And you have already awaited for the fill pipeline so you already have the descriptor. You could reuse that descriptor directly instead of awaiting the full initialization of the clip pipeline to prepare the restore variant.
Either way though, this is correct and an issue with the API forcing an await for a descriptor when it should only do so for the actual pipeline itself.
There was a problem hiding this comment.
Consider filing an issue and writing a TODO for follow-up work.
There was a problem hiding this comment.
I filed flutter/flutter#98684 specifically for this issue. Backfilling the issues in the bug tracker now (adding the impeller label). Will update the TODOs to reference the bugs once all issues are filed.
There was a problem hiding this comment.
Ah yup, makes sense and sgtm. I added a TODO referencing that issue nearby.
| void Canvas::RestoreClip() { | ||
| Entity entity; | ||
| entity.SetTransformation(GetCurrentTransformation()); | ||
| entity.SetPath({}); |
There was a problem hiding this comment.
Can you add a comment here that the the path is empty because it clear the entire render target? Just for clarity.
3 participants
Fixes flutter/flutter#98631.
Restoration works with a single draw call: