-
Notifications
You must be signed in to change notification settings - Fork 534
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
(moveit_py) Python Bindings for MoveGroupInterface #2574
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2574 +/- ##
=======================================
Coverage 50.73% 50.73%
=======================================
Files 386 386
Lines 32087 32087
=======================================
Hits 16276 16276
Misses 15811 15811 ☔ View full report in Codecov by Sentry. |
6fda0ef
to
0a69720
Compare
Re-based off the latest main branch. |
The warning is caused (in this case) by binding a C++ function that takes a std::vector without including <pybind11/stl.h>
The MoveGroupInterface class can now be imported in python with from moveit.planning import MoveGroupInterface Minimal functionality is ported over, but enough to enable a parallel tutorial to the "Your First C++ MoveIt Project" This means that the following functions are wrapped for python: MoveGroupInterface::MoveGroupInterface MoveGroupInterface::Plan (a struct that holds the motion plan) MoveGroupInterface::setPoseTarget MoveGroupInterface::plan MoveGroupInterface::execute
0a69720
to
b0eaa9a
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.
Thank you for this contribution!
I am not sure, if it makes sense to expose the move_group_interface with the python bindings. We'd have a python wrapper around the cpp wrapper of the ros interface of moveit which is probably too many layers. I think an approach such as the one taken here where the python code uses the move_group ros api directly might be more appropriate although I am not sure if something like this needs to live in the main repository. Please let me know if you have a good rationale for the MoveGroupInterface python bindings, I am not seeing at the moment!
Thanks for the feedback @sjahr. There are two primary motivations behind why I decided to work on this:
Here's the main motivating example: The Your First C++ MoveIt Project I wrote an equivalent python tutorial Your First Python MoveIt Project (which relies on this PR). The python tutorial could not be written with existing It would definitely be possible to do all this with a third-party python wrapper for the ROS API (such as
I agree that there are advantages to the approach |
I think I agree with @sjahr. The move group interface is some cpp code to talk to ROS API of moveit. I don't think we should wrap that with pybind. I do think it is a good idea to create a move group interface for python, but written in python. If that library is close to the cpp interfaces, it could live in this repo, maybe even be part of moveit_py. |
Thanks for the feedback @MatthijsBurgh. It seems like the main reluctance here is that MoveGroupInterface is already a wrapper around the ROS API so this is "wrapping a wrapper". My concern is that MoveGroupInterface does not seem to be just a simple ROS API wrapper: it uses threads, futures, and matrix math,all of which a pure python approach needs to re-implement and maintain. And the existing python API wrappers (such as pymoveit2) are non-trivial. Anyway, thanks for taking the time to consider this. I'd like to help contribute to making the python and C++ moveit experiences as similar as possible (especially in terms of ROS 2 knowledge required for a given task) regardless of the outcome of this PR, so I'd appreciate a bit more feedback on how to proceed. Is the pybind11 approach in this PR ruled out completely (seems like it might be)? If so I'll close the PR (and can make a new one removing the currently unused python wrapping code in the C++ MoveGroupInterface). If there is a possibility of acceptance down the road, I'm happy to attempt writing a full wrapper implementation with tests and tutorials to prove the concept further. |
@m-elwin We really appreciate your effort here!
Maybe this would be a good step forward. Furthermore, you could try writing a minimal python move_group interface (e.g. as a tutorial for moveit and we'll see where to go from there). If you're interested you could join the public maintainer meeting later this month to discuss any further ideas |
This pull request is in conflict. Could you fix it @m-elwin? |
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
This pull request is in conflict. Could you fix it @m-elwin? |
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2574 +/- ##
=======================================
Coverage 50.73% 50.73%
=======================================
Files 386 386
Lines 32087 32087
=======================================
Hits 16276 16276
Misses 15811 15811 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Description
Implements preliminary python bindings for the C++ MoveGroupInterface
Only the minimum needed to create a python version of the Your First C++ MoveIt Project Tutorial, is currently implemented.
A draft of a python version of the tutorial can be found here.
For example,
ros2 launch moveit2_tutorials demo.launch.py
. Then, with this PR,running the following python script will move end-effector to a target pose:
Checklist