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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 26 commits into from
Dec 6, 2023
Merged

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

merged 26 commits into from
Dec 6, 2023

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Oct 17, 2023 •
edited
Loading

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

@sjahr sjahr mentioned this pull request Oct 18, 2023
Closed
9 tasks
@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
Extend Planning Pipeline to allow calling multiple planners in sequence #2408
Closed
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
@mergify Mergify
Copy link

mergify bot commented Nov 21, 2023

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

@sjahr sjahr force-pushed the pr-enable_multiple_planners branch from 7d94924 to 6130b20 Compare November 22, 2023 09:38
@sjahr sjahr mentioned this pull request Nov 22, 2023
Merged
@sjahr sjahr marked this pull request as ready for review November 22, 2023 12:39
sea-bass
sea-bass reviewed Nov 22, 2023
View reviewed changes
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.

moveit_configs_utils/default_configs/chomp_planning.yaml Outdated Show resolved Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_interface.h Outdated Show resolved Hide resolved
moveit_ros/benchmarks/include/moveit/benchmarks/BenchmarkExecutor.h Outdated Show resolved Hide resolved
moveit_ros/move_group/src/default_capabilities/query_planners_service_capability.cpp Outdated Show resolved Hide resolved
moveit_ros/move_group/src/default_capabilities/query_planners_service_capability.cpp Outdated Show resolved Hide resolved
moveit_ros/planning/planning_pipeline/include/moveit/planning_pipeline/planning_pipeline.h Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Nov 22, 2023 •
edited
Loading

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.

@sjahr @sea-bass
Apply suggestions from code review
a34e545 
Co-authored-by: Sebastian Castro <[email protected]>
@sjahr sjahr requested a review from sea-bass November 27, 2023 17:25
sea-bass
sea-bass reviewed Nov 27, 2023
View reviewed changes
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?

moveit_ros/move_group/src/default_capabilities/query_planners_service_capability.cpp

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

moveit_ros/move_group/src/default_capabilities/query_planners_service_capability.cpp Show resolved Hide resolved
@sjahr sjahr mentioned this pull request Nov 28, 2023
Draft
2 tasks
sjahr
sjahr commented Nov 30, 2023
View reviewed changes
moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp
{
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.

@sea-bass

@sjahr sjahr mentioned this pull request Dec 6, 2023
Merged
This was referenced Dec 6, 2023
Merged
Merged
@sjahr sjahr merged commit f596b9d into moveit:main Dec 6, 2023
9 of 11 checks passed
@vimior vimior mentioned this pull request Apr 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sea-bass sea-bass sea-bass left review comments

@tylerjw tylerjw tylerjw approved these changes

@henningkayser henningkayser Awaiting requested review from henningkayser

Assignees

@sjahr sjahr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend Planning Pipeline to allow calling multiple planners in sequence
3 participants
@sjahr @tylerjw @sea-bass