add WarpBatch - array process warp with single callback #651
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #651 +/- ##
==========================================
+ Coverage 91.61% 91.66% +0.04%
==========================================
Files 36 37 +1
Lines 4724 4737 +13
==========================================
+ Hits 4328 4342 +14
+ Misses 396 395 -1 ☔ View full report in Codecov by Sentry. |
77b5cd8 to
36636c1
Compare
|
This looks good to me - would you mind adding a python example for this? Perhaps convert our Heart or Torus Knot examples? It would also be a good excuse to see a performance comparison between the two warp approaches, which it would be nice to record here. I'm also curious if we should do the same thing for our JS bindings. Though perhaps WASM has less overhead in that tight loop, but I'm not sure. @pca006132 this also seems related to some discussions we've had around SDF. Does this seem like a useful pattern there as well? |
|
SDF: #598 Yes this is also a useful pattern for SDF, but I think we have m
8000
ore things to consider for SDF in the previous PR. |
2e07a18 to
fb3a13d
Compare
|
I added torus_knot.py showing fully vectorized warp math. We're gonna convert you to a python lover :P
If you have the time, sketch it up and run some timings on torus knot with high segment numbers. I'm sure the JIT helps a lot but even fully native code is punished by high numbers of function-pointer calls. |
|
Is there a helper script somewhere to auto-format everything? I've been reverse-engineering the commands to run from the CI outputs, and it's a bit of a pain. |
|
Similar thing can help for js, the problem is that JIT is not good at optimizing across language boundaries. We don't have a script right now, but it is just using the black formatter for python files. |
|
On my laptop, using the exact same numpy warp code but running 1 point at a time (Manifold.warp) it takes ~800ms. Running warp_batch takes about 40-50ms. |
|
I think black is asking you to reformat |
|
Yeah, I think I have it figured out now. I just mean having to download black and learn how to use it based on failure reports. It was worse with clang-format - I figured out I needed to downgrade versions to get consistent with CI. A script would save some effort, especially for new people getting set up. |
|
Interesting, I thought clang-format should have consistent settings across versions. Agreed, we should pin the detailed format settings and provide a script to format everything. Can be addressed in another PR. |
|
I'd like to change the function signature to |
0f219ce to
69a4acf
Compare
514fb72 to
a25a28d
Compare
| CrossSection CrossSection::WarpBatch( | ||
| std::function<void(VecView<glm::vec2>)> warpFunc) const { | ||
| std::vector<glm::vec2> tmp_verts; | ||
| C2::PathsD paths = GetPaths()->paths_; // deep copy |
There was a problem hiding this comment.
If this line the deep copy, or the for loops following?
There was a problem hiding this comment.
I copy it up front, then use it to update in-place after the warp func. Mainly because it allocates all the right vector sizes in one simple line of code. Are you concerned with the wasted data-copy doing it this way? I could optimize it if you think the complexity is worth it.
There was a problem hiding this comment.
I think this is fine, I guess Emmett is just not sure about where the deep copy comment belongs to (the single line or including the loop after it).
There was a problem hiding this comment.
Yeah, it kind of looked like it was just a one-line deep copy followed by a loop deep copy - I'm still not quite sure why.
There was a problem hiding this comment.
Nevermind, I think I understand now. It's all good.
| std::vector<glm::vec2> tmp_verts; | ||
| C2::PathsD paths = GetPaths()->paths_; // deep copy | ||
| for (C2::PathD& path : paths) { | ||
| for (C2::PointD& p : path) { |
There was a problem hiding this comment.
Our style is to use const anywhere is possibly can be (mostly for readability). Mind taking a pass through the PR to constify it?
There was a problem hiding this comment.
I fixed these and don't see any more, but let me know if I missed any.
|
Regarding formatting, we generally use VSCode and their related extensions. I think that keeps things fairly synced, probably since MS also owns Github and Actions. What dev environment are you using? |
|
No, vscode extension only calls the clang-format in your environment, so microsoft does not control what formatter you use :) |
|
I'm on clang-format v17 on my mac and it's working fine. What was the problem version you had, @wrongbad? |
|
I was also using v17 on mac and it was formatting this line in a way that didn't pass the CI, but v11 was working for me. v17 does this |
|
Yes I also saw the same problem with clang-format 15. I think it changes when upgrading from 14 to 15. |
There was a problem hiding this comment.
Thanks! Minor nit - for ease of review I'd appreciate it if you refrain from force-pushing. I know lots of people use rebase to avoid merge commits, but we do a squash and merge anyway so it makes no difference. If we stick to regular merging, then we can more easily see what changed since the last review.
Add WarpBatch - array process warp with single callback
I introduce
WarpBatchfor bothManifoldandCrossSection.The new callback signature is
void(manifold::VecView<glm::vec3>)to allow for arbitrary contiguous storage. This moves the per-vertex tight-loop fully into the callback, where math vectorizations can now apply.This is especially helpful for python users where scalar math is vastly slower, but array math can use compiled libs like numpy.