-
Notifications
You must be signed in to change notification settings - Fork 655
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
Port OpenVDB Python bindings to nanobind and add NanoVDB Python bindings #1916
base: master
Are you sure you want to change the base?
Port OpenVDB Python bindings to nanobind and add NanoVDB Python bindings #1916
Conversation
badc722
to
e073f14
Compare
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.
This looks really great, thanks @matthewdcong! I didn't do any user testing, but the implementation, build and CI changes all look reasonable to me. I just had one small question about which minimum version of nanobind we should support.
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 have not runtime tested the changes. I have only reviewed the OpenVDB binding changes.
Can we please squash all commits into a single change? Or, better yet, a single commit which ports the OpenVDB bindings to nanobind, with a second commit to add the nanovdb bindings would be perfect. I'm happy with a single commit if necessary, I'm not sure the current commit chain is particularly useful.
Minor comments from me, looks good!
5d3707c
to
ee37791
Compare
Thanks for the review! I will squash before merging. Delaying for now in case others have feedback in the meantime. |
Signed-off-by: Matthew Cong <[email protected]>
Signed-off-by: Matthew Cong <[email protected]>
ee37791
to
f5f3f6c
Compare
Waiting on #1933 to be merged in order to fix the Windows CI. |
No description provided.