As far as I could see, both start_state_ and planner_id_ are not actually used. What's the purpose of these fields?

It is possible to pass an optional solution_selection_callback and/or a stopping_criterion_function both use the produced MotionPlanResponses to make a decision and these to additional fields might be useful to make that decision. For example

bool stoppingCriterionCallBack(
    moveit_cpp::PlanningComponent::PlanSolutions const& plan_solutions,
    moveit_cpp::PlanningComponent::MultiPipelinePlanRequestParameters const& plan_request_parameters)
  for (auto const& solution : solutions)
  {
      // Stop parallel planning if PILZ found a solution
      if (solution.planner_id_ == "LIN")
      {
        return true;
      }
  }
 return false;
}

I agree for the planner_id_, but all plans share the very same start state, don't they?
Please add a comment, e.g. "planner this solution originates from".

That's true, it does not matter for the solution selection. I added it since the MotionPlanResponse message has a similar field and it can be useful when the MotionPlanResponse is processed further independent from moveit_cpp/ the planning_component. Do you think we should remove it or is a comment sufficient?

I'm not sure yet. If the start state is identical for all solutions, saving it here would be a waste of memory.
However, due to different pipeline configs, modifying the start state (or not), the actual start state might differ between solutions (but I don't know by heart). That would be an argument to keep it here...

I'd remove the underscore from the field names but you're right, the API will be broken by this. I can open a PR for moveit_tutorials to fix this. Would MTC be affected?

I'm not only worried about moveit_tutorials or MTC. The problem is that any user code out there accessing those members will be affected by this change! At least we should provide a deprecation period for transition.
My suggestion is to:

• remove the underscores
• provide additional, deprecated members with the underscore as const references to the new members:

  moveit_msgs::RobotState start_state;
  [[deprecated("Use start_state instead.")]] const moveit_msgs::RobotState& start_state_;

  Obviously, those references need to be initialized in the constructor. This will render the struct read-only. So you will need to provide a custom copy constructor and assignment operator as well.

Same here.

What's the purpose of setting these empty planner configs?

Isn't it only empty when the robot model does not have any JointModelGroups? The line above adds for each joint model group settings

But those settings are simply empty: <std::string, std::string>()

Maybe introduce this as the default implementation of PlannerManager?

Personally, I'd like to remind the person that implements a new planner plugin to think about the planner config and forcing them to implement this function would be a good way. But I can make this a default implementation if you don't like that

I understand your reasoning and for MoveIt2 it is probably the right way to go.
For MoveIt1, I personally think we should maintain API stability. Other maintainers might have a different opinion.

Reverting this will be required for CI to pass...

Ok, let's do it that way then 👍

Why did you decide to initialize settings for "missing" groups here instead of in PlannerManager::initialize as suggested here?

Sorry, you reviewed to fast, that change did not work. The problem is that initialize() function can easily be overwritten and thus compatibility with moveit_cpp be destroyed. The way I implemented it would have added a default empty config for all planning plugins with setPlannerConfigurations. But that did not work because the robot model is not available in this function.

I think the planner plugins should be modified because the flaw right now is that pconfig is not used correctly (as expected by moveit_cpp) by the plugins. If we hide this implementation detail away in the base class, I fear future plugin implementations will repeat the same error,

pconfig is not used correctly (as expected by moveit_cpp) by the plugins.

Can you please detail this? As far as I can see, moveit_cpp just generates a "shortcut" map from these configs.
If there are no config settings, why should a plugin declare them as empty? I think we should first agree on the semantics, before implementing anything further...

My preference would be to use the current solution and open an issue to re-evaluate the PlannerConfigurationSettings, since it is only properly used by OMPL at the moment. Maybe it would be better to remove/refactor it and the pipeline detection in moveit_cpp but I think this is out of the scope of this PR.

From: moveit/moveit2#1522

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].

Why does the OMPL planner use a different approach to configure settings?
I would expect a similar approach to other planners, i.e. calling setPlannerConfigurations(pconfig).

I like the way, ompl defined a default setPlannerConfigurations implementation to add empty configs for joint model groups and moved that implementation into the base class. But that did not work

@rhaschke Probably I am doing something wrong, but if I make these references const, the API breaks again. Do I miss something here?