Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Skia bindings #418

Merged
merged 11 commits into from
May 17, 2017
Merged

Add Skia bindings #418

merged 11 commits into from
May 17, 2017

Conversation

nornagon
Copy link
Contributor

This binds the mono/skia fork of Skia, which has expanded C bindings.

@saudet
Copy link
Member

saudet commented May 14, 2017

Great, thanks! But ↓ it doesn't seem like the fork is being maintained?

This branch is 347 commits ahead, 2114 commits behind google:master.

One of the reasons for using JavaCPP is to save time having to maintain such C wrappers. I'd like to see if we really can't map the C++ API, even if only partially at first. Could you make a quick list of pending issues so we can evaluate how much effort that might require?

BTW, it looks like they are not releasing versions, so let's do like we decided for Liquidfun in pull #356

@nornagon
Copy link
Contributor Author

Check out the update-master branch, which was touched 3 days ago and is < 100 commits behind upstream. The fork is also the basis of the SkiaSharp library for C#, which is a core project of the Mono team, and so seems likely to receive continued attention for the foreseeable future. Further, the fork expands on the existing C API in Skia, which isn't well maintained in upstream, but the Skia team has written that they intend for the C API to be their stable interface. I've also reached out to the maintainer of that fork to ask about efforts to merge their changes upstream.

If Skia still intends their C API to be their stable API, I think there's value in binding that API, even though the C++ API could potentially be bound directly.

Here's a diff from my efforts binding the C++ API of me commenting stuff out that wasn't working: https://gist.github.com/nornagon/d801cabc36fa33ff4cd275cd2eecbefd

I was trying to move pretty fast when building the binding, and with limited understanding of InfoMap (the docs could use some improving!), so I'm sure some of my difficulties were simply that I don't understand very well how JavaCPP works. I also didn't spend very much time trying to work out a root cause for a lot of the issues I came across, because I was just trying to get something to compile. Here's a vague, incomplete list of the problems I encountered:

  • I couldn't work out how to map SkNVRefCnt (or its friends, SkRefCnt and SkWeakRefCnt). See here for an example usage.
  • In SkData, the Parser also has trouble with the construction class Foo final : Bar {...}
  • Inner classes aren't correctly generated (e.g. SkDrawLooper::Context)
  • I couldn't figure out how to properly map opaque types like SkSpecialImage. I'm sure this is possible, though, just a matter of documenting how Info works.
  • Something wasn't working when mapping the GrPaint&& type, but I don't remember what.
  • static_assert doesn't seem to be supported by the parser.
  • In the generated C++ code, I was getting errors for every usage of unique_ptr. I didn't take the time to find a root cause.

@saudet
Copy link
Member

saudet commented May 15, 2017

I see, so it looks like this is going to become the official C API? That's a good start then. The C++ API sounds like it'll require some effort to get working alright.

For the version, the date is the date of the commit, so 20170511 in this case. Not the date we changed something in this repository. Also, let's use it in the "Introduction" of the README.md file.

Did you forget to add a build for Windows? We should either add the commands to build in cppbuild.sh, or remove "windows" from the @Platform list and not add it to .appveyor.yml either. Thanks!

@saudet
Copy link
Member

saudet commented May 16, 2017

There's also an issue building on Linux:
https://travis-ci.org/bytedeco/javacpp-presets/jobs/232196641
Do you think you could fix this as well? Thanks!!

@nornagon
Copy link
Contributor Author

I'm not sure if this precisely will become the official C API, but it does seem likely that a C API of some kind will be the stable ABI to Skia in the future.

I've switched to building a release instead of debug build, which should fix the linux build issue (hopefully). I've also added a windows build to the cppbuild.sh, but I'm not sure if there's anything else I'll need to do to get it building properly on Windows. We'll see what Appveyor has to say :)

@nornagon
Copy link
Contributor Author

Per https://skia.org/user/build, it looks like Skia only builds on Windows with Visual Studio 2015 Update 3, but I think the appveyor setup for javcpp-presets is set up to build with mingw/msys2. I'll leave the windows build problem for another time, but it looks like the linux and mac builds are working fine on travis!

@saudet
Copy link
Member

saudet commented May 17, 2017

Sounds good, others can add other platforms like Windows, etc at some later point in time when they need it :) We'll also just need to remove "windows" from @Platform(value = {"linux-x86", "macosx", "windows"}, ... to make sure it doesn't try to do anything on Windows for now though...

@nornagon
Copy link
Contributor Author

Done.

@saudet saudet merged commit bd963fc into bytedeco:master May 17, 2017
@nornagon nornagon deleted the skia branch May 17, 2017 01:58
@saudet
Copy link
Member

saudet commented May 28, 2017

FYI, thanks to @vb216, SNAPSHOT artifacts are now available here:
https://oss.sonatype.org/content/repositories/snapshots/org/bytedeco/javacpp-presets/skia/
Let us know if you have any issues with them and thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants