This repository has been archived by the owner. It is now read-only.

add ApplyPlanningSceneService capability #686

Merged
merged 4 commits into from Jun 16, 2016

Conversation

Projects
None yet
3 participants
@v4hn
Member

v4hn commented Jun 13, 2016

See https://groups.google.com/forum/#!topic/moveit-users/D7MWZEUSKLc
for a longer discussion on why this makes sense to have around.

The only thing I'm unsure about is whether the service should be
~-namespaced or not. There might be multiple monitored scenes
around, so I moved it to the namespace. On the other hand,
the ClearOctomap service is not namespaced...

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Jun 13, 2016

Member

This obviously depends on ros-planning/moveit_msgs#21

Member

v4hn commented Jun 13, 2016

This obviously depends on ros-planning/moveit_msgs#21

* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
/* Author: Michael 'v4hn' Goerner */

This comment has been minimized.

@davetcoleman

davetcoleman Jun 13, 2016

Member

im a big fan of descriptions in header files for what the class does....

@davetcoleman

davetcoleman Jun 13, 2016

Member

im a big fan of descriptions in header files for what the class does....

This comment has been minimized.

@v4hn

v4hn Jun 14, 2016

Member

On Mon, Jun 13, 2016 at 03:41:28PM -0700, Dave Coleman wrote:

im a big fan of descriptions in header files for what the class does....

Now you're being unfair. :)
I totally agree, but none of the other capabilities include a description
and I wouldn't even know what to describe here, as it's pretty self-documenting.
"ApplyPlanningSceneService : MoveGroupCapability is a MoveGroupCapability
that provides a service to apply a given PlanningScene" is just overly redundant.

@v4hn

v4hn Jun 14, 2016

Member

On Mon, Jun 13, 2016 at 03:41:28PM -0700, Dave Coleman wrote:

im a big fan of descriptions in header files for what the class does....

Now you're being unfair. :)
I totally agree, but none of the other capabilities include a description
and I wouldn't even know what to describe here, as it's pretty self-documenting.
"ApplyPlanningSceneService : MoveGroupCapability is a MoveGroupCapability
that provides a service to apply a given PlanningScene" is just overly redundant.

This comment has been minimized.

@davetcoleman

davetcoleman Jun 14, 2016

Member

You are exactly right about not having enough documentation throughout the code base and I know the others lack it. How about:
"Provides ability to update the shared planning scene with a remote blocking call"

@davetcoleman

davetcoleman Jun 14, 2016

Member

You are exactly right about not having enough documentation throughout the code base and I know the others lack it. How about:
"Provides ability to update the shared planning scene with a remote blocking call"

@@ -478,9 +481,6 @@ class PlanningSceneMonitor : private boost::noncopyable
// Callback for a new planning scene msg
void newPlanningSceneCallback(const moveit_msgs::PlanningSceneConstPtr &scene);
// Called to update the planning scene with a new message.
void newPlanningSceneMessage(const moveit_msgs::PlanningScene& scene);

This comment has been minimized.

@davetcoleman

davetcoleman Jun 13, 2016

Member

just a guess, but wouldn't this break ABI too? I don't care about ABI, actually, but maybe we should go ahead and make it return bool while we're at it (doesn't break API).

@davetcoleman

davetcoleman Jun 13, 2016

Member

just a guess, but wouldn't this break ABI too? I don't care about ABI, actually, but maybe we should go ahead and make it return bool while we're at it (doesn't break API).

This comment has been minimized.

@v4hn

v4hn Jun 14, 2016

Member

On Mon, Jun 13, 2016 at 03:42:43PM -0700, Dave Coleman wrote:

just a guess, but wouldn't this break ABI too?

No, changing visibility private -> public does not break ABI.
As it turns out, in this case changing the void return
to bool also is ABI compliant as the function was private before.
I'll prepare the changes.

@v4hn

v4hn Jun 14, 2016

Member

On Mon, Jun 13, 2016 at 03:42:43PM -0700, Dave Coleman wrote:

just a guess, but wouldn't this break ABI too?

No, changing visibility private -> public does not break ABI.
As it turns out, in this case changing the void return
to bool also is ABI compliant as the function was private before.
I'll prepare the changes.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Jun 14, 2016

Member

@davetcoleman: now this request also spans 4 repos :-)
Full +1 to merge most moveit repositories...

I do care quite a lot about a compatible ABI within ros distros until we add sonames to our libraries to explicitly mark updates as ABI-breaking.
As @mikeferguson pointed out last week it's pretty horrible to debug random segfaults due to ABI changes without sonames. Last year I spent more than 12 hours debugging one of these issues in the PCL, although it was a particularly horrible one.

Any comments on the question of namespacing?

Member

v4hn commented Jun 14, 2016

@davetcoleman: now this request also spans 4 repos :-)
Full +1 to merge most moveit repositories...

I do care quite a lot about a compatible ABI within ros distros until we add sonames to our libraries to explicitly mark updates as ABI-breaking.
As @mikeferguson pointed out last week it's pretty horrible to debug random segfaults due to ABI changes without sonames. Last year I spent more than 12 hours debugging one of these issues in the PCL, although it was a particularly horrible one.

Any comments on the question of namespacing?

Show outdated Hide outdated planning/planning_scene_monitor/src/planning_scene_monitor.cpp
if (!scene_)
return false;
bool ret;

This comment has been minimized.

@davetcoleman

davetcoleman Jun 14, 2016

Member

result

Show outdated Hide outdated planning/planning_scene_monitor/src/planning_scene_monitor.cpp
bool ret;
SceneUpdateType upd = UPDATE_SCENE;

This comment has been minimized.

@davetcoleman

davetcoleman Jun 14, 2016

Member

again, full var names...

@davetcoleman

davetcoleman Jun 14, 2016

Member

again, full var names...

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jun 14, 2016

Member

i don't have an opinion on the namespacing, other than we should do it as the other services have been done. i don't know of any current uses cases needing more than one planning scene

Member

davetcoleman commented Jun 14, 2016

i don't have an opinion on the namespacing, other than we should do it as the other services have been done. i don't know of any current uses cases needing more than one planning scene

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Jun 14, 2016

Member

Ok, I renamed all the result variables,
added the comment you proposed, and decided to move
the service away from ~. Most of the other services
don't use it so no reason to start with it for now.

Member

v4hn commented Jun 14, 2016

Ok, I renamed all the result variables,
added the comment you proposed, and decided to move
the service away from ~. Most of the other services
don't use it so no reason to start with it for now.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jun 14, 2016

Member

I'll merge these as soon as @mikeferguson gives me a +1 on the moveit_msgs change. Until then, could you add a section to the documentation here mentioning this new way to update the PS? It doesn't need to be much.

Member

davetcoleman commented Jun 14, 2016

I'll merge these as soon as @mikeferguson gives me a +1 on the moveit_msgs change. Until then, could you add a section to the documentation here mentioning this new way to update the PS? It doesn't need to be much.

@mikeferguson

This comment has been minimized.

Show comment
Hide comment
@mikeferguson

mikeferguson Jun 14, 2016

Member

@davetcoleman -- we need to merge the moveit_msgs and then release it as Debs so that this can properly build, else the CI will be broken

Member

mikeferguson commented Jun 14, 2016

@davetcoleman -- we need to merge the moveit_msgs and then release it as Debs so that this can properly build, else the CI will be broken

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jun 14, 2016

Member

just restarted build after merging msgs and core

Member

davetcoleman commented Jun 14, 2016

just restarted build after merging msgs and core

v4hn added some commits Jun 13, 2016

monitor: make newPlanningSceneMessage public
This is required for capabilities to update the planning scene.

ABI-compatible.
add apply_planning_scene capability
This new capability allows to apply changes to a monitored planning
scene and *blocks* until the changes are applied. This is meant to
replace the quasi-standard pattern:
```
planning_scene_interface.addCollisionObjects(...)
sleep(2.0)
group.pick("object")
```
by
```
ros::ServiceClient client = n.serviceClient<moveit_msgs::ApplyPlanningScene>("apply_planning_scene");
client.call(...)
group.pick("object")
```

This makes it much more convenient to add&interact with objects
without useless and arbitrarily long sleeps to ensure planning scene
updates succeeded.

@davetcoleman davetcoleman merged commit 7d3ba75 into ros-planning:indigo-devel Jun 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.