[DisplayList] DlPath object provides auto-conversion from Skia to Impeller #55055
Conversation
There was a problem hiding this comment.
mostly LGTM with question from offline discussion:
Do we have a path (ha ha) to moving the conversion to be eager rather than lazy? Could DlBuilders know "hey I'm running on Impeller, I'll just convert when I record it" et cetera.
For things like path ops, we only ever need to answer those questions with the dart:ui Object. So there may be a world where dart:ui still uses SkPath but we still eagerly convert on recording in the display list. And that lets us remove the Path volatility tracker which is basically a hack to make the mutable SkPaths stable
|
|
||
| private: | ||
| const SkPath sk_path_; | ||
| mutable impeller::Path path_; |
There was a problem hiding this comment.
does the default initialized impeller::Path end up constructing a shared_ptr even if its not used? Do we need to make this Optional?
There was a problem hiding this comment.
I'll look into it. I also want to see what happens when you copy construct these objects - does it share the underlying data at all? I believe SkPath will not share the object, but will share the vector data. Not sure about Impeller.
There was a problem hiding this comment.
IIRC PathRef should behave a bit like a shared_ptr
| return sk_path_.isInverseFillType(); | ||
| } | ||
|
|
||
| bool DlPath::IsRect(DlRect* rect, bool* is_closed) const { |
There was a problem hiding this comment.
I think we only actually care about closed rects right? This could handle checking is_closed internally.
There was a problem hiding this comment.
For fill it doesn't matter if it is closed, for stroke then you care.
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
We discussed the future plans offline, TLDR, yes, but more stuff needs to happen first.
LGTM with nits
flar
force-pushed
the
dl-path-object
branch
from
5e3163e to
e1ecffc
Compare
2 participants
Switch from using the clumsy manual CacheablePath object to a more automatic DlPath object for holding paths in DisplayLists and dispatching them to either Skia or Impeller with auto-conversion.
For now DlPath is just a wrapper around SkPath with an auto-generating Impeller Path object which is very similar in design from what was done with the CacheablePath object except that it manages the caching of the Impeller path internally without extra burden on Impeller or Skia. There is also no need to communicate with the Dispatch method as to which type of path you prefer, they're all "auto-converting" DlPath objects now.
For now, ui.Path still generates an SkPath and so we wrap it when we record it into a DisplayList, just like the former CacheablePath mechanism. It will be a simple conversion to create the DlPath wrapper in ui.Path, though, so as to maintain the cached Impeller paths across frames even if the DisplayList itself is not preserved.
Eventually DlPath will take on more of a role of hiding the construction and internal representation of the paths so that we could be using SkPath, impeller::Path, or some other internal storage. For now, SkPath will likely remain primary storage for a while so that we can deal with PathOps.