Do not overwrite the error code with OMPL interface

[galou]

Description

In case of failure, set the error code to the one returned by the planning pipeline's solve method rather than overwriting it with PLANNING_FAILED.

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

[galou]

Should I document the changes in MIGRATION.md?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.61%. Comparing base (d962501) to head (02ec743).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2725      +/-   ##
==========================================
- Coverage   50.74%   42.61%   -8.13%     
==========================================
  Files         392      692     +300     
  Lines       32553    56331   +23778     
  Branches        0     7273    +7273     
==========================================
+ Hits        16517    24001    +7484     
- Misses      16036    32167   +16131     
- Partials        0      163     +163     

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

[sjahr]

Thanks! I think this can be simplified even further. How about

void ModelBasedPlanningContext::solve(planning_interface::MotionPlanDetailedResponse& res)
{
  res.error_code = solve(request_.allowed_planning_time, request_.num_planning_attempts);
}

My understanding is that the whole if statement is not necessary or am I missing something 🤔?

[galou]

Simplification done.

By the way, what uses solve(planning_interface::MotionPlanDetailedResponse&)?

[mergify]

This pull request is in conflict. Could you fix it @galou?

[sjahr]

Thanks!

[sjahr]

By the way, what uses solve(planning_interface::MotionPlanDetailedResponse&)?

Good question 🤔 Looks like it is by the benchmarking code. Theoretically, you could implement a custom planning pipeline that uses the detailed response. We considered removing or unifying it but that was never fleshed out. Do you have thoughts?

[sjahr]

@galou Looks like the PR needs to be rebased on the latest version of main but otherwise it is good to go 👍

[galou]

I rebased and squashed. I hope that it's ok that the conversation may lose context.

[galou]

To really see the effects of this PR, we have to wait for ompl/ompl#1147 to be merged.