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

WASM warp #221

Merged
merged 3 commits into from
Sep 27, 2022
Merged

WASM warp #221

merged 3 commits into from
Sep 27, 2022

Conversation

elalish
Copy link
Owner

@elalish elalish commented Sep 27, 2022

Fixes #197 (well, it's mostly fixed by earlier PRs)

This works! In the editor I tried this, which clearly created a squashed ball:

const ball = sphere(60, 100);
const func = (v: Vec3) => {
    v[2] /= 2;
};
const result = ball.warp(func);

@elalish elalish self-assigned this Sep 27, 2022
@@ -72,6 +73,13 @@ Manifold Transform(Manifold& manifold, std::vector<float>& mat) {
return manifold.Transform(matrix);
}

Manifold Warp(Manifold& manifold, void* funcPtr) {
void (*f)(glm::vec3&) = reinterpret_cast<void (*)(glm::vec3&)>(funcPtr);
Copy link
Owner Author

Choose a reason for hiding this comment

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

This part feels the most wrong to me. I'm slightly hazy on the distinction between a reference and a pointer, but I feel like the JS wants a pointer and the C++ is giving a reference to the glm::vec3 and I'm not really sure how to resolve that. And I'm not even sure that's the error I'm running into.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well, it's working now, so apparently that distinction is pretty moot. I'm honestly amazed this works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, references are basically implemented as pointers under the hood, but with a bit more restrictions (cannot be null, cannot have T&&&&&& while you can have T********, for example).

bindings/wasm/bindings.cpp Outdated Show resolved Hide resolved
Manifold Warp(Manifold& manifold, void* funcPtr) {
void (*f)(glm::vec3&) = reinterpret_cast<void (*)(glm::vec3&)>(funcPtr);
Manifold out = manifold.Warp(f);
EM_ASM({removeFunction($0)}, f);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of feel like that the removeFunction call should be called in the binding code? So in case we want to reuse some warp with js functions, we can do so with minimal overhead. This is not really C++ code anyway.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good call, I was following that example a little too blindly.

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 92.23% // Head: 92.23% // No change to project coverage 👍

Coverage data is based on head (7b2f066) compared to base (3f33c91).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #221   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files          32       32           
  Lines        3424     3424           
=======================================
  Hits         3158     3158           
  Misses        266      266           

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Can we add an example and make sure that it works before merging this? Just to make sure that everything is working correctly.

@@ -72,6 +73,13 @@ Manifold Transform(Manifold& manifold, std::vector<float>& mat) {
return manifold.Transform(matrix);
}

Manifold Warp(Manifold& manifold, void* funcPtr) {
void (*f)(glm::vec3&) = reinterpret_cast<void (*)(glm::vec3&)>(funcPtr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, references are basically implemented as pointers under the hood, but with a bit more restrictions (cannot be null, cannot have T&&&&&& while you can have T********, for example).

Manifold Warp(Manifold& manifold, void* funcPtr) {
void (*f)(glm::vec3&) = reinterpret_cast<void (*)(glm::vec3&)>(funcPtr);
Manifold out = manifold.Warp(f);
EM_ASM({removeFunction($0)}, f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of feel like that the removeFunction call should be called in the binding code? So in case we want to reuse some warp with js functions, we can do so with minimal overhead. This is not really C++ code anyway.

@elalish
Copy link
Owner Author

elalish commented Sep 27, 2022

I tested the example at the top. We don't have a great home for JS examples at the moment, so I'm going to add this along with yours when I do #219, which is coming soon.

@elalish elalish merged commit 3e61c1a into master Sep 27, 2022
@elalish elalish deleted the wasmWarp branch September 27, 2022 15:51
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* builds, doesn't run

* runs!

* addressing feedback
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.

JS Interface Modification
3 participants