Move Skia conversion utilities to own TU #40997
Conversation
Bumps up some size assumptions to accomodate larger paths. Fixes bugs in the polyline shader that were highlighted by processing larger more complicated paths. Adds a test for a larger more complex path. Adds a playground for testing arbitrary SVG-encoded paths.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
lib/ui/BUILD.gn
Outdated
| deps += [ "//flutter/impeller" ] | ||
| deps += [ | ||
| "//flutter/impeller", | ||
| "//flutter/impeller/display_list", |
There was a problem hiding this comment.
I think we should turn the skia_conversions to its own target instead of making ui have to depend on all of display_list. Otherwise looks good to me.
|
Filed flutter/flutter#124455 that I noticed while looking this over. I'll take care of it as a separate bookkeeping PR when these go in as it has no impact on functionality or performance. |
|
@gaaclarke it looks like this PR is blocked by your review even though @dnfield addressed the issues. |
I think I've addressed this - happy to follow up if there was something I missed.
Move Skia conversion utilities to own TU
Move Skia conversion utilities to own TU
These utilities are useful in tests and it would otherwise be tempting to duplicate the logic of them in other TUs. It also helps make it a little clearer which parts of Impeller are talking to Skia types without DL interfaces right now, for better or worse.
This is split from #40975 and has a test in there.