-
Notifications
You must be signed in to change notification settings - Fork 6k
Expose SkPathOps in a standalone library. #34267
Conversation
|
Gold has detected about 1 new digest(s) on patchset 4. |
tools/pathkit/dart/lib/pathkit.dart
Outdated
| } else if (Platform.isIOS || Platform.isMacOS) { | ||
| return ffi.DynamicLibrary.open('libpathkit.dylib'); | ||
| } else if (Platform.isAndroid || Platform.isLinux) { | ||
| return ffi.DynamicLibrary.open('libpathkit.so'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use wasm to avoid having to ship platform-specific libraries? I'm not sure what the wasm support level is like in the Dart VM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made Dan promise he wouldn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not really)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory yes, but we'd need to ship a wasm runtime to use it and it probably won't perform quite as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Skia already has a WASM version of this that they publish called "PathKit" on NPM)
|
zanderso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed the logic, but the overall construction lgtm w/ a couple nits.
| @@ -0,0 +1,35 @@ | |||
| import("//flutter/shell/version/version.gni") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a copyright header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| // TODO(dnfield): Figure out where to put this. | ||
| // https://github.com/flutter/flutter/issues/99563 | ||
| final ffi.DynamicLibrary _dylib = () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that it is more idiomatic to import (and not re-export) the stuff from here down from a library under lib/src.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all private to the library - but I can move it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with importing it is that people could then do source imports, which I'd like to avoid. I'm not sure which is better to handle here.
This is a basic namespacing/visibility problem in Dart though and maybe I just need to accept it? I know in the framework we don't trust people to not do source imports.
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you intended to use this Dart library? (I cannot think of any reasonable way it could be imported besides path imports - unless there is a separate step that updates to pub)
|
@jonahwilliams - it's basically the same as the tessellator one. In that case I think we more or less copy-pasted the Dart portion directly into vector_graphics_compiler. Ideally, we'd have a way to publish FFI based libraries to pub that do not require developers to build from source (since the build system here is not CMake and building this library would take a non-trivial amount of time for "normal" developer machines without GOMA). |
|
I'm going to land this and we can figure out the right way to consume this as we go. I think worst case we can copy/paste the Dart library, or distribute it via the Flutter SDK if we don't have a good way to distribute it via pub. |
Exposes a simple interface to support a subset building an SkPath and applying SkPathOps to it.
This is useful to figure out things like overdraw and doing offline optimizations without depending on dart:ui.
@c-h-i-a-m-a-k-a-2 @jonahwilliams fyi