Add utility functions to Python PSI #2532
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2532 +/- ##
==========================================
+ Coverage 60.24% 60.25% +0.02%
==========================================
Files 351 351
Lines 26473 26473
==========================================
+ Hits 15945 15949 +4
+ Misses 10528 10524 -4
Continue to review full report at Codecov.
|
| @@ -55,10 +55,13 @@ | |||
|
|
|||
|
|
|||
| class PlanningSceneInterface(object): | |||
| """ Simple interface to making updates to a planning scene """ | |||
| """ | |||
| Python interface for a planning scene. | |||
There was a problem hiding this comment.
Does this provide an interface to modifying a PlanningScene or to modifying the PlanningScene curated by the PlanningSceneMonitor? (Not sure if the first one even exists in Python)
There was a problem hiding this comment.
True, it's the latter. A Python interface for the PlanningSceneInterface, not the PlanningScene.
There are no Python bindings for the PlanningScene, although there is one for the RobotState. There has been discussion in #900 and I should probably raise the issue again.
| """ | ||
| Applies the planning scene message. | ||
| """ | ||
| return self._psi.apply_planning_scene(conversions.msg_to_string(planning_scene_message)) |
There was a problem hiding this comment.
I don't think we should add these methods.
__submit will already use the service iff the PSI object is instantiated as synchronous.
There is no merit in depending on the C++ implementation here.
There was a problem hiding this comment.
__submit is not user-facing, so users of this class can't publish their own CollisionObject.
I also don't see the problem with depending on the C++ implementation.
I can implement apply_collision_object and apply_attached_collision_object with a call to __submit, but a pure Python API doesn't seem like it will be maintained, so adding C++ bindings with familiar names seems better.
There was a problem hiding this comment.
__submit is not user-facing, so users of this class can't publish their own CollisionObject.
That's true, but that's why you added the add_object method that uses __submit (just like almost every other method in this class)!
So you basically add a second way to do the same thing if you also expose the C++ apply* methods.
There was a problem hiding this comment.
Oh. Right. These commits are so old that I forgot that I did that. I just knew that they worked and we've been using them.
I'm not sure what I think about 1) the naming being different from C++ for no reason and 2) maintaining a pure Python API instead of bindings as described in #900, but I can leave out the C++ methods for now.
Are you good with apply_collision_object and apply_attached_collision_object calling __submit?
There was a problem hiding this comment.
I'm not sure what I think about 1) the naming being different from C++ for no reason and
neither am I, but that's what we got. The idea back then was to avoid breaking the whole class, but still support the update service for all existing methods. The only proper way to have a 1-to-1 mapping with the C++ classes would be to create an entirely new package for it. It's an interesting idea, but quite some work too. :-)
Are you good with apply_collision_object and apply_attached_collision_object calling __submit?
No, because if synchronous == False, these methods would use the topic interfaces then!
With the current design we should not have the add vs apply distinction we have in C++.
There was a problem hiding this comment.
Also true.
Alright, I took applyCollisionObject out. applyPlanningScene was there already though, it just wasn't exposed in Python. Surely that was an oversight?
Yes, more Python bindings would be great. Someone from #900 should start adding PlanningScene (not PSI) bindings for WMD.
|
Looks like this CI failure is unrelated:
|
|
`applyPlanningScene` was there already though, it just wasn't exposed in Python. Surely that was an oversight? I don't see a way to publish a PlanningScene message synchronously otherwise.
True. To stay consistent with the current layout we should in theory decide based on `synchronous` whether the PlanningScene is sent via `/planning_scene` or apply it via the service.
However there is currently no such Publisher and I don't think it's overly important to have it.
|
I found a few very basic functions that I had added in the past, but apparently forgot to push upstream. The
ApplyPlanningScenefunction seems to have been implemented already inwrap_python_planning_scene_interface.cppbut not exposed in the Python class, so I added that as well.The interface seems not very useful without these functions and I'd like to use them in a different project where we don't build MoveIt from source, so it would be great if these found their way into the binaries.
This should be ported back to Melodic, possibly even Kinetic.
Checklist