Don't default to random algorithm if no plugin is defined #2228
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #2228 +/- ##
=======================================
Coverage 50.50% 50.50%
=======================================
Files 386 386
Lines 31730 31730
=======================================
Hits 16022 16022
Misses 15708 15708 ☔ View full report in Codecov by Sentry. |
| "Multiple planning plugins available. You should specify the '~planning_plugin' parameter. Using '%s' for " | ||
| "now.", | ||
| planner_plugin_name_.c_str()); | ||
| if (classes.size() == 1) |
There was a problem hiding this comment.
I think that this is a really weird edge case. Reacting to what a use might or might not have installed selectively just asks for hard to reproduce issues. Let's just focus on these options: A) we have a non-empty and valid plugin name and B) we don't. Everything else is a config issue that we should push "up" in the abstraction layer.
sjahr
force-pushed
the
pr-change_planning_pipeline_default_behavior
branch
from
c79db30
to
6340675
Compare
| @@ -194,6 +196,10 @@ void planning_pipeline::PlanningPipeline::configure() | |||
| } | |||
| } | |||
| } | |||
| else | |||
| { | |||
| RCLCPP_INFO(LOGGER, "No planning request adapter names specified."); | |||
There was a problem hiding this comment.
This seems like it should be a warning or error. Is it valid to load MoveIt without any adapters specified?
There was a problem hiding this comment.
I thought about this too and concluded that it is not necessarily something invalid. If you have a super fancy planning plugin that generates a full motion plan response and that is robust without any pre-processing: Go for it. Nonetheless, I think this INFO print statement is nice to have because none of our existing planning plugins can do this.
There was a problem hiding this comment.
I think WARN sounds like it fits better because as you stated we don't have any planning plugins that can perform an end to end solution.
moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp
Outdated
Show resolved
Hide resolved
Description
Currently, if
planner_plugin_name_.empty()& multiple planner plugin classes are defined, we default to the first one (In many cases that is CHOMP because the elements seem to be alphabetically ordered 🤷 ). This PR changes that to throw in case no planner plugin name is defined and multiple options are available because it is more useful to throw here rather than loading a random pipeline.Also, I added throw statements at the end of the plugin lib exception catch blocks to propagate the exception upwards. Please correct me if that is not correct.
Checklist