-
Notifications
You must be signed in to change notification settings - Fork 695
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
Tutorial on OMPL's Constrained State Space for position or orientation constraints #651
base: master
Are you sure you want to change the base?
Conversation
Thanks for helping in improving MoveIt and open source robotics! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I ran the tutorial (using the appropriate MoveIt branch) and it ran fine. I just suggested a few grammar things.
There are a few places in the text where there are links that would also be appropriate to be formatted in fixed-width font (such as the enforce_joint_model_state_space
option). I'm not familar with reStructuredText well enough to know how to do that (or if it's even possible). It's no big deal either way.
I'd also suggest adding the directory to the root CMakeLists.txt
. I'm guessing if you built this in an installed workspace it wouldn't work without that.
Co-authored-by: John Stechschulte <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it that we finally can add an example with orientation constraints. But it seems it was not added to the tutorial text yet. This PR needs a bit of work, I'm sorry I did not go through it in detail earlier.
I will first focus my attention on the main PR for now. I can always come back here later and fix some of my own review comments myself.
|
||
When can I use this planner? | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
The interface currently only supports position constraints on any link of the robot, where the constrained region is represented using a box_. The planning approach provides an alternative for the `enforce_joint_model_state_space`_ option. It is expected to be most valuable for constraint regions that have a small (or zero) volume in Cartesian space, where the rejection sampling does not always works. For example, keeping the end-effector on a plane or along a line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true anymore as the tutorial contains an example with orientation constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we split the tutorial in two PRs: 1 for position + 1 for orientation (+ eventually pose) ?
Or we keep one PR that will work partly after the moveit position PR is merged and entirely once the moveit orientation PR is merged?
|
||
.. tutorial-formatter:: ./scripts/ompl_constrained_planning_tutorial.py | ||
|
||
.. _this video: https://youtu.be/RkPydgtIq-M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a new video once this is finished.
doc/ompl_constrained_planning/scripts/ompl_constrained_planning_tutorial.py
Outdated
Show resolved
Hide resolved
doc/ompl_constrained_planning/scripts/ompl_constrained_planning_tutorial.py
Show resolved
Hide resolved
# print("============ Press enter to continue with the pose constrained planning problem.") | ||
# input() | ||
# tutorial.remove_all_markers() | ||
# tutorial.remove_obstacle() | ||
|
||
# pcm = tutorial.create_vertical_plane_constraints() | ||
# ocm = tutorial.create_orientation_constraints() | ||
|
||
# path_constraints = moveit_msgs.msg.Constraints() | ||
# path_constraints.orientation_constraints.append(ocm) | ||
# path_constraints.position_constraints.append(pcm) | ||
# pose_goal = tutorial.create_pose_goal_under_obstacle() | ||
# # pcm = tutorial.create_vertical_plane_constraints() | ||
|
||
# move_group.set_start_state(start_state) | ||
# move_group.set_pose_target(pose_goal) | ||
# move_group.set_path_constraints(path_constraints) | ||
# move_group.set_planning_time(1) | ||
# move_group.plan() | ||
# move_group.clear_path_constraints() | ||
|
||
# print("============ Press enter to continue with the pose constrained planning problem with obstacle.") | ||
# input() | ||
# tutorial.remove_all_markers() | ||
# tutorial.add_obstacle() | ||
|
||
# pcm = tutorial.create_vertical_plane_constraints() | ||
# ocm = tutorial.create_orientation_constraints() | ||
|
||
# path_constraints = moveit_msgs.msg.Constraints() | ||
# path_constraints.orientation_constraints.append(ocm) | ||
# path_constraints.position_constraints.append(pcm) | ||
# pose_goal = tutorial.create_pose_goal_under_obstacle() | ||
# # pcm = tutorial.create_vertical_plane_constraints() | ||
|
||
# move_group.set_start_state(start_state) | ||
# move_group.set_pose_target(pose_goal) | ||
# move_group.set_path_constraints(path_constraints) | ||
# move_group.set_planning_time(60) | ||
# move_group.plan() | ||
# move_group.clear_path_constraints() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is replaced by a new version in the tutorial we can remove the code in comments to avoid confusion. I forgot why this is in comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added and could test it. I had in mind to maybe add pose constraints to my moveit PR. I think if we keep orientation constraints in this PR here we can keep pose constraints too. I will wait to see how we arrange the moveit PRs before resolving this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest keeping this PR open until orientation constraints are also merged. So indeed it could be interesting to add this example for real.
Also, I noticed some of the tutorial cases still fail sometimes. (Planner timeout I think?) @gautz if you have ideas to improve this situation they're more than welcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to work on an orientation constraints PR ? Otherwise I could try to cherry-pick relevant commits and open a PR this week.
Solving also fails for collision-avoidance examples around 1/4th of the time. My plan was to test Atlas based solving to see whether it improves performance and reliability. I don't have more ideas, except maybe tuning some numerical parameters.
Maybe we can add a note to the tutorial that performance is still spotty and needs improvement. But it still provides a valid alternative to try when solving constraint problems? (And using Atlas could be one of these future improvements.)
I'm sorry I hoped to have some more time now but, alas, I don't. If you cherry-pick (again) I'll try to be very active in the review :) Thanks again for keeping this alive! |
Description
Tutorial that goes together with the pull request on the main MoveIt repo: moveit/moveit#2742
Co-authored-by: @JeroenDM [email protected]
This PR is consists of:
Required PR in MoveIt:
-moveit/moveit#2742
Checklist