-
Notifications
You must be signed in to change notification settings - Fork 108
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
use nanobind builtin stubgen #949
Conversation
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.
Does this mean we can remove the python auto-gen docs now?
@@ -17,7 +17,8 @@ project(python) | |||
execute_process( | |||
COMMAND "${Python_EXECUTABLE}" -m nanobind --version | |||
OUTPUT_STRIP_TRAILING_WHITESPACE OUTPUT_VARIABLE NB_VERSION) | |||
if(NB_VERSION VERSION_GREATER_EQUAL 1.8.0) | |||
# we are fine with 2.0.0 | |||
if(NB_VERSION VERSION_GREATER_EQUAL 2.0.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.
Should we just go with 2.1.0 across the board?
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.
Some distros may have 2.1.0? Not sure about if we should support that.
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.
Okay, so 2.0 is significantly more widely available than 2.1? This is fine if so - I was just thinking we could reduce CMake complexity a bit.
@@ -84,7 +84,7 @@ def floor(length, width): | |||
) | |||
) | |||
if length == 1 and width > 1: | |||
results.append(row(width - 1).rotate(0, 0, 90)) | |||
results.append(row(width - 1).rotate((0, 0, 90))) |
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.
What are the extra parens for?
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.
tuple
@@ -327,7 +327,8 @@ NB_MODULE(manifold3d, m) { | |||
.def("refine", &Manifold::Refine, nb::arg("n"), manifold__refine__n) | |||
.def("refine_to_length", &Manifold::RefineToLength, nb::arg("length"), | |||
manifold__refine_to_length__length) | |||
.def("to_mesh", &Manifold::GetMeshGL, nb::arg("normal_idx") = ivec3(0), | |||
.def("to_mesh", &Manifold::GetMeshGL, | |||
nb::arg("normal_idx") = std::make_tuple(0, 0, 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.
👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
==========================================
- Coverage 91.84% 88.25% -3.59%
==========================================
Files 37 62 +25
Lines 4976 8666 +3690
Branches 0 1049 +1049
==========================================
+ Hits 4570 7648 +3078
- Misses 406 1018 +612 ☔ View full report in Codecov by Sentry. |
and no, we still need to autogen docs for getting the doc strings. |
will figure out the wasm issue tmr, probably some library version issue. |
Maybe we can just disable |
@elalish should we merge this? |
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!
Closes #855.
The stub is not perfect because it is python, but I think we are pretty good now.