-
Notifications
You must be signed in to change notification settings - Fork 126
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
Quat, Frustum Python bindings #464
base: main
Are you sure you want to change the base?
Conversation
Fix for issue AcademySoftwareFoundation#428 Created a pybind11 wrapping file based on the specified template. Requires further testing, as this is my first time using pybind11. Signed-off-by: Nikhil Mishra <[email protected]> Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
7112f33
to
519afbb
Compare
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
Signed-off-by: Piotr Barejko <[email protected]>
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 for the contributions! Some comments below.
@@ -0,0 +1,43 @@ | |||
import sys |
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 think the best strategy for the test is to duplicate src/test/PyImathTest/pyImathTest.in in src/pybind11, and comment out the sections that aren't yet implemented, and then bring the back in as new classes/features get implemented. That's better than building up a duplicate set of tests. I recommend just removing this for now and dealing with the test as a separate PR
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 recommend just removing this for now and dealing with the test as a separate PR
I will remove those tests - those were sanity check for me - that the code actually runs. The code can compile, but we might get a lot of runtime issues related to unbinded types, etc.
That's better than building up a duplicate set of tests.
Agreed. I need to spend more time to understand what was the premise of making integration tests as *.in
file, why not break it down into multiple different files. Ideally, we would like to run same tests for boost
and pybind11
to confirm that we are backwards compatible.
Can you share what is the future of boost
bindings? Would you like to remove them?
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.
The plan is to ultimately retire the boost bindings altogether, once the pybind implementation replicates all the functionality. The setup of building both in parallel allows us to implement the pybind wrappings incrementally. I'll look into arranging the test to run with the pybind implementation. I agree the test would be more manageable as separate files, but that's the way it was implemented.
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.
One more question related to *Array
classes, what was motivation for those? Perf? Would you like to transition them to pybind11 in the future or retire them as well?
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.
Ignore the Array classes for now, they need some special consideration, it's not entirely clear what to do with them.
I've made some progress with bringing the existing test over to the pybind11 bindings, will submit that shortly.
Based on existing boost bindings, should be swappable between pybind and boost impl.