Cleanup planning request adapter interface

[sjahr]

Description

Cleans up the planning_request_adapter code and updates param handling to use generate_paramters_library.

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]

Codecov Report

Patch and project coverage have no change.

Comparison is base (cdf7a98) 50.50% compared to head (e1e8360) 50.50%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2266   +/-   ##
=======================================
  Coverage   50.50%   50.50%           
=======================================
  Files         386      386           
  Lines       31732    31732           
=======================================
  Hits        16022    16022           
  Misses      15710    15710           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sjahr]

@sea-bass Thank you!

[kylc]

This pattern of passing the default std::vector<std::size_t>& added_path_index = EMPTY_PATH_INDEX_VECTOR isn't making sense to me. It is an out-parameter? If the user doesn't pass their own, does it not mutate the global EMPTY_PATH_INDEX_VECTOR, which surely is not the intent?

[kylc]

Also note that clang-tidy CI is complaining about the naming of this and failing.

[sjahr]

My intend with this was to remove these functions:

bool PlanningRequestAdapterChain::adaptAndPlan(const planning_interface::PlannerManagerPtr& planner,
                                               const planning_scene::PlanningSceneConstPtr& planning_scene,
                                               const planning_interface::MotionPlanRequest& req,
                                               planning_interface::MotionPlanResponse& res) const
{
  std::vector<std::size_t> empty_added_path_index;
  return adaptAndPlan(planner, planning_scene, req, res, empty_added_path_index);
}

As far as I understand there are no optional references, so like before we just write the path indices to a memory location we never read and dump them that way. Maybe another possible implementation would be to change this interface to use a pointer but that would be a bigger change. Do you have a smarter idea of how to do this?

[henningkayser]

can't you just use std::vector<std::size_t>& added_path_index = {}?

[kylc]

@henningkayser C++ won't let you assign a default reference like that.

@sjahr I guess this pattern will technically work as long as adaptAndPlan makes sure to clear the "empty" vector before it starts every time. Otherwise it's a memory leak!

[henningkayser]

oh of course not... content-wise, this parameter would fit into the MotionPlanResponse as well, or at least the detailed response. I wouldn't sweat it too much since we should take another close look at this interface anyway.

[kylc]

Approved pending 1 remaining comment above.