-
Notifications
You must be signed in to change notification settings - Fork 277
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
Added Python interfaces to some Ignition Gazebo methods #1219
Conversation
Signed-off-by: ahcorde <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo6 #1219 +/- ##
============================================
Coverage 62.93% 62.93%
============================================
Files 299 299
Lines 24167 24167
============================================
Hits 15210 15210
Misses 8957 8957 Continue to review full report at Codecov.
|
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.
Great work, works as advertised! At this first pass, I just have some high-level questions about the implementation.
You can ignore the style nitpicks for now and address them once we've settled on the approach.
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[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.
Just some minor stylistic comments, I think we're almost there!
It would be good to have a little test for this on CI to make sure that it's working once we release ign-math
debs.
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: Alejandro Hernández <[email protected]>
Signed-off-by: ahcorde <[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.
Can we add the python dependencies to packages.apt
so that the bindings are built on CI?
Signed-off-by: ahcorde <[email protected]>
I made some changes that might be conflictive.
I can open a different PRs, these PRs might require to |
Signed-off-by: ahcorde <[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.
It's looking great! I just have final suggestions in #1308 and after that I'm ready to approve! 🎉
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: ahcorde <[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.
Yay! This is great! Now we just need an ign-math6
stable release for the tests to pass.
I tested this branch against |
Signed-off-by: Louise Poubel <[email protected]>
A couple of problems in Jenkins CI.
|
Signed-off-by: Louise Poubel <[email protected]>
For packaging, we'll need to do the same as gazebo-release/gz-math6-release#6 for |
Signed-off-by: Louise Poubel <[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.
Patch seems to work as expected, tested via patch in release repo here:
-- Installing: /home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/sdformat.cpython-38-x86_64-linux-gnu.so
-- Installing: /home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/ignition/gazebo.cpython-38-x86_64-linux-gnu.so
-- Set runtime path of "/home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/ignition/gazebo.cpython-38-x86_64-linux-gnu.so" to ""
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1 |
Signed-off-by: ahcorde [email protected]
🎉 New feature
Summary
Added Python interfaces to same Ignition Gazebo methods with pybind11
Requires gazebosim/gz-math#280
Related with this issue #789 (comment)
Test it
I created an example
examples/python/helperFixture.py
.Export your python path:
Run the example:
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge