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

WIP: Tutorial on using OMPL's Constrained State Space #518

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

JeroenDM
Copy link
Contributor

Description

The tutorial that goes together with the pull request on the main MoveIt repo:
moveit/moveit#2273

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@JeroenDM
Copy link
Contributor Author

The clang-format check fails for some C++ tutorials. (I used black for formatting Python code, I hope that's ok.)

Copy link
Contributor

@ommmid ommmid left a comment

Choose a reason for hiding this comment

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

It looks good except that creating another class here (ConstrainedPlanningTutorial) might confuse some readers, I would prefer not to have any new class. For example, move_group.plan() is more familiar for readers comparing to tutoial.solve() where they have to figure out what solve() function is here. Moreover, it (not creating any new classes for tutorials) would be more consistent with other parts of the MoveIt tutorial if I am not mistaken

@JeroenDM
Copy link
Contributor Author

@ommmid Thank you for the review!

It looks good except that creating another class here (ConstrainedPlanningTutorial) might confuse some readers, I would prefer not to have any new class. For example, move_group.plan() is more familiar for readers comparing to tutoial.solve() where they have to figure out what solve() function is here. Moreover, it (not creating any new classes for tutorials) would be more consistent with other parts of the MoveIt tutorial if I am not mistaken

I borrowed the approach with a class from the existing Python tutorial. But whenever they use the move_group, they add the following lines at the start of a method:

# Copy class variables to local variables to make the web tutorials more clear.
# In practice, you should use the class variables directly unless you have a good
# reason not to.
 move_group = self.move_group

Is the self keyword to confusing?
I agree that the solve function is confusing, I will remove it.

The advantage of using a class is that it encapsulates variables that are used all over the place, such as the name of the end-effector link. This would otherwise be global variables and function input arguments whenever they are used. The class approach also better reflects how a user might program its own application in a structured way, maybe?

But I will happily create a non-class version so we can compare the two.

@JeroenDM
Copy link
Contributor Author

I removed the solve function and now the final part of the tutorial explicitly uses move_group to solve the planning problem. (Instead of first defining a separate function that combines all these steps.)

# Now we have everything we need to configure and solve a planning problem
move_group.set_start_state(start_state)
move_group.set_pose_target(pose_goal)

# Don't forget the path constraints! That's the whole point of this tutorial.
move_group.set_path_constraints(path_constraints)

# And let the planner find a solution.
# The move_group node should automatically visualize the solution in Rviz if a path is found.
move_group.plan()

# Clear the path constraints for our next experiment
move_group.clear_path_constraints()

@PeterMitrano
Copy link

Just wanted to say I would really love to use all this constrained planning stuff!

@AndyZe
Copy link
Member

AndyZe commented May 30, 2022

Hey @PeterMitrano, it is there in MoveIt2! (In a PR, waiting to be merged.) It even has a good tutorial already.

Unfortunately one thing we've found so far -- you can only apply position constraints OR orientation constraints. Not both at the same time :(

moveit/moveit2_tutorials#386

@v4hn
Copy link
Contributor

v4hn commented May 30, 2022 via email

@aarsht7
Copy link

aarsht7 commented Nov 11, 2022

Hey @JeroenDM Wondering if it is implemented and make it available in default ros moveit installation or do I need to build some branch from source?
Thanks :)

@c-oechsner
Copy link

c-oechsner commented Aug 10, 2023

Hey @JeroenDM Wondering if it is implemented and make it available in default ros moveit installation or do I need to build some branch from source? Thanks :)

Yes, I am also trying to figure this out at the moment. Did you find an answer yet?

@aarsht7
Copy link

aarsht7 commented Aug 10, 2023

Hey @JeroenDM Wondering if it is implemented and make it available in default ros moveit installation or do I need to build some branch from source? Thanks :)

Yes, I am also trying to figure this out at the moment. Did you find an answer yet?

I tried with cpp and it is kind of working ig. Although for the first time in my life it looked more difficult with python as i haven't figured it out yet 😆

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.

7 participants