-
Notifications
You must be signed in to change notification settings - Fork 938
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
WIP: Rework Python API #2910
base: master
Are you sure you want to change the base?
WIP: Rework Python API #2910
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2910 +/- ##
===========================================
- Coverage 47.15% 14.49% -32.65%
===========================================
Files 603 281 -322
Lines 58999 24731 -34268
Branches 6969 0 -6969
===========================================
- Hits 27814 3583 -24231
+ Misses 30773 21148 -9625
+ Partials 412 0 -412 ☔ View full report in Codecov by Sentry. |
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'm not sure why you remove the calls to initialize roscpp in the demo scripts, surely it is still needed since the C++ code it calls uses ROS, ros::init should be called somewhere?
Would you consider changing how the C++ node is named? the default argument of "moveit_python_wrappers" means if you run multiple python nodes using moveit you get lots of confusing ros node names. Perhaps have a look at the ros init python bindings we use in my lab, which names nodes as "my_name" and "my_name_cpp".
https://github.com/UM-ARM-Lab/arc_utilities/blob/master/src/roscpp_initializer.cpp
https://github.com/UM-ARM-Lab/arc_utilities/blob/master/src/arc_utilities/ros_init.py
I'm not sure why you added the string <--> PoseStameped converter but seems fine.
I don't see anything obviously wrong with these changes, I think it's a step in the right direction.
Ideally, those explicit
Generally, I like this idea. It's easily implemented for
It allows creating a |
Hm ok I guess that's reasonable. So MoveGroupInterfaceWrapper inhereits from it, so that should stlil work, interesting. I prefer an explicit initialize at the beginning of my script, because often use multiple C++ ROS bindings, not just moveit, but that's still compatible with this I think |
Sure. The first call wins. |
a7e1aae
to
3fc3765
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.
I know this is a draft, just hitting "approve" so github will mark up to which commit I last looked at this.
Always happy to help 😄
If we use Side note: I tried to enable coverage for moveit_commander, but some problems arised: ros/ros_comm#2199 |
Using the old library wrappers directly isn't very useful because you would need to handle serialized byte strings. I was expecting nobody to do that and thus not provide backwards compatibility to this awkward API. |
- Use pybind11 and its stl conversion routines to pass remappings instead of argv - Use overloading of roscpp_init() - Expose ros::InitOptions - Migrate from boost to std
This will create an identity transform with the given string as the reference frame_id.
3cc8560
to
ca0c360
Compare
libmoveit_py_bindings_tools -> libmoveit_python_bindings_tools
969b5b4
to
2fb9741
Compare
2fe3dd4
to
a9a8ecf
Compare
a9a8ecf
to
0d4248a
Compare
@ros-planning/moveit-maintainers, @gleichdick, @PeterMitrano: These python wrapper improvements are lying around (nearly ready) for over a year now. The (only) road blocker was and is a major design flaw in catkin's devel-space layout (ros/catkin#1153), which renders multiple namespace packages installing into the same devel-space location impossible. As we won't fix catkin anymore, I only see the following options:
What do you think? @henningkayser: We should discuss that in the next maintainer meeting. |
Seems like the 2nd option, i.e. change the package naming, is by far the least work and honestly not that bad. Moveit 1 doesn't seem to be getting much core support anymore, and python for moveit 1 isn't high on the priorities (although I really want it). So seems like the second option is best? As for #2613 I think we're still blocked on docs not generating/being hosted somewhere? #2606 |
.def("get_planning_frame", &RobotModel::getModelFrame) | ||
.def("get_robot_root_link", &RobotModel::getRootLinkName) | ||
.def("has_group", &RobotModel::hasJointModelGroup, py::arg("group")) | ||
.def("get_robot_name", &RobotModel::getName) |
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 was rebasing #2613 on top of this and noticed you have this method bound twice with different names. Is that right? I think we should only have get_name
I've done some more investigating on how to get
Thoughts? |
Alternatively, we can just not have in centralized imports, and stick with |
e1b0ec1
to
fdc3b1c
Compare
This is an attempt to modernize the Python API from
boost::python
topybind11
and reduce boilerplate code for type conversions based on initial work in #2547. See individual commits for details.py_bindings_tools
frommoveit_ros_planning_interface
tomoveit_core
(and some cleanup)rospy.names.get_mappings()
intoROScppInitializer
to use python remappings for C++ as well?ros_msg_typecasters.h
requires PyBytes handlingas introduced by @gleichdick in Python 3 friendly message serialization (master branch) #1870
pybind11
and its automatic type conversions. Removeserialize_msg.h
.py_conversions.h
withpybind11/stl.h
gil_releaser.h
withpybind11::gil_scoped_release
eigenpy
withpybind11/eigen.h
moveit_commander
@gleichdick, @PeterMitrano: I would be happy if you could contribute to this work, filing PRs against my PR branch.
What do you think of the cleanup so far?