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

Add planner configurations to CHOMP and PILZ #1522

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Aug 16, 2022

Description

Right now it is not possible to use planners other than OMPL with the moveit_cpp interface. The reason for this is the following:

When the plan() method of the planning component gets called, all available planning pipelines are searched based on their name [1]. These names are provided by moveit_cpp [2]. The moveit_cpp function that provides the planning pipeline names reads them from an internal map [3]. A planning pipelines needs to have a planner plugin that has a PlannerConfiguration in order to be added to this map [4]. This PR adds the missing configuations to CHOMP and PILZ to make them usable in moveit_cpp and in addition to that it makes the PlannerManager's setPlannerConfigurations function pure virtual to remind futur planning plugin implementations to add a planner configuation

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

@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #1522 (a4c9caa) into main (042709a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1522      +/-   ##
==========================================
+ Coverage   50.97%   50.98%   +0.01%     
==========================================
  Files         380      380              
  Lines       31758    31768      +10     
==========================================
+ Hits        16186    16194       +8     
- Misses      15572    15574       +2     
Impacted Files Coverage Δ
...al_motion_planner/pilz_industrial_motion_planner.h 100.00% <ø> (ø)
...ion_planner/src/pilz_industrial_motion_planner.cpp 100.00% <100.00%> (ø)
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.36% <0.00%> (-1.07%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.44% <0.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sjahr sjahr requested a review from JafarAbdi August 16, 2022 21:30
@tylerjw tylerjw force-pushed the pr-add_missing_planner_configs branch from cd8f9fc to 386dd9a Compare August 23, 2022 14:30
@sjahr sjahr force-pushed the pr-add_missing_planner_configs branch from 386dd9a to 2a3c773 Compare August 24, 2022 14:45
@sjahr sjahr force-pushed the pr-add_missing_planner_configs branch from 2a3c773 to a4c9caa Compare August 24, 2022 15:40
@tylerjw tylerjw merged commit 888fc53 into moveit:main Aug 24, 2022
@tylerjw tylerjw deleted the pr-add_missing_planner_configs branch August 24, 2022 16:06
@tylerjw tylerjw added the backport-humble Mergify label that triggers a PR backport to Humble label Oct 28, 2022
mergify bot pushed a commit that referenced this pull request Oct 28, 2022
tylerjw pushed a commit that referenced this pull request Oct 28, 2022
(cherry picked from commit 888fc53)

Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 10, 2022
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 10, 2022
This attempts to cleanup the mess in configuring multiple planning pipelines,
which exists since the very beginning and was changed back and forth since then,
see moveit#1096, moveit#1114, moveit#1522.

This PR removes MoveItCpp::getPlanningPipelineNames(), which was obviously intended
initially to provide a planning-group-based filter for all available planning pipelines:
A pipeline was discarded for a group, if there were no `planner_configs` defined
for that group on the parameter server.

As pointed out in moveit#1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), moveit#1522
introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless.
Thus, here we just remove the function getPlanningPipelineNames().
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 10, 2022
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 10, 2022
This attempts to cleanup the mess in configuring multiple planning pipelines,
which exists since the very beginning and was changed back and forth since then,
see moveit#1096, moveit#1114, moveit#1522.

This PR removes MoveItCpp::getPlanningPipelineNames(), which was obviously intended
initially to provide a planning-group-based filter for all available planning pipelines:
A pipeline was discarded for a group, if there were no `planner_configs` defined
for that group on the parameter server.

As pointed out in moveit#1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), moveit#1522
introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless.
Thus, here we just remove the function getPlanningPipelineNames().
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 11, 2022
rhaschke added a commit to rhaschke/moveit2 that referenced this pull request Nov 11, 2022
This attempts to cleanup the mess in configuring multiple planning pipelines,
which exists since the very beginning and was changed back and forth since then,
see moveit#1096, moveit#1114, moveit#1522.

This PR removes MoveItCpp::getPlanningPipelineNames(), which was obviously intended
initially to provide a planning-group-based filter for all available planning pipelines:
A pipeline was discarded for a group, if there were no `planner_configs` defined
for that group on the parameter server.

As pointed out in moveit#1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), moveit#1522
introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless.
Thus, here we just remove the function getPlanningPipelineNames().
sjahr pushed a commit to rhaschke/moveit2 that referenced this pull request Nov 15, 2022
sjahr pushed a commit to rhaschke/moveit2 that referenced this pull request Nov 15, 2022
This attempts to cleanup the mess in configuring multiple planning pipelines,
which exists since the very beginning and was changed back and forth since then,
see moveit#1096, moveit#1114, moveit#1522.

This PR removes MoveItCpp::getPlanningPipelineNames(), which was obviously intended
initially to provide a planning-group-based filter for all available planning pipelines:
A pipeline was discarded for a group, if there were no `planner_configs` defined
for that group on the parameter server.

As pointed out in moveit#1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), moveit#1522
introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless.
Thus, here we just remove the function getPlanningPipelineNames().
sjahr pushed a commit to rhaschke/moveit2 that referenced this pull request Nov 15, 2022
sjahr pushed a commit to rhaschke/moveit2 that referenced this pull request Nov 15, 2022
This attempts to cleanup the mess in configuring multiple planning pipelines,
which exists since the very beginning and was changed back and forth since then,
see moveit#1096, moveit#1114, moveit#1522.

This PR removes MoveItCpp::getPlanningPipelineNames(), which was obviously intended
initially to provide a planning-group-based filter for all available planning pipelines:
A pipeline was discarded for a group, if there were no `planner_configs` defined
for that group on the parameter server.

As pointed out in moveit#1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), moveit#1522
introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless.
Thus, here we just remove the function getPlanningPipelineNames().
rhaschke added a commit that referenced this pull request Nov 15, 2022
* Revert "Add planner configurations to CHOMP and PILZ (#1522)"
* Cleanup lookup of planning pipelines
   Remove MoveItCpp::getPlanningPipelineNames(), which was obviously intended initially to provide a planning-group-based filter for all available planning pipelines: A pipeline was discarded for a group, if there were no `planner_configs` defined for that group on the parameter server.

As pointed out in #1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well (and thus circumventing the original filter mechanism), #1522 introduced empty dummy planner_configs for all other planners as well (CHOMP + Pilz).

This, obviously, renders the whole filter mechanism useless. Thus, here we just remove the function getPlanningPipelineNames() and the corresponding member groups_pipelines_map_.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants