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

Parallel planning example #551

Merged
merged 37 commits into from
Dec 9, 2022
Merged

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Nov 28, 2022

Description

This PR adds an example of how to use the recently merged parallel planning interface for moveit2. To test it, run

ros2 launch moveit2_tutorials parallel_planning_example.launch.py

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

@sjahr sjahr force-pushed the pr_parallel_planning_example branch from 2eb9bc2 to ea2457b Compare November 28, 2022 15:56
@sjahr sjahr force-pushed the pr_parallel_planning_example branch from 0fae6e4 to db1fe1d Compare November 28, 2022 16:38
@sjahr sjahr requested a review from sea-bass November 28, 2022 16:39
Copy link
Contributor

@mamoll mamoll left a comment

Choose a reason for hiding this comment

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

Looks good. I'd add another ompl planner to the example config. and show how you can use multiple planners from the same planner pipeline.

CMakeLists.txt Outdated Show resolved Hide resolved
@stephanie-eng
Copy link
Contributor

stephanie-eng commented Nov 28, 2022

Your title is a bit long, which makes the formatting of the header a bit crowded:
image

This is really not a real issue.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

This looks great and worked for me no problem.

It would be nice if the tutorial printed not just how long a solution tool, but which planner ID was being used. I know the info is there if you read all the node names, but it would be a nice convenience to the user to just show things in one line.

Also, this doesn't have to be for this PR, but having a MoveGroup Interface example would be good as well.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Did another pass, but it looks good for me after these small tweaks.

@sjahr sjahr requested review from sea-bass and ChrisThrasher and removed request for ChrisThrasher December 7, 2022 18:33
Copy link
Contributor

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

No more comments

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

I'm good with this PR, but have more comments to add if you want!

node_, { "ompl_rrtc", "pilz_lin", "chomp" }
};

The constructor of this function will initialize :code:`PlanningRequestParameter` configurations based on the config that is provided in the node's
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be MultiPipelinePlanRequestParameters, or am I wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I rephrased the sentence to make it clearer what I am talking about:

The constructor of this class will initialize multiple :code:`PlanningRequestParameter` class members

Comment on lines +192 to +204
void setJointGoal(double const panda_joint1, double const panda_joint2, double const panda_joint3,
double const panda_joint4, double const panda_joint5, double const panda_joint6,
double const panda_joint7)
{
auto robot_goal_state = planning_component_->getStartState();
robot_goal_state->setJointPositions("panda_joint1", &panda_joint1);
robot_goal_state->setJointPositions("panda_joint2", &panda_joint2);
robot_goal_state->setJointPositions("panda_joint3", &panda_joint3);
robot_goal_state->setJointPositions("panda_joint4", &panda_joint4);
robot_goal_state->setJointPositions("panda_joint5", &panda_joint5);
robot_goal_state->setJointPositions("panda_joint6", &panda_joint6);
robot_goal_state->setJointPositions("panda_joint7", &panda_joint7);

// Set goal state
planning_component_->setGoal(*robot_goal_state);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

... part of me wants to recommend that this accept a std::vector or something, but it's fine if you don't want to do that extra work.

If you went that way you'd have to loop through like this (UNTESTED)

  void setJointGoal(std::vector<double> const& panda_joint_angles)
  {
    size_t idx = 0;
    auto robot_goal_state = planning_component_->getStartState();
    for (const auto& joint_angle : panda_joint_angles) { 
        robot_goal_state->setJointPositions("panda_joint" + std::to_string(idx++), joint_angle);
    }
    planning_component_->setGoal(*robot_goal_state);
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see you point. Maybe this would be worth adding to the PlanningComponent class

@sjahr sjahr requested a review from sea-bass December 9, 2022 17:41
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Few small things, I'm good after this

Comment on lines 61 to 65
"request_adapters": """default_planner_request_adapters/AddTimeOptimalParameterization \
default_planner_request_adapters/FixWorkspaceBounds \
default_planner_request_adapters/FixStartStateBounds \
default_planner_request_adapters/FixStartStateCollision \
default_planner_request_adapters/FixStartStatePathConstraints""",
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is weird here

Comment on lines 31 to 32
provided by the :code:`parameters`, multiple threads are launched and each tries to solve the planning problem with a different planning pipeline. Once
all pipelines have been terminated. Please keep in mind, that no solution is also a possible result, the :code:`solution_selection_callback` is called to determine which
Copy link
Contributor

Choose a reason for hiding this comment

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

I commented on this earlier but it's gone away.

This "Once all pipelines have been terminated." sentence looks like an incomplete sentence that was left behind. I'm not sure what this means as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I put Please keep in mind, that no solution is also a possible result. in the middle of a sentence

Comment on lines 71 to 98
# PlanRequestParameters for the first parallel pipeline
ompl_rrtc:
plan_request_params:
planning_attempts: 1
planning_pipeline: ompl
planner_id: "RRTConnectkConfigDefault"
max_velocity_scaling_factor: 1.0
max_acceleration_scaling_factor: 1.0
planning_time: 0.5

# PlanRequestParameters for the second parallel pipeline
pilz_lin:
plan_request_params:
planning_attempts: 1
planning_pipeline: pilz_industrial_motion_planner
planner_id: "LIN"
max_velocity_scaling_factor: 1.0
max_acceleration_scaling_factor: 1.0
planning_time: 0.8

# PlanRequestParameters for the third parallel pipeline
chomp:
plan_request_params:
planning_attempts: 1
planning_pipeline: chomp
max_velocity_scaling_factor: 1.0
max_acceleration_scaling_factor: 1.0
planning_time: 1.5
Copy link
Contributor

Choose a reason for hiding this comment

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

For these comments, maybe add more than just "first, second, and third" and describe them as you did in the fourth one?

@sjahr sjahr force-pushed the pr_parallel_planning_example branch from e39578b to d7b0438 Compare December 9, 2022 21:06
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

SHIP IT

@sjahr sjahr merged commit ee4f9b2 into moveit:main Dec 9, 2022
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