rework PipelinePlanner creation#249
Conversation
| planning_pipeline::PlanningPipelinePtr PipelinePlanner::create(const PipelinePlanner::Specification& spec) { | ||
| static PlannerCache cache; | ||
|
|
||
| constexpr char PLUGIN_PARAMETER_NAME[]{ "planning_plugin" }; |
There was a problem hiding this comment.
I dropped support for custom plugin parameter names on purpose because the new parameter scheme is a much cleaner replacement for non-standard parameter names.
I'm ok with leaving the parameter configurable as well or hard-code the adapter_param.
I believe there might be sufficient use for a configurable adapter parameter though.
moveit/moveit#2127 merged support for multiple planner plugins. As a result, the planner specification should include a pipeline name from now on. Support the new schema and provide a fallback for older configurations. While doing so, I moved the create methods to the class they actually belong to and introduced a clean options parameter
2b27914 to
a28cb88
Compare
Wow, feels like we are back in the old days... I pushed a patch without the language feature... |
Codecov Report
@@ Coverage Diff @@
## master #249 +/- ##
==========================================
- Coverage 34.26% 34.22% -0.04%
==========================================
Files 93 93
Lines 5596 5603 +7
==========================================
Hits 1917 1917
- Misses 3679 3686 +7
Continue to review full report at Codecov.
|
Sorry, I'm not a fan of the brace initializers. They can be confused easily with initializer lists.
rhaschke
left a comment
There was a problem hiding this comment.
I pushed two more commits:
- I think we can directly remove Task::createPlanner (and not only deprecate it)
- I'm not a fan of brace initializers.
| }; | ||
|
|
||
| static planning_pipeline::PlanningPipelinePtr create(const moveit::core::RobotModelConstPtr& model) { | ||
| return create(Specification{ .model = model }); |
There was a problem hiding this comment.
Isn't this line using the C++20 feature as well? (#249 (comment))
There was a problem hiding this comment.
Yes and no. It's the same feature, but apparently the version of gcc in CI does implicitly support it as long as all initialized members are in order, even if some in the end are not specified. The other use skipped some members and triggered the "sorry unimplemented" error.
I rewrote it here as well.
It's a shame, this is so much less readable...
I do prefer and use uniform initialization a lot these days and I do think they solve actual problems in the language. I'll not touch it again because you went through the trouble of changing it, but I do prefer to avoid printf-style and non-uniform initializers. That aside, this should be good to merge now. |
moveit/moveit#2127 merged support
for multiple planner plugins. As a result, the planner specification
should include a pipeline name from now on.
Support the new schema and provide a fallback for older configurations.
While doing so, I moved the create methods to the class they actually belong to
and introduced a clean options parameter
This change is somewhat urgent because the pickplace demo is broken since moveit/2127 was merged.