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

[Planning Pipeline Refactoring] #2 Enable chaining planners #2457

Merged
merged 26 commits into from Dec 6, 2023

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Oct 17, 2023

Related PRs:

Description

This PR enables chaining multiple planners. This way it is e.g. possible to create an initial trajectory or reference trajectory for optimizing planners.

TODO

  • Add unittest for getTrajectoryConstraints
  • Write issue to adapt MotionPlanRequest to changes
  • Write issue to make chain respect max. planning time
  • Write issue to update move_group interface
  • Test if Benchmarking still work

Testing

- planner_plugin: my_fancy/Planner1
+ planner_plugins:
+  my_fancy/Planner1 # for example ompl_interface/OMPLPlanner
+  my_fancy/Planner2 # for example stomp_moveit/StompPlanner
  • run ros2 launch moveit2_tutorials pipeline_testbench.launch.py

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@mergify
Copy link

mergify bot commented Oct 18, 2023

This pull request is in conflict. Could you fix it @sjahr?

@sjahr sjahr force-pushed the pr-enable_multiple_planners branch from 01552a9 to 09fb0fd Compare October 23, 2023 11:47
@sjahr sjahr linked an issue Oct 25, 2023 that may be closed by this pull request
9 tasks
@mergify
Copy link

mergify bot commented Oct 27, 2023

This pull request is in conflict. Could you fix it @sjahr?

@sjahr sjahr self-assigned this Nov 16, 2023
@sjahr sjahr force-pushed the pr-enable_multiple_planners branch from 09fb0fd to 2e8c166 Compare November 17, 2023 15:57
Copy link

mergify bot commented Nov 21, 2023

This pull request is in conflict. Could you fix it @sjahr?

@sjahr sjahr marked this pull request as ready for review November 22, 2023 12:39
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.

Overall looks like a reasonable cleanup! Seems there are still 3 commented-out functions that we need to decide what to do with, so let us know how we can help make that decision.

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (1d2bf44) 50.49% compared to head (f59379c) 50.46%.

Files Patch % Lines
...anning/planning_pipeline/src/planning_pipeline.cpp 41.18% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2457      +/-   ##
==========================================
- Coverage   50.49%   50.46%   -0.02%     
==========================================
  Files         387      387              
  Lines       32157    32183      +26     
==========================================
+ Hits        16233    16238       +5     
- Misses      15924    15945      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
@sjahr sjahr requested a review from sea-bass November 27, 2023 17:25
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.

Changes all look good, just have a question about some comments that still say TODO.

Also, there are still CI failures, though I gave a quick look and it has to do with some Fast DDS stuff?


const planning_interface::PlannerManagerPtr& planner_interface = planning_pipeline->getPlannerManager();
if (planner_interface)
// TODO(sjahr): Update for multiple planner plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to get done, or is it updated?

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 needs to be updated. For now the first planner in the chain is used which is usually OMPL. Updating this involves more changes in the visualization with RVIZ and the moveit plugins which is why I'd like to keep this out of the PR

{
context->solve(res);
publishPipelineState(mutable_request, res, planner_instance_->getDescription());
mutable_request.trajectory_constraints.constraints = getTrajectoryConstraints(res.trajectory);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Extend Planning Pipeline to allow calling multiple planners in sequence
3 participants