Skip to content
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

ompl_interface: uniform & simplified handling of the default planner #371

Conversation

v4hn
Copy link
Member

@v4hn v4hn commented Nov 23, 2016

Eventually #197 (comment) made me look into this part of the code, and I cleaned it up a bit while I changed ompl_interface to respect the default_planner_config setting ...

PR #340 introduced additional logic to select the configuration "group_name[default]" as
the default configuration and sets it to RRTConnect.
But there was already logic in place for this!
The previous default configuration is plainly called "group_name".

This basically reverts #340 and sets the (old and new) default config to RRTConnect.

Additionally, this unifies the default planner setup within the ompl_interface
with the variable "default_planner_config" that can be specified for
each group. This was previously only implemented in the RViz plugin
and could be retrieved via MoveGroupInterface::getDefaultPlannerId,
but is not set there by default.
As a result the planner specified in the variable was always selected in RViz,
but never when an innocent user requested a motionplan in their code,
without explicitly setting the default planner.

PR ros-planning#340 introduced additional logic to select the configuration "group_name[default]" as
the default configuration and sets it to RRTConnect.
But there was already logic in place for this!
The previous default configuration is plainly called "group_name".

This basically reverts ros-planning#340 and sets the (old and new) default config to RRTConnect.

Additionally, this unifies the default planner setup within the ompl_interface
with the variable "default_planner_config" that can be specified for
each group. This was previously only implemented in the RViz plugin
and could be retrieved via `MoveGroupInterface::getDefaultPlannerId`,
but is not set there by default.
As a result the planner specified in the variable was *always* selected in RViz,
but *never* when an innocent user requested a motionplan in their code,
without explicitly setting the default planner.
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By eye this looks good, but I think this needs someone to test during runtime

{
for (int32_t j = 0; j < config_names.size(); ++j)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially you moved this logic to loadPlannerConfiguration()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of it, yes.
I did this to make use of it to load the specified default planner above.

@rbbg
Copy link
Contributor

rbbg commented Nov 24, 2016

Looks good. Just to clarify; the reason I went for group_name[default] instead of just group_name is because of the following comment in planning_interface.h:

/* Name of the configuration. If there is only one configuration, this should be the same as the group name. If there are multiple configurations, the form "group_name[config_name]" is expected for the name. */

I wasn't sure whether any piece of code actually relied on there being only one when a configuration called group_name is available, so I figured it was a little safer to add a new one.

The ompl_interface code always implemented it differently
and it makes sense to assume there is always a default "group_name"
configuration around.
@v4hn v4hn force-pushed the pr-kinetic-cleanup-ompl-default-config branch from 4cd8242 to 56c392a Compare November 24, 2016 12:48
@v4hn
Copy link
Member Author

v4hn commented Nov 24, 2016

Just to clarify; the reason I went for group_name[default] instead of just group_name is because of the following comment in planning_interface.h:

Ah, that makes sense. I didn't know you reasoned like this.
I just looked into the history of the comment and the code and it looks like the comment never agreed with the actual implementation in ompl_interface.

I propose to change the comment there too. Imho, there is no reason why a "group_name" (default) config should not be allowed when multiple configs are available.
I added a commit to the request.

@v4hn
Copy link
Member Author

v4hn commented Nov 24, 2016

I tested this, it would be great if someone else could give it a try.

@davetcoleman
Copy link
Member

I tested this locally with the UR5 and moveit's demo.launch

@davetcoleman davetcoleman merged commit 1d7df4f into ros-planning:kinetic-devel Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants