Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jun 23, 2022

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

@skia-gold
Copy link

Gold has detected about 1 new digest(s) on patchset 4.
View them at https://flutter-engine-gold.skia.org/cl/github/34267

} else if (Platform.isIOS || Platform.isMacOS) {
return ffi.DynamicLibrary.open('libpathkit.dylib');
} else if (Platform.isAndroid || Platform.isLinux) {
return ffi.DynamicLibrary.open('libpathkit.so');
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not really)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

@dnfield dnfield changed the title Pathkit FFI start. Expose SkPathOps in a standalone library. Jul 7, 2022
@dnfield
Copy link
Contributor Author

dnfield commented Jul 7, 2022

  • Changed applyOp to return a new path
  • Renamed this to path_ops, because it's not really the same as the existing pathkit library Skia distributes.
  • Added some Dart based tests.

@dnfield dnfield marked this pull request as ready for review July 8, 2022 17:53
@dnfield dnfield requested a review from zanderso July 8, 2022 17:53
Copy link
Member

@zanderso zanderso left a 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a copyright header.

Copy link
Contributor Author

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 = () {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@jonahwilliams jonahwilliams left a 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)

@dnfield
Copy link
Contributor Author

dnfield commented Jul 8, 2022

@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).

@dnfield
Copy link
Contributor Author

dnfield commented Jul 11, 2022

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants