Skip to content

Cleanup lookup of planning pipelines in MoveItCpp#1710

Merged
rhaschke merged 3 commits intomoveit:mainfrom
rhaschke:cleanup-planning-configs
Nov 15, 2022
Merged

Cleanup lookup of planning pipelines in MoveItCpp#1710
rhaschke merged 3 commits intomoveit:mainfrom
rhaschke:cleanup-planning-configs

Conversation

@rhaschke
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke commented Nov 10, 2022

This PR attempts to cleanup the mess in configuring multiple planning pipelines in MoveItCpp, which exists since the very beginning and was changed back and forth since then, see #1096, #1114, #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 #1522, only OMPL actually explicitly declares planner_configs on the parameter server.
To enable all other pipelines as well, #1522 introduced empty dummy planner_configs for all other planners (CHOMP + Pilz) - and thus, obviously, circumventing the original filter mechanism!

As the filter mechanism is useless now, we can just remove the function getPlanningPipelineNames() and revert #1522 instead.
@sjahr, this is what I was thinking of for moveit/moveit#3244 (actually the PR here simplifies even more).

@henningkayser
Copy link
Copy Markdown
Member

henningkayser commented Nov 10, 2022 •
edited
Loading

Are you suggesting to remove the PlannerConfigurationSettings as well? I think this would be the direct consequence of omitting the interface, and I would agree that it would make a lot of sense to do so. Also, if we don't require a mapping, the groups_pipelines_map_ should also be removed with this PR.

Otherwise, a simple PlannerManager::canServiceGroup() would be a more elegant solution if we do want to filter for groups.

@rhaschke rhaschke force-pushed the cleanup-planning-configs branch from a628faf to 44da0c9 Compare November 10, 2022 14:18
@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Nov 10, 2022

Are you suggesting to remove the PlannerConfigurationSettings as well? I think this would be the direct consequence of omitting the interface.

No, not at all. I think it makes a lot of sense to allow different planner configs for different groups!
The PR here just provides a fallback in case a planner plugin doesn't setup any config settings. Most user's assumption is that the plugin will serve all groups will all its planning capabilities = planners.

Also, if we don't require a mapping, the groups_pipelines_map_ should also be removed with this PR.

I agree with that and pushed another commit.

@rhaschke rhaschke marked this pull request as draft November 10, 2022 14:37
@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Nov 10, 2022

Sorry for using GHA as a compiler. I don't have a ROS2 system running right now.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 10, 2022 •
edited
Loading

Codecov Report

Base: 50.99% // Head: 50.97% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (f2288a9) compared to base (4c26cbe).
Patch has no changes to coverable lines.

❗ Current head f2288a9 differs from pull request most recent head 5025cc4. Consider uploading reports for the commit 5025cc4 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1710      +/-   ##
==========================================
- Coverage   50.99%   50.97%   -0.01%     
==========================================
  Files         378      378              
  Lines       31653    31639      -14     
==========================================
- Hits        16138    16125      -13     
+ Misses      15515    15514       -1
Impacted Files Coverage Δ
...al_motion_planner/pilz_industrial_motion_planner.h 100.00% <ø> (ø)
...ion_planner/src/pilz_industrial_motion_planner.cpp 100.00% <ø> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 66.45% <0.00%> (-1.49%) ⬇️
moveit_ros/moveit_servo/src/pose_tracking.cpp 76.78% <0.00%> (-0.47%) ⬇️
...anning_scene_monitor/src/current_state_monitor.cpp 75.51% <0.00%> (-0.09%) ⬇️
...veit_servo/include/moveit_servo/servo_parameters.h 100.00% <0.00%> (ø)
...ssistant/moveit_setup_framework/src/rviz_panel.cpp 0.00% <0.00%> (ø)
...g/rdf_loader/src/synchronized_string_parameter.cpp 32.82% <0.00%> (ø)
...nclude/moveit/robot_state/cartesian_interpolator.h 50.00% <0.00%> (ø)
...include/moveit/robot_trajectory/robot_trajectory.h 98.72% <0.00%> (+0.02%) ⬆️
... and 4 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sjahr
sjahr approved these changes Nov 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

I tested this with OMPL and PILZ and multiple pipelines are usable. CI failure is not cause by this PR but unrelated clang-tidy errors that are probably fixed by #1706.

@rhaschke rhaschke marked this pull request as ready for review November 10, 2022 21:32
@rhaschke
Copy link
Copy Markdown
Contributor Author

rhaschke commented Nov 10, 2022

@sjahr, if this gets approved here, can you please adapt moveit/moveit#3244 correspondingly?

@rhaschke rhaschke force-pushed the cleanup-planning-configs branch from f2288a9 to 4734030 Compare November 11, 2022 07:25
@sjahr
Copy link
Copy Markdown
Contributor

sjahr commented Nov 11, 2022 •
edited
Loading

@sjahr, if this gets approved here, can you please adapt ros-planning/moveit#3244 correspondingly?

Yes, thanks a lot for contributing these changes 🙏

rhaschke added a commit to ubi-agni/moveit that referenced this pull request Nov 11, 2022
@rhaschke
Cleanup lookup of planning pipelines
08a48b3 
This is a backport of moveit/moveit2#1710,
dropping function getPlanningPipelineNames().
rhaschke added a commit to moveit/moveit that referenced this pull request Nov 11, 2022
@sjahr @rhaschke
Allow planning with multiple pipelines in parallel with moveit_cpp (#…
5e6f817 
…3244)

This is a backport of:
- moveit/moveit2#1420
- moveit/moveit2#1710

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@sjahr sjahr force-pushed the cleanup-planning-configs branch from 4734030 to d7fce96 Compare November 15, 2022 08:30
@rhaschke @sjahr
Revert "Add planner configurations to CHOMP and PILZ (moveit#1522)"
8f99a1e 
This reverts commit 888fc53.
@rhaschke @sjahr
Cleanup lookup of planning pipelines
691d963 
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 @sjahr
Remove MoveItCpp::groups_pipelines_map_
5025cc4 
This member is not needed anymore.
@sjahr sjahr force-pushed the cleanup-planning-configs branch from d7fce96 to 5025cc4 Compare November 15, 2022 17:26
@rhaschke rhaschke merged commit 2dfdad0 into moveit:main Nov 15, 2022
@rhaschke rhaschke deleted the cleanup-planning-configs branch November 15, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningkayser henningkayser henningkayser approved these changes

@sjahr sjahr sjahr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rhaschke @henningkayser @sjahr