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 C bindings for Triangulate() #1034

Merged
merged 4 commits into from
Nov 16, 2024
Merged

Conversation

james-bern
Copy link
Contributor

@james-bern james-bern commented Nov 10, 2024

Here's my attempt at adding C bindings for Triangulate(). The code seems to work; but there are still a couple things I need help with.

  • Triangulate() returns a std::vector<ivec3>, which I tried to respect. Since ivec3 is 3 int's, I have manifold_triangulation_tri_verts(void *mem, ...) copy_data assume mem has size (num_tri * 3 * sizeof(int)).

  • NOTE: manifoldc.cpp now includes "manifold/polygon.h"

  • NOTE: I couldn't figure out manifold_destruct_triangulation(...); need some help on this.

    • Also, I don't understand the use case(s) of the destruct functions (literally don't understand; I am sure they exist). Help on this too would be great.
  • NOTE: I have not attempted to add bindings for TriangulateIdx()

  • Usage Code

    ManifoldTriangulation *triangulation = manifold_triangulate(manifold_alloc_triangulation(), polygons, epsilon);
    size_t num_tri = manifold_triangulation_num_tri(triangulation);
    int *tri_verts = (int *) manifold_triangulation_tri_verts(malloc(num_tri * 3 * sizeof(int)), triangulation);
    manifold_delete_triangulation(triangulation);
  • Test

    • NOTE: I hacked this into my current codebase. I took the polygons I was already getting from ManifoldPolygons *polygons = manifold_cross_section_to_polygons(manifold_alloc_polygons(), cross_section);, triangulated them as in the usage code above, and drew them as outlined triangles.
    Screenshot 2024-11-10 at 8 13 30 AM

* Triangulate() returns a std::vector<ivec3>, which I tried to respect.
  Since ivec3 is 3 int's, I have
  manifold_triangulation_tri_verts(void *mem, ...)
  copy assuming mem has space for 3 * num_tri int's

* NOTE: manifoldc.cpp now includes "manifold/polygon.h"

* NOTE: I couldn't figure out manifold_destruct_triangulation(...)
  Also, I don't understand the use case(s) of the destruct functions
  (literally don't understand; I am sure they exist)

* Usage code:

  ```
  ManifoldTriangulation *triangulation = manifold_triangulate(manifold_alloc_triangulation(), polygons, epsilon);
  size_t num_tri = manifold_triangulation_num_tri(triangulation);
  int *tri_verts = (int *) manifold_triangulation_tri_verts(malloc(num_tri * 3 * sizeof(int)), triangulation);
  manifold_delete_triangulation(triangulation);
  ```
Copy link

codecov bot commented Nov 10, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.69%. Comparing base (d437097) to head (bba18e8).
Report is 152 commits behind head on master.

Files with missing lines Patch % Lines
bindings/c/manifoldc.cpp 66.66% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
- Coverage   91.84%   84.69%   -7.15%     
==========================================
  Files          37       62      +25     
  Lines        4976     9682    +4706     
  Branches        0     1049    +1049     
==========================================
+ Hits         4570     8200    +3630     
- Misses        406     1482    +1076     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@james-bern james-bern changed the title Adds C bindings for Triangulate() Add C bindings for Triangulate() Nov 10, 2024
Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

This C memory management stuff is not my cup of tea. @pca006132 can you review, or perhaps @geoffder if you happen to have a spare moment while your daughter is asleep?

ManifoldTriangulation *manifold_triangulate(void *mem, ManifoldPolygons *ps,
double epsilon) {
auto triangulation = manifold::Triangulate(*from_c(ps), epsilon);
return to_c(new (mem) std::vector<ivec3>(triangulation));
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't auto already just std::vector<ivec3>?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For in-place new I think you do need this to specify the type, it is not a type cast.

Copy link
Collaborator

@pca006132 pca006132 left a comment

Choose a reason for hiding this comment

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

I think you should also add a simple test case in test/manifoldc_test.cpp to use the API. Just your example usage code would be great.

@pca006132
Copy link
Collaborator

Regarding destruct, the thing to keep in mind is that you can have two separate regions of memory: One managed by C/C++ and one managed by the alternative host language, e.g. Java heap. The in-place new syntax (new (mem) ...) initializes the data structure in the provided buffer mem, but the buffer is allocated separately. Users can either use our manifold_alloc_* API, which allocates from the heap managed by C/C++, or use a buffer provided by their host language runtime.

When you destroy the object, it should be done in two steps: 1) call the destructor to drop the internal pointers; 2) free the buffer. When you use the manifold_delete_* API, we call C++ delete, which does the two steps for us. However, this is invalid when the buffer is user provided instead of managed by C/C++ runtime, and in that case the user should just call the manifold_destruct_* API, and manually free the buffer. Note that the internal pointers are pointing to regions managed by the C/C++ runtime, so we don't have to worry about those. As to why the outer object resides in the user buffer... I am not the designer, and we should probably ask @geoffder :)

@james-bern
Copy link
Contributor Author

Regarding destruct, the thing to keep in mind is that you can have two separate regions of memory: One managed by C/C++ and one managed by the alternative host language, e.g. Java heap. The in-place new syntax (new (mem) ...) initializes the data structure in the provided buffer mem, but the buffer is allocated separately. Users can either use our manifold_alloc_* API, which allocates from the heap managed by C/C++, or use a buffer provided by their host language runtime.

When you destroy the object, it should be done in two steps: 1) call the destructor to drop the internal pointers; 2) free the buffer. When you use the manifold_delete_* API, we call C++ delete, which does the two steps for us. However, this is invalid when the buffer is user provided instead of managed by C/C++ runtime, and in that case the user should just call the manifold_destruct_* API, and manually free the buffer. Note that the internal pointers are pointing to regions managed by the C/C++ runtime, so we don't have to worry about those. As to why the outer object resides in the user buffer... I am not the designer, and we should probably ask @geoffder :)

Hi @pca006132 , Thank you so much for taking the time to explain destruct vs. delete to me!--I fully understand it now. Sorry I didn't manage to get the changes you requested added in a timely fashion (and thank you for taking care of them!)--I've been traveling and unable to do any real coding.

@pca006132
Copy link
Collaborator

Yeah, this is fine, I worked on it because we want to push for v3.0, normally we will wait a bit more :)

@pca006132 pca006132 merged commit ee32b30 into elalish:master Nov 16, 2024
20 checks passed
@elalish elalish mentioned this pull request Nov 16, 2024
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.

3 participants