[Impeller] Tessellate paths directly using PathReceiver #166759
Conversation
|
This PR currently only does the filling side of things, but will eventually address stroking. I haven't checked clipping yet, but if it doesn't use FillGeometry directly, it can be converted easily as well. One planned side benefit for this work is that we should tessellate stroked paths directly from the PathReceiver to the widened tessellation without first generating a polygonalized version of the path, thus saving the intermediate allocation time and storage for the unwidened polygon. There is some incidental de-skiafication work here that was originally started because I was trying to minimize the footprint of the |
jonahwilliams
left a comment
There was a problem hiding this comment.
Approach LGTM, with some nits
We should monitor the path tessellation benchmarks to see if we get an improvement there (as we'd previously just moved the work around between ui/raster, now it should drop off)
| .SetBounds(rse.GetBounds()) | ||
| .TakePath(); | ||
| DrawPath(path, paint); | ||
| DrawPath(flutter::DlPath(path), paint); |
There was a problem hiding this comment.
Does this end up converting back to a Skia path (here and above)?
That is probably ok as a first step, but if we're going to end up converting a draw to a path we should do so while recording.
There was a problem hiding this comment.
We can iterate directly from either SkPath or impeller::Path. The only code that would force a conversion would be DlPath::GetPath() or DlPath::GetSkPath(). Skia will use GetSkPath() until we stop using Skia, but impeller can switch from GetPath to iterators to avoid any conversions.
There was a problem hiding this comment.
When I'm done with this work I may be able to delete DlPath::GetPath() -> impeller::Path.
|
None of this work had been done on UI I don't think. DL dispatch should have always been happening on raster thread. It's a question of removing conversion of the path description in favor of just interpreting the original description. I think for fills it should be pretty close, but there is the potential that I've introduced more layers of vtable dispatch somewhere. For strokes we can eliminate the intermediate un-widened polygonal storage, but again at the cost of more work being done on the fly inside virtual methods. |
|
I double-checked. ClipPath uses FillPathGeometry which is converted so we are using the new paradigm for both filled and clipped paths. |
|
This changed as a result of the dl_dispatcher change. I'm looking for the raster number to go back down. |
|
Marked it ready for review, but I'm still planning to add some more specific unit tests for the new code. |
|
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. |
|
The one golden diff is a mystery. I don't think I changed the way in which we tessellate the curves by this change so I'm not sure why we'd see "edge fuzziness" on the curves in that one test... |
|
Seems like a pretty harmless diff |
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
|
Any last looks? I triaged the golden diff and the unit tests are in now... |
|
I took another quick look ... LGTM! Ship itttttt |
Currently Impeller asks incoming paths to convert themselves into the impeller::Path class before tessellating their outlines. This wastes conversion time when the path is only needed to iterate the path segments. We now tessellate them directly from a path segment dispatcher saving the conversion cost.
Currently Impeller asks incoming paths to convert themselves into the impeller::Path class before tessellating their outlines. This wastes conversion time when the path is only needed to iterate the path segments. We now tessellate them directly from a path segment dispatcher saving the conversion cost.