-
Notifications
You must be signed in to change notification settings - Fork 98
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
adding python set_properties #504
Conversation
bindings/python/manifold3d.cpp
Outdated
glm::vec3 v, | ||
const float *oldProps) { | ||
f(py::array(newNumProp, newProps), std::make_tuple(v.x, v.y, v.z), | ||
py::array(oldNumProp, oldProps)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting a compile error here that I don't understand. Am I missing something?
I guess I understand the problem here, but this is not a simple issue to solve. I think the best way would be to change the python API for set_properties (and warp as well) due to fundamental issue with lifetime management and efficiency concerns. The problem here is that py::array will and must perform copying when we give it a pointer, so the performance is slow due to many allocations and we need to copy the changes back. I think the simplest way would be to pass a large buffer to python (requires large copying), get the return value and copy it back to c++. This requires 2 copy operations, but avoids calling the slow python function many times, and avoids the lifetime issue. (sorry for accidentally closing the issue, I am on my phone now) Also, I just noticed that I should not be using vsplit in the python code, it should be [:, :3]. |
Ugh, that's too bad - I had thought the point of giving |
No it is not much worse comparing with WASM, we are also doing copying in our js binding or we will have similar issues regarding lifetime. We can do something similar, but I think the performance will likely be bad with Python. Yeah, I think skipping |
Hmm, I haven't noticed anything too awful about the performance of Warp or SetProperties on WASM. Of course that's partly because they're pretty simple, O(n) operations. Maybe we should just give this a try and see how big a problem it is? Even if it's 2x slower, it's not a particularly long operation. I'm also curious to what extent the compiler is able to optimize away the copies. If we did want to make this function work in Python so we could at least test its performance, do you know how? |
It should be pretty simple, just don't think about avoid copying and you are done, basically just the logic in our js code, but need to convert into python code. manifold/bindings/wasm/bindings.js Lines 220 to 245 in 59760b3
I can only do it this weekend, as I am in the middle of doing something else. |
Okay, sounds good - I'm working on the 3MF export stuff so if you get to it first, you're welcome to it. |
Finished, and I also updated our trimesh export example to export vertex colors. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 90.34% 90.36% +0.01%
==========================================
Files 35 35
Lines 4422 4431 +9
==========================================
+ Hits 3995 4004 +9
Misses 427 427 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this for me!
|
||
|
||
def posColors(pos, _): | ||
return [1 - p / 2 for p in pos] + [1.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the simplicity!
result -= hole.rotate([90, 0, 0]) | ||
result -= hole.rotate([0, 90, 0]) | ||
|
||
return result.trim_by_plane([1, 1, 1], 0).set_properties(4, posColors).scale(100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the new number of properties 4? It seems like it would be 3, and anything else would trigger your invalid shape error below? Did you check that the resulting GLB looks like our logo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trimesh expects rgba for vert_color (and max is 255, but I did the conversion in the export part)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my python is just weak - I totally missed that + [1.0]
was pushing an additional item into the array. Thanks!
* adding test of properties * python set_properties * fixed sponge example --------- Co-authored-by: pca006132 <[email protected]>
Adding our colored Menger Sponge example to verify that mesh properties are working properly. I'll also update our trimesh usage to write a colored GLB.