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

Use RRTConnect planning algorithm by default #196

Merged
merged 3 commits into from
Jul 14, 2015
Merged

Use RRTConnect planning algorithm by default #196

merged 3 commits into from
Jul 14, 2015

Conversation

marcoesposito1988
Copy link
Contributor

Fixes bug #193 about slow planning on Indigo

LBKPIECE1 (the previous default) looks to be the wrong planning algorithm for the robot
See also: https://groups.google.com/forum/#!topic/moveit-users/M71T-GaUNgg

Marco Esposito added 2 commits April 10, 2015 14:31
Fixes bug #193 about slow planning on Indigo

LBKPIECE1 (the previous default) looks to be the wrong planning algorithm for the robot
See https://groups.google.com/forum/#!topic/moveit-users/M71T-GaUNgg
Fixes bug #193 about slow planning on Indigo

LBKPIECE1 (the previous default) looks to be the wrong planning algorithm for the robot
See https://groups.google.com/forum/#!topic/moveit-users/M71T-GaUNgg
@@ -34,7 +34,6 @@ manipulator:
- TRRTkConfigDefault
- PRMkConfigDefault
- PRMstarkConfigDefault
projection_evaluator: joints(shoulder_pan_joint,shoulder_lift_joint)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to configure a specific planner?

If I understood you correctly, removing the projection setting leads MoveIt to choose something other than LBKPIECE1 which is good, but if the selection behaviour changes, we could run into issues again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the suggestion of the OMPL developers. I guess that removing the projection makes the robot "fall" into the most generic class, so I hope that if they change the default algorithm for that class, it would still behave best for the robot among the available planning algorithms.

But I am actively looking for a way to be more specific.

@davetcoleman
Copy link
Contributor

As I'm sure you know, this is not the cleanest way to specify what planner to use. Having a projection_evaluator should not hurt anything but I guess the way things are currently setup, it does cause LBKPIECE to be used. You could change the used planner manually in the Rviz plugin, but I'm afraid at the moment there is no feature to load a default planner from a configuration or roslaunch file. This would be a good feature to add.

@davetcoleman
Copy link
Contributor

Also, to directly answer @ipa-fxm email questions:

Do you know whether there is any reason for why LBKPIECE should not be feasible for the URX's?

At the moment, no. -2PI to 2PI is a somewhat large configuration space, but it should still work, just slower.

Is LBKPIECE chosen as default planner because of the "projection_evaluator" entry in the ompl_planning.yaml?

Yes, here is the exact logic: https://github.com/ompl/ompl/blob/master/src/ompl/tools/config/src/SelfConfig.cpp#L267

Is there any other (more obvious) way to specify which planner is to be used by default within the ompl_planning.yaml?

No, but I don't think it would be hard to add this feature

Is the threaded/parallel planning working or are there indeed some known issues....

If you specify planning attemtps > 1, then yes. If your computer has 4 threads and you specify 4 attempts, then 4 threads are run. If you say 8 attempts, then two runs are done on each of the four threads. See https://github.com/ros-planning/moveit_planners/blob/indigo-devel/ompl/ompl_interface/src/model_based_planning_context.cpp#L534

@gavanderhoorn
Copy link
Member

@davetcoleman: how involved do you think it would be to implement the infrastructure in MoveIt to be able to configure a specific planner from the semantic robot description file?

@davetcoleman
Copy link
Contributor

specifying a planner is outside the domain of the SRDF, and I believe is a bad place for it. it should be in ompl_planners.yaml or the move_group launch file

@gavanderhoorn
Copy link
Member

it should be in ompl_planners.yaml or the move_group launch file

Ok, I agree the srdf is the wrong location, but the question remains. Would it just be a matter of reading in an additional field from the yaml, then propagating that up to a config struct and extending the logic you linked?

@davetcoleman
Copy link
Contributor

I think the easiest would read in a rosparam loaded from the roslaunch file inside this file:
https://github.com/ros-planning/moveit_ros/blob/indigo-devel/visualization/motion_planning_rviz_plugin/src/motion_planning_frame_planning.cpp

It would then set the QT dropdown box to the appropriate default planner. This is a very superficial change that only would affect the rviz plugin (so is safe to merge)

@gavanderhoorn
Copy link
Member

Right, but what about the use case where only move_group.launch is used? So no RViz plugin? We could decide to just move/leave the responsibility of configuring a planner to the user, but it seems like being able to configure the (default) planner through an item in a configuration file should be possible.

It would at least remove the need for users to implement that functionality themselves and / or recompile their initialisation code.

@davetcoleman
Copy link
Contributor

If you are using the move_group actionlib interface, it is easy to specify what planner to use in the Request message, so I don't think the ability to specify the default at a low level is super necessary. But if you so desire, I think it would fit well into the moveit_ompl_interface package here: https://github.com/ros-planning/moveit_planners/blob/indigo-devel/ompl/ompl_interface/src/model_based_planning_context.cpp#L544
(allow option to override getDefaultPlanner)

@marcoesposito1988
Copy link
Contributor Author

I think there should be an entry in ompl_planning.yaml, which can then be overridden through the move_group interface and the launch files.

But I also think that this discussion should happen elsewhere; this pull request is just a try at having a suitable default configuration for the UR5, which is currently lacking. A new user would just fire it up, try it out and close it immediately because it "doesn't work". I think we should fix this as soon as possible, since there is a quick and easy way to do it, and then do things properly as soon as the infrastructure has been laid out.

@davetcoleman
Copy link
Contributor

I agree the discussion should be moved to moveit_ros

@shaun-edwards
Copy link
Member

Is this PR still alive? Should it be closed?

@marcoesposito1988
Copy link
Contributor Author

@shaun-edwards: I also wonder what the hold-up is. This is a fix to an existing problem that makes the package unusable as it is out-of-the-box.

As for reasons against merging it, there seem to be no concrete one.

@fmessmer
Copy link
Contributor

I was waiting with the merge as this PR is a workaround for an issue within moveit....
I'm ok with merging this, but I think, I would rather like to have the deleted line commented out and a comment added explaining it. Something like:

##Note: commenting the following line lets moveit chose RRTConnect as default planner rather than LBKPIECE
#projection_evaluator: joints(shoulder_pan_joint,shoulder_lift_joint)

@marcoesposito1988 would that do for you?

@marcoesposito1988
Copy link
Contributor Author

@ipa-fxm: definitely, I updated the pull request.

Thanks!

@fmessmer
Copy link
Contributor

Thanks, I think this better helps to understand why these configs differ from the default config generated by the SetupAssistant!

fmessmer added a commit that referenced this pull request Jul 14, 2015
Use RRTConnect planning algorithm by default
@fmessmer fmessmer merged commit 4cfe91d into ros-industrial:indigo-devel Jul 14, 2015
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.

5 participants