Complete scene return for component = 0 (#1067) #1424

j-petit · 2019-04-02T11:19:33Z

Description

When calling the /get_planning_scene service with component = 0, the service will return the full scene instead of an empty one (proposed in #1067)
I identified a single place in the codebase where a call to the service with all components takes place and replaced it with a call with components = 0. I am not to happy about writing it out as it is (that's why I had to add the comment), maybe someone can give me a hint to make it more understandable intuitively.

Checklist

Required by CI: Code is auto formatted using clang-format
Extended the tutorials / documentation, if necessary reference
Document API changes relevant to the user in the moveit/MIGRATION.md notes
Decide if this should be cherry-picked to other current ROS branches

felixvd

Nice work, thanks.

I think what @v4hn meant was not if there is a call to the service that requests the whole scene, but that there might be a place in the code where get_planning_scene is called and an empty scene is expected as a response. E.g. a loop that requests parts of the scene when they need to be updated, and sends an empty service request otherwise.

It seems that no one is certain, so rather than hunting through the entire code base, it might be more efficient to check if performance drops after this change.

I did not test this locally, for the record.

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

moveit_ros/move_group/src/default_capabilities/get_planning_scene_service_capability.cpp

j-petit · 2019-04-08T09:18:21Z

I searched through the codebase thoroughly and am quite confident that there are no calls to the service where an empty scene response is expected. The service is called only through an intermediate function which always requests the full scene. Nevertheless, I would still like to do the performance check but I don't know what exactly do you mean when referring to performance and how would you measure it. Could you give me a hint how to do this?

felixvd · 2019-04-08T11:58:44Z

The keyword is code benchmarking, and I would simply test if calling the get_planning_scene service is slower with req.components.components = 0 and req.components.components = UINT_MAX. If you call the service from a Python script a bunch of times and time the execution, that should be enough. There are much better tools for this, but I don't know them. This might be overkill anyway; someone with more compiler knowledge would know if this will affect performance.

Apart from that, I also had a look through the code and couldn't find any calls to this service except the PlanningSceneMonitor::requestPlanningSceneState wrapper which you changed. I think the change itself is fine.

To document it, I would personally add the same comment to the PlanningSceneRequest msg file and maybe a line like "get_planning_scene service now returns the whole scene if no components are specified" to the migration notes. Maybe some maintainers can give their opinion and request those changes. Apart from that and the first paragraph, I think this is fine.

That said, I haven't tested it.

j-petit · 2019-04-18T10:45:24Z

The keyword is code benchmarking, and I would simply test if calling the get_planning_scene service is slower with req.components.components = 0 and req.components.components = UINT_MAX. If you call the service from a Python script a bunch of times and time the execution, that should be enough. There are much better tools for this, but I don't know them. This might be overkill anyway; someone with more compiler knowledge would know if this will affect performance.

I did some measurements and the difference is tiny. So I think we are good to go from this aspect.

To document it, I would personally add the same comment to the PlanningSceneRequest msg file and maybe a line like "get_planning_scene service now returns the whole scene if no components are specified" to the migration notes. Maybe some maintainers can give their opinion and request those changes. Apart from that and the first paragraph, I think this is fine.

I think that the comment is better fit in the service description (moveit_msgs/srv/GetPlanningScene.srv) as this is a property of the service and not the message itself?

felixvd · 2019-04-18T10:59:32Z

Yes, the service description file is the right place. I got confused copying things around.

BryceStevenWilley

LGTM in general, with two small corrections:

This should target master branch, not melodic-devel. You should be able to change the target branch on this same PR.
IMO a change like this should go in MIGRATION.md, which I'd prefer to have in this PR.

MIGRATION.md

j-petit · 2019-05-03T09:17:06Z

[Resolved]
Sorry, when trying to get a clean PR through incorporating changes from master to my devel-branch, I messed it up. Cleaning up now...

moveit_core/robot_state/src/robot_state.cpp

…in get_planning scene (moveit#1067)

felixvd · 2019-05-03T10:01:20Z

MIGRATION.md

@@ -6,6 +6,7 @@ API changes in MoveIt! releases
 - Extended the return value of `MoveitCommander.MoveGroup.plan()` from `trajectory` to a tuple of `(success, trajectory, planning_time, error_code)` to better match the C++ MoveGroupInterface ([790](https://github.com/ros-planning/moveit/pull/790/))
 - `moveit_rviz.launch`, generated by MSA, provides an argument `rviz_config` to configure the rviz config to be used. The old boolean config argument was dropped. ([1397](https://github.com/ros-planning/moveit/pull/1397))
 - Moved the example package `moveit_controller_manager_example` into [moveit_tutorials](https://github.com/ros-planning/moveit_tutorials)
+- Requests to `get_planning_scene` service without explicitely setting "components" now returns full scene


-> explicitly

Sorry, missed my spelling mistakes.

rhaschke

One more typo. Code is a little bit too verbose. I suggested (and pushed) an alternative.

MIGRATION.md

rhaschke · 2019-05-03T17:44:44Z

moveit_ros/move_group/src/default_capabilities/get_planning_scene_service_capability.cpp

-  ps->getPlanningSceneMsg(res.scene, req.components);
+
+  // all scene components are returned if none are specified
+  if (!req.components.components)


I suggested a more compact code snippet.

moveit_ros/move_group/src/default_capabilities/get_planning_scene_service_capability.cpp

BryceStevenWilley · 2019-05-06T13:02:19Z

Travis failed because it looks like rosdep (or maybe github) was down a few days ago. I'm going to close and re-open this PR to retrigger the build.

BryceStevenWilley · 2019-05-07T12:54:33Z

Attempting to fix flaky CI here.

BryceStevenWilley · 2019-05-14T01:24:07Z

Trying one more time to get Travis to pass.

felixvd reviewed Apr 7, 2019

View reviewed changes

felixvd approved these changes Apr 18, 2019

View reviewed changes

This was referenced Apr 18, 2019

Add comment all scene components returned if no components specified j-petit/moveit_msgs#1

Closed

Add comment all scene components returned if no components specified moveit/moveit_msgs#51

Merged

rhaschke force-pushed the melodic-devel branch from 8fde277 to a20b363 Compare April 26, 2019 09:43

BryceStevenWilley suggested changes May 3, 2019

View reviewed changes

j-petit changed the base branch from melodic-devel to master May 3, 2019 08:10

j-petit requested review from davetcoleman, IanTheEngineer, mlautman, rhaschke and v4hn as code owners May 3, 2019 09:12

felixvd reviewed May 3, 2019

View reviewed changes

MIGRATION.md Outdated Show resolved Hide resolved

felixvd reviewed May 3, 2019

View reviewed changes

moveit_core/robot_state/src/robot_state.cpp Outdated Show resolved Hide resolved

j-petit added 9 commits May 3, 2019 11:25

show all components of planning_scene if 0 is specified as requested …

71ca31b

…in get_planning scene (moveit#1067)

Fixed implementation to query all components

e22c5b3

Autoformat

3370bda

added debug msg

caf0cb3

Remove print debugging commands

9d0ddda

Remove more debugging commands

22602b4

Added comment

53017f2

Added comments and simplified bitfield with all 1

6b1978d

Added migration description

00ed699

j-petit force-pushed the complete_scene_return branch from a67d4dc to 00ed699 Compare May 3, 2019 09:29

felixvd reviewed May 3, 2019

View reviewed changes

j-petit and others added 2 commits May 3, 2019 12:23

Corrected spelling mistakes

3cda472

more compact code

6ca8e9f

rhaschke requested changes May 3, 2019

View reviewed changes

felixvd reviewed May 4, 2019

View reviewed changes

moveit_ros/move_group/src/default_capabilities/get_planning_scene_service_capability.cpp Show resolved Hide resolved

rhaschke approved these changes May 4, 2019

View reviewed changes

rhaschke added the awaits 2nd review one maintainer approved this request label May 4, 2019

BryceStevenWilley approved these changes May 6, 2019

View reviewed changes

BryceStevenWilley closed this May 6, 2019

BryceStevenWilley reopened this May 6, 2019

BryceStevenWilley removed the awaits 2nd review one maintainer approved this request label May 7, 2019

BryceStevenWilley mentioned this pull request May 7, 2019

Wrapped rosdep update in travis_retry moveit/moveit_ci#63

Closed

BryceStevenWilley closed this May 14, 2019

BryceStevenWilley reopened this May 14, 2019

rhaschke and others added 3 commits May 14, 2019 08:19

fixup! more compact code

0f534ac

Fixup for more compact code

1712cfb

remove static

1e2d0d1

rhaschke merged commit 0eae58c into moveit:master May 14, 2019

rhaschke mentioned this pull request May 14, 2019

get_planning_scene with 0-component should return complete scene #1067

Closed

j-petit deleted the complete_scene_return branch May 14, 2019 11:43

tylerjw mentioned this pull request Jun 24, 2020

July 2020 Melodic Release #2066

Closed

20 tasks

simonschmeisser mentioned this pull request Apr 27, 2021

Initial sync of PlanningSceneDisplay missing #2624

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete scene return for component = 0 (#1067) #1424

Complete scene return for component = 0 (#1067) #1424

j-petit commented Apr 2, 2019 •

edited

Loading

felixvd left a comment

j-petit commented Apr 8, 2019

felixvd commented Apr 8, 2019

j-petit commented Apr 18, 2019

felixvd commented Apr 18, 2019

BryceStevenWilley left a comment

j-petit commented May 3, 2019 •

edited

Loading

felixvd May 3, 2019

felixvd May 3, 2019

j-petit May 3, 2019

rhaschke left a comment

rhaschke May 3, 2019

j-petit May 4, 2019

BryceStevenWilley commented May 6, 2019

BryceStevenWilley commented May 7, 2019

BryceStevenWilley commented May 14, 2019

Complete scene return for component = 0 (#1067) #1424

Complete scene return for component = 0 (#1067) #1424

Conversation

j-petit commented Apr 2, 2019 • edited Loading

Description

Checklist

felixvd left a comment

Choose a reason for hiding this comment

j-petit commented Apr 8, 2019

felixvd commented Apr 8, 2019

j-petit commented Apr 18, 2019

felixvd commented Apr 18, 2019

BryceStevenWilley left a comment

Choose a reason for hiding this comment

j-petit commented May 3, 2019 • edited Loading

felixvd May 3, 2019

Choose a reason for hiding this comment

felixvd May 3, 2019

Choose a reason for hiding this comment

j-petit May 3, 2019

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

rhaschke May 3, 2019

Choose a reason for hiding this comment

j-petit May 4, 2019

Choose a reason for hiding this comment

BryceStevenWilley commented May 6, 2019

BryceStevenWilley commented May 7, 2019

BryceStevenWilley commented May 14, 2019

j-petit commented Apr 2, 2019 •

edited

Loading

j-petit commented May 3, 2019 •

edited

Loading