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 missing JS bindings #375

Merged
merged 7 commits into from
Mar 16, 2023
Merged

Add missing JS bindings #375

merged 7 commits into from
Mar 16, 2023

Conversation

elalish
Copy link
Owner

@elalish elalish commented Mar 16, 2023

Fixes #373

Also adds mirror and trimByPlane.

@elalish elalish self-assigned this Mar 16, 2023
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (e17eb03) 85.48% compared to head (015425d) 85.48%.

❗ Current head 015425d differs from pull request most recent head 41c09e0. Consider uploading reports for the commit 41c09e0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   85.48%   85.48%           
=======================================
  Files          36       36           
  Lines        4415     4415           
=======================================
  Hits         3774     3774           
  Misses        641      641           

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

@elalish elalish requested a review from pca006132 March 16, 2023 18:39
@elalish
Copy link
Owner Author

elalish commented Mar 16, 2023

@pca006132 Do you have any idea why the mac build is suddenly failing? It's complaining about python, but I've only changed the JS bindings...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you have not updated your pybind11 locally (may need to do something like merging in the changes that weren't applied from master or init again). This is why the mac CI failed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It looks like the changes from #367 were somehow lost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/elalish/manifold/tree/master/bindings/python/third_party They are still present on master, the issue is that git is a bit funny about actually applying these types of changes for you locally. For instance, when I do a submodule update on one branch and commit, then switch over to another, the commit change becomes visible as an unstaged changed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Looks like it was removed here? https://github.com/elalish/manifold/pull/366/files#diff-723c1253d0c94075d3d7c80504b08943c02209f016433937ccf803a3ca6eb5feL198 I've already merged master - how would my local setup affect the CI?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think when you do the commit, you accidentally added pybind11 directory to the list of staged changes, which is probably there because you merged master without doing a manual submodule update. Submodules are really second class citizen in git...
Maybe you can try to do the update and see if there is an unstaged change to pybind11 now?

Copy link
Owner Author

@elalish elalish Mar 16, 2023

Choose a reason for hiding this comment

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

Yeah, okay I see I was confused. This is such a strange state. I swear changes to submodules used to just show up as a hash change in a hidden text file and this was all simple to fix in github. Now that seems to all be obfuscated and I can't figure out where to revert whatever difference there seems to be in my branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually I just checkout to the master and cherry-pick the commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@elalish

I can't figure out where to revert whatever difference there seems to be in my branch.

In this case, I think you should be able to navigate to the pybind11 submodule folder, and do a pull on the pybind11 stable branch.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Awesome, that worked, thanks @geoffder! Still baffled as to how this happened though. One thing: I notice that pybind11 is our only submodule that has a branch specified (stable). I wonder if that's an error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, I just noticed that the commit you were pinned on before was a previous stable branch commit (corresponding to a release). So I pulled from that branch to update. I didn't checkout stable though (just pulled the most recent commit from it).

@elalish elalish merged commit d713986 into master Mar 16, 2023
@elalish elalish deleted the fixTransform branch March 16, 2023 20:00
@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
* readd transform

* added mirror to wasm bindings

* added trimByPlane wasm bindings

* fixed bugs

* update test

* trying to fix submodule
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.

.transform not available in wasm
3 participants