C bindings to facilitate static FFI bindings #292
Conversation
Squashed commit of the following: commit a05f9b0 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Dec 10 16:21:47 2022 -0800 Add flag (default on) for python bindings Signed-off-by: Geoff deRosenroll <geoffderosenroll@gmail.com> commit ce5b601 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Dec 9 23:45:29 2022 -0800 conv function naming consistency commit 1b50cf0 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Dec 6 17:42:18 2022 -0800 manifold_level_set_seq to C API (force sequential) commit 77e06da Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Dec 6 17:24:10 2022 -0800 Use an optional executionpolicy param in LevelSet This allows bindings to avoid breaking their runtime locks while passing in function pointers without introducing another compile flag. commit 12c62b1 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Dec 5 15:09:41 2022 -0800 Better section comments commit 24f1d51 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Dec 2 12:01:50 2022 -0800 Add missing management functions for Components commit a7d5aae Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Thu Dec 1 23:45:45 2022 -0800 Break Decompose up to avoid repetition in C binds commit a650a15 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 27 22:41:38 2022 -0800 Add manifold_get_properties commit 97a5839 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 26 22:53:42 2022 -0800 circular segment functions corrected to static commit c9c1ca7 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Thu Nov 24 21:19:11 2022 -0800 polygons from dbl ptr of SimplePolygon commit 8e5390a Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Wed Nov 23 12:16:44 2022 -0800 compose and decompose switched to double ptrs commit 45c78c3 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 22 11:40:32 2022 -0800 Add manifold copy and empty constructors commit cfc06fe Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 21 15:03:00 2022 -0800 Add MANIFOLD_SEQ_SDF to avoid runtime lock commit e0f9d09 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 20 16:56:50 2022 -0800 Add missing export related contructors. commit 0cbaa4a Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 20 16:03:43 2022 -0800 Add separate destruct functions commit fd6c1c2 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 17:29:12 2022 -0800 More consistent array handling commit ec1eef8 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 16:52:07 2022 -0800 Replace array args with pointers commit 9079cbf Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 15:00:47 2022 -0800 Revert "Switch back to funptr struct args for simplicity" This reverts commit dc78841. commit 62c1078 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 14:52:37 2022 -0800 Switch back to funptr struct args for simplicity commit d30992d Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 14:06:12 2022 -0800 Avoid needing to pass in vec struct for funptrs commit 2ba7bec Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 00:32:09 2022 -0800 Use template spec to avoid fun ptr to cuda commit 1faf58b Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 21:54:56 2022 -0800 Make fPIC more conditional. commit 6215f89 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 21:35:09 2022 -0800 Avoid passing dynamic lambda to GPU commit 30fa3dc Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 18:02:04 2022 -0800 note and temporary hack RE cuda compilation commit bb4dfe9 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 17:21:25 2022 -0800 Set graphlite library to OBJECT commit e803da5 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 16:04:28 2022 -0800 Convert sublibraries to OBJECT to avoid archives commit d0e9d17 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 15:28:15 2022 -0800 Add missing polygon size and deletion funcs commit 7db7284 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 15 22:26:17 2022 -0800 Remove static flag (control with shared flag) commit 5ae33f3 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 15 22:16:27 2022 -0800 Add options to avoid python bindings commit 4181a58 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 22:02:10 2022 -0800 Rename to manifoldc commit c4b14d6 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 21:51:45 2022 -0800 Cleanup unused typedefs and simplify names commit a282c90 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 21:23:07 2022 -0800 Placeholder (terrible) decompose binding commit 9409e81 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 20:37:02 2022 -0800 Turn on export in CLI instead commit 9dad24b Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 20:33:17 2022 -0800 "Rebased" WIP cbindings
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Thanks! Hopefully the CLA is no big deal? Any thoughts on what it'll take to get it passing our CI? This is great, but my main concern is it's a lot of code that'll need to be updated as other developments happen. What do you think is the best way to go about this? For instance, #290 is steadily growing in scope. |
|
Oh weird, I thought fPIC was on for cases that it needed to be, but seems like it should be on any time building with shared libs at all (which my conditional doesn't catch). I should be able to address that later today, hopefully that's the only thing blocking the CI. As far as updating to changes, I started on this knowing that it'd have to evolve along with Manifold, so I can help to keep it up to date (feel free to tag me if it needs updating along with some change). With the existing base and patterns established, I don't think that the amount of effort to adjust to breaking changes or new additions will be too bad (e.g. adding/changing functions for extracting fields from one of the opaque pointer types like ManifoldMeshGL). Do you guys foresee anything that you think might be a problem in particular, or something we should do to make staying on top of it easier? Adding a CI build that includes the C bindings would make sure there is a reminder to keep the relevant bits up to date. |
|
Ah it appears |
|
I don't have any problem with pushing us up to C++17. If it helps to update the Ubuntu version of our CI, that's fine too. |
|
I think we were already using C++17 somewhere in the code, but the public API is C++14, so changing the version should not require updating Ubuntu. I think it would be better if we can reduce the size of this C binding, I think it can be reduced considering most of them are merely converting the fields to the C equivalent one, perhaps we can do some template magic here. (haven't look at it in details yet) |
|
After reading this a bit more carefully, it seems that it is hard to simplify this by using templates. For the API design I guess we can discuss about it later when there are other users of this C binding? I am not using the binding so it is hard for me to think about the API design. |
I was using a template for the array conversion helper initially for example, but then I rewrote without it since there were only a couple instantiations anyway.
For an idea of how these are used you can look at my C stub generation descriptions, and one of the modules using them. Each language will have it's own quirks in interfacing with C of course, but I think that a lot of this pattern is preserved. type t = C.Types.Manifold.t Ctypes_static.ptr
let size = C.Funcs.manifold_size () |> size_to_int
let destruct t = C.Funcs.destruct_manifold t
let alloc () =
let finalise = Mem.finaliser C.Types.Manifold.t destruct in
let buf = Mem.allocate_buf ~finalise size in
buf, Ctypes_static.(Ctypes.coerce (ptr void) (ptr C.Types.Manifold.t) buf)
let add a b =
let buf, t = alloc () in
let _ = C.Funcs.manifold_union buf a b in
tTaking pre-allocated memory as an argument makes the process of ensuring it is registered with the host language properly fairly simple. For a bit more complicate of an example using the additional machinery around let decompose t =
let comps_buf, comps = Components.alloc () in
let _ = C.Funcs.manifold_get_components comps_buf t in
let len = size_to_int @@ C.Funcs.manifold_components_length comps in
let bufs = Ctypes.(CArray.make (ptr void) len)
and ts = ref [] in
for i = 0 to len - 1 do
let buf, man = alloc () in
Ctypes.CArray.set bufs i buf;
ts := man :: !ts
done;
let _ = C.Funcs.manifold_decompose (Ctypes.CArray.start bufs) t comps in
!tsFor a non-ocaml version of following this kind of C API binding scheme, there are the olm python bindings, which as I mentioned in the discussion, is the library I used as an example. Minus the object-orientation, I used these official python bindings as a guide when I wrote mine in ocaml. More people working on binding through these from other languages would certainly help to catch any rough edges that remain. They had to get a handful of fixes as I worked through them myself, like switching all arrays to pointers (including the dbl ptrs for the opaque types). |
|
|
Nice work! The Assimp stuff is not core to library, so feel free to hack in whatever's necessary to make it function. The |
TODO: investigate why this is failing on Mac, but not other backends.
|
I tried adding the I opened #293 to test whether the std version bump is responsible, but it passed. |
Codecov ReportBase: 91.37% // Head: 91.24% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #292 +/- ##
==========================================
- Coverage 91.37% 91.24% -0.14%
==========================================
Files 33 33
Lines 3490 3505 +15
==========================================
+ Hits 3189 3198 +9
- Misses 301 307 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
test/mesh_test.cpp
Outdated
| @@ -1053,7 +1053,8 @@ TEST(Boolean, Subtract) { | |||
| first.GetMesh(); | |||
| } | |||
|
|
|||
| TEST(Boolean, Close) { | |||
| // FIXME: test is failing on Mac CI (passing on others) | |||
| TEST(Boolean, Close_DISABLED) { | |||
There was a problem hiding this comment.
Sorry, meant DISABLED_Close - I always get that wrong...
There was a problem hiding this comment.
Ok that did the trick, test is being skipped so the mac build is "passing". 👍
| #ifndef AI_MATKEY_ROUGHNESS_FACTOR | ||
| #include "assimp/pbrmaterial.h" | ||
| #define AI_MATKEY_METALLIC_FACTOR \ | ||
| AI_MATKEY_GLTF_PBRMETALLICROUGHNESS_METALLIC_FACTOR |
There was a problem hiding this comment.
This is looking pretty good to me; mostly questions. What do you think, @pca006132?
.github/workflows/manifold.yml
Outdated
| @@ -35,7 +35,7 @@ jobs: | |||
| git apply thrust.diff | |||
| mkdir build | |||
| cd build | |||
| cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DMANIFOLD_DEBUG=ON -DMANIFOLD_PAR=${{matrix.parallel_backend}} -DMANIFOLD_USE_CUDA=${{matrix.cuda_support}} .. && make | |||
| cmake -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DMANIFOLD_DEBUG=ON -DMANIFOLD_PYBIND=ON -DMANIFOLD_CBIND=ON -DMANIFOLD_EXPORT=ON -DMANIFOLD_PAR=${{matrix.parallel_backend}} -DMANIFOLD_USE_CUDA=${{matrix.cuda_support}} .. && make | |||
There was a problem hiding this comment.
How do you want to handle maintenance of the C bindings? I feel like we should have some CI testing, at least ensuring it still compiles, but on the other hand I'd also like to not block PRs on it so you can update it on your own time. Perhaps another job that we can mark as not required? That way at least it'll be easy to see.
There was a problem hiding this comment.
Having them only turned on for one build (maybe one with Par and CUDA enabled) and not failing CI sounds good to me. I just flipped it on everywhere to make sure they were working.
There was a problem hiding this comment.
Okay, I think this is the only part left to do for this PR, yes? I'm happy to merge once this is done.
There was a problem hiding this comment.
I've broken out the building/testing of the C bindings into a separate job so that it can be marked as not required easily in your branch protection rules.
|
Sorry I am not able to review this thoroughly as I just caught the flu, but it seems that the bindings are nicely written and will not affect the main code (except the weird macos error), I think this looks good to me. |
|
Started up a test file for the C bindings: 5dd8309 |
There was a problem hiding this comment.
Thanks, this looks good to me; I look forward to seeing what you do with it! Progress has been a bit slow on #290, but it's getting there and there will be a couple of follow-ons to finish updating the API. Once all that lands, you'll probably want to look into updating these bindings accordingly.
|
Ok sounds good. 🙏 Thanks for providing the library and being open to this addition! |
* + c bindings to facilitate static FFI bindings Squashed commit of the following: commit a05f9b0 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Dec 10 16:21:47 2022 -0800 Add flag (default on) for python bindings Signed-off-by: Geoff deRosenroll <geoffderosenroll@gmail.com> commit ce5b601 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Dec 9 23:45:29 2022 -0800 conv function naming consistency commit 1b50cf0 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Dec 6 17:42:18 2022 -0800 manifold_level_set_seq to C API (force sequential) commit 77e06da Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Dec 6 17:24:10 2022 -0800 Use an optional executionpolicy param in LevelSet This allows bindings to avoid breaking their runtime locks while passing in function pointers without introducing another compile flag. commit 12c62b1 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Dec 5 15:09:41 2022 -0800 Better section comments commit 24f1d51 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Dec 2 12:01:50 2022 -0800 Add missing management functions for Components commit a7d5aae Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Thu Dec 1 23:45:45 2022 -0800 Break Decompose up to avoid repetition in C binds commit a650a15 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 27 22:41:38 2022 -0800 Add manifold_get_properties commit 97a5839 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 26 22:53:42 2022 -0800 circular segment functions corrected to static commit c9c1ca7 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Thu Nov 24 21:19:11 2022 -0800 polygons from dbl ptr of SimplePolygon commit 8e5390a Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Wed Nov 23 12:16:44 2022 -0800 compose and decompose switched to double ptrs commit 45c78c3 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 22 11:40:32 2022 -0800 Add manifold copy and empty constructors commit cfc06fe Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 21 15:03:00 2022 -0800 Add MANIFOLD_SEQ_SDF to avoid runtime lock commit e0f9d09 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 20 16:56:50 2022 -0800 Add missing export related contructors. commit 0cbaa4a Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sun Nov 20 16:03:43 2022 -0800 Add separate destruct functions commit fd6c1c2 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 17:29:12 2022 -0800 More consistent array handling commit ec1eef8 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 16:52:07 2022 -0800 Replace array args with pointers commit 9079cbf Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 15:00:47 2022 -0800 Revert "Switch back to funptr struct args for simplicity" This reverts commit dc78841. commit 62c1078 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 14:52:37 2022 -0800 Switch back to funptr struct args for simplicity commit d30992d Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 14:06:12 2022 -0800 Avoid needing to pass in vec struct for funptrs commit 2ba7bec Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Sat Nov 19 00:32:09 2022 -0800 Use template spec to avoid fun ptr to cuda commit 1faf58b Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 21:54:56 2022 -0800 Make fPIC more conditional. commit 6215f89 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 21:35:09 2022 -0800 Avoid passing dynamic lambda to GPU commit 30fa3dc Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 18:02:04 2022 -0800 note and temporary hack RE cuda compilation commit bb4dfe9 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 17:21:25 2022 -0800 Set graphlite library to OBJECT commit e803da5 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 16:04:28 2022 -0800 Convert sublibraries to OBJECT to avoid archives commit d0e9d17 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Fri Nov 18 15:28:15 2022 -0800 Add missing polygon size and deletion funcs commit 7db7284 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 15 22:26:17 2022 -0800 Remove static flag (control with shared flag) commit 5ae33f3 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Tue Nov 15 22:16:27 2022 -0800 Add options to avoid python bindings commit 4181a58 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 22:02:10 2022 -0800 Rename to manifoldc commit c4b14d6 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 21:51:45 2022 -0800 Cleanup unused typedefs and simplify names commit a282c90 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 21:23:07 2022 -0800 Placeholder (terrible) decompose binding commit 9409e81 Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 20:37:02 2022 -0800 Turn on export in CLI instead commit 9dad24b Author: Geoff deRosenroll <geoffderosenroll@gmail.com> Date: Mon Nov 14 20:33:17 2022 -0800 "Rebased" WIP cbindings * Turn on -fPIC whenever shared libs are being built * Add parens to disambiguate sdf policy logic * Bump public std from 14 to 17 * Add binding flags to workflow builds * Add export when building cbindings * Bump ubuntu version (PBR material deprecation) * Select no-param overload of Decompose * Add missing const to decompose sig overload * Re-organize * variable capitalization consistency * Drop to std14 to test for regression fix * Attempted old libassimp compatibility hack * Rebump to std_17 * Temporarily disabled TODO: investigate why this is failing on Mac, but not other backends. * Remove unused tuple include * Remove uneccesary auto-includes * Properly disable Boolean_Close * rename discrete -> numComponents + early return * Rename discrete -> numComponents * Rename conv.hh -> conv.h * Prune auto-includes * Note on level_set vs level_set_seq * Set export on for c-bindings * Adding C binding tests. * Separate and don't require CBIND build/test * Remove bad flag * Take const char ptr for filename * Remove testc executable * Add tests from removed testc
As discussed, I've written C bindings to facilitate bindings through FFI from static compiled languages. I've been writing bindings to Manifold from OCaml using these bindings to make sure that they work as intended. They aren't polished and fully documented yet, but the API is covered and integration with the garbage collector seems correct.
The changes mostly live under bindings/c, but the Manifold build/source/API has been touched in a few places, namely:
OBJECTand thefPICflag is used when building thecbindings to allow more painless relocation of the archives/objects that is required for statically bundling them into executables in binding host languages (ran into this building the OCaml bindings)ExecutionPolicyis avoided when the sdf parameter toLevelSetis not a static lambda (results in runtime failure)ExecutionPolicyhas been added toLevelSetto allow languages with thread/interpreter locks to avoid parallel access to the sdf closureDecomposehas been broken up to allow thecbindings to know how manyManifoldallocations will be needed in advance without needing to repeat the expensive part of the method.