Add C-binding functions to access the normalIdx parameter of GetMeshGL/GetMeshGL64#1494
Conversation
…ming normals with GetMeshGL()/GetMeshGL64().
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1494 +/- ##
==========================================
+ Coverage 91.79% 92.05% +0.25%
==========================================
Files 35 35
Lines 6023 6039 +16
==========================================
+ Hits 5529 5559 +30
+ Misses 494 480 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm also looking at a way to write to So I wonder if it would be best to just add e.g: void manifold_meshgl_set_run_index(ManifoldMeshGL* m, uint32_t* runIndex, size_t length);
void manifold_meshgl_set_run_original_id(ManifoldMeshGL* m, uint32_t* runOriginalID, size_t length);
void manifold_meshgl_set_merge_verts(ManifoldMeshGL* m, uint32_t* mergeFromVert, uint32_t* mergeToVert, size_t length);(plus accompanying meshgl64 versions, which would be identical) I don't know how strict the semantics on the C api are so I'd like your thoughts on this one |
|
I don't have much opinion about this, it sounds ok to me. @geoffder what do you think? |
|
Could we make a |
In this case I assume non-set optional fields would just be left as null pointers, so it's not super different from just adding a big constructor function with all the options in it and allowing nullptr arguments, other than the conciseness of the API although I don't know how many people are actualy using the C API directly without some sort of binding. I can implement that either way on this PR or open a separate one if you want |
|
Yeah, I think your struct version looks good. I like this because it's both more convenient to use, and it's easy for us to extend without breaking the API. Let's merge it in here. |
I'll merge it in here, although I would note that if the struct was extended in the future this could be potentially breaking if a newer dll is used with a program that was compiled for the pre-extension struct, since the API consumer is going to be allocating the options struct at the old smaller size, and the C-bindings would then read past it into undefined memory. |
|
Ugh, I hate C. I really hope that's not a use case - trying to upgrade a dependency without recompiling? What if we changed the order of an enum or something? Feels very fragile... |
To be honest, I don't think that ABI compatibility is essential if you ship the bindings with headers, unless it's something that you guarantee. If you want to be extra sure I could add a function to get the size of the options struct, so that programs can armour against it growing in the future. |
elalish
left a comment
There was a problem hiding this comment.
Thanks! Just a couple small things.
| uint32_t* merge_from_vert; | ||
| uint32_t* merge_to_vert; | ||
| size_t merge_verts_length; | ||
| float* halfedge_tangents; |
There was a problem hiding this comment.
Do these need default values so that one doesn't have to specify all of them?
There was a problem hiding this comment.
IIRC C doesn't have default values. One way is to provide a function that returns the default value of the struct.
| size_t n_verts, size_t n_props, | ||
| uint32_t* tri_verts, size_t n_tris, | ||
| float* halfedge_tangent); | ||
| ManifoldMeshGL* manifold_meshgl_w_options(void* mem, float* vert_props, |
There was a problem hiding this comment.
with this function there's no longer a need for manifold_meshgl_w_tangents. I'm tempted to just remove it, but I suppose deprecating would be better.
This functionality was not previously accessible via the C API and can be exposed via two new functions. Only new functions are added and no existing functions were changed, to not break FFI compatibility. Names chosen were
manifold_get_meshgl_w_normalsandmanifold_get_meshgl64_w_normals, to match the naming established bymanifold_meshgl64_w_tangents. I am unaware of any documentation for the C api but if this exists I can update it too.