Replace PlanSolution with MotionPlanResponse #1542

sjahr · 2022-08-29T08:18:53Z

Description

This PR breaks the moveit_cpp API!

~~Two small additions for PlanningComponent.~~
Replace PlanSolution with MotionPlanResponse since both contain the same data.

Related PR: moveit2_tutorials#515 should be merged right after this PR to prevent the tutorials from bracking

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 · 2022-08-29T08:37:09Z

Codecov Report

❗ No coverage uploaded for pull request base (main@5213ceb). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1542   +/-   ##
=======================================
  Coverage        ?   51.08%           
=======================================
  Files           ?      381           
  Lines           ?    31727           
  Branches        ?        0           
=======================================
  Hits            ?    16203           
  Misses          ?    15524           
  Partials        ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

henningkayser

I'm fine with these changes. Keep in mind that we are planning to simplify this class a lot, as most of it would be better implemented by a state-less MotionPlanRequest builder

moveit_ros/planning/moveit_cpp/src/planning_component.cpp

henningkayser · 2022-09-19T14:51:53Z

moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h

  double planning_time_;
-  moveit_msgs::msg::MoveItErrorCodes error_code_;
+  // Result status of the query
+  moveit::core::MoveItErrorCode error_code_;


This change is not consistent with MotionPlanDetailedResponse. Whatever you change here should be reflected in the other struct as well.

I'll add it there as well 👍

@henningkayser What do you think about using inheritance

struct MotionPlanDetailedResponse : public MotionPlanResponse { // Make this a free function for both classes // void getMessage(moveit_msgs::msg::MotionPlanDetailedResponse& msg) const; std::vector<std::string> description_; std::vector<double> processing_time_; };

or aggregation here?

struct MotionPlanDetailedResponse { void getMessage(moveit_msgs::msg::MotionPlanDetailedResponse& msg) const; MotionPlanResponse response; std::vector<std::string> description_; std::vector<double> processing_time_; };

I think both would avoid changing the other if one changes in the future. Or to remove MotionPlanDetailedResponse completely and make processing time and description std::optional

well, this got stale... I think neither option is applicable, the detailed response includes a vector of trajectories.

I see, why is it a vector of trajectories anyways?

The message definition states

# Multiple solution paths are reported, each reflecting intermediate steps in the trajectory processing # The list of reported trajectories

But is that used anywhere/ how is it interpretable when the response is just a vector of unlabeled trajectories?

I'm not sure if it's being used anywhere, but my understanding is that the trajectories show you the result for each step in the process. For instance, you should be able to compare the raw planner solution with the smoothed output.

AndyZe · 2022-10-26T15:27:48Z

moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h

+  // Enable checking of query success or failure, for example if(response) ...
+  explicit operator bool() const
+  {
+    return bool(error_code_);


I don't think this is going to work. If error_code_ is negative, like int32 PLANNING_FAILED=-1, it will cast to true.

int32 SUCCESS=1 also casts to true.

Suggested change

return bool(error_code_);

return error_code_ == moveit::core::MoveItErrorCode::SUCCESS;

This is not the error code value. It's using the MoveItErrorCode type which is convertible to bool. See https://github.com/ros-planning/moveit2/blob/main/moveit_core/utils/include/moveit/utils/moveit_error_code.h#L62

I suggested to use the same in your PR. #1605 (comment)

sjahr · 2022-11-03T15:09:33Z

I'll close this PR since it is also included in #1420 and additional addressed reviews are included there

sjahr force-pushed the pr-small_moveit_cpp_improvements branch from 4ecb6c2 to b1cac10 Compare August 29, 2022 08:20

sjahr force-pushed the pr-small_moveit_cpp_improvements branch from 666411d to 3e54058 Compare August 31, 2022 11:46

sjahr requested a review from henningkayser September 5, 2022 19:03

henningkayser reviewed Sep 6, 2022

View reviewed changes

moveit_ros/planning/moveit_cpp/src/planning_component.cpp Outdated Show resolved Hide resolved

sjahr mentioned this pull request Sep 8, 2022

Benchmark parallel planning pipelines #1539

Merged

6 tasks

sjahr force-pushed the pr-small_moveit_cpp_improvements branch from 3e54058 to 32fad11 Compare September 18, 2022 19:53

sjahr changed the title ~~Add setTrajectoryConstraints() to PlanningComponent and add planning time to PlanningSolution~~ Replace PlanSolution with MotionPlanResponse Sep 19, 2022

sjahr requested a review from henningkayser September 19, 2022 07:53

sjahr mentioned this pull request Sep 19, 2022

Update moveit_cpp tutorial moveit/moveit2_tutorials#515

Merged

2 tasks

henningkayser reviewed Sep 19, 2022

View reviewed changes

sjahr requested a review from henningkayser September 29, 2022 19:49

AndyZe reviewed Oct 26, 2022

View reviewed changes

henningkayser mentioned this pull request Oct 28, 2022

Python Bindings - moveit_py #1546

Merged

sjahr added 3 commits November 1, 2022 11:15

Add setTrajectoryConstraints() to PlanningComponent

c3cbbda

Add planning time to PlanningComponent::PlanSolution

8677167

Replace PlanSolution with MotionPlanResponse

d351118

tylerjw force-pushed the pr-small_moveit_cpp_improvements branch from 32fad11 to d351118 Compare November 1, 2022 17:15

sjahr closed this Nov 3, 2022

sjahr mentioned this pull request Nov 10, 2022

MotionPlanResponse and MotionPlanDetailedResponse are redundant #1708

Open

rhaschke mentioned this pull request Jan 10, 2023

Update moveit_cpp_tutorial because of changes from moveit2#1542 (#515) moveit/moveit2_tutorials#569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace PlanSolution with MotionPlanResponse #1542

Replace PlanSolution with MotionPlanResponse #1542

sjahr commented Aug 29, 2022 •

edited

Loading

codecov bot commented Aug 29, 2022 •

edited

Loading

henningkayser left a comment

henningkayser Sep 19, 2022

sjahr Sep 28, 2022

sjahr Sep 28, 2022 •

edited

Loading

henningkayser Oct 18, 2022

sjahr Oct 25, 2022 •

edited

Loading

sjahr Oct 25, 2022

henningkayser Oct 28, 2022

AndyZe Oct 26, 2022

AndyZe Oct 26, 2022

henningkayser Oct 28, 2022

henningkayser Oct 28, 2022

sjahr commented Nov 3, 2022

	return bool(error_code_);
	return error_code_ == moveit::core::MoveItErrorCode::SUCCESS;

Replace PlanSolution with MotionPlanResponse #1542

Replace PlanSolution with MotionPlanResponse #1542

Conversation

sjahr commented Aug 29, 2022 • edited Loading

Description

Checklist

codecov bot commented Aug 29, 2022 • edited Loading

Codecov Report

henningkayser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjahr Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjahr Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjahr commented Nov 3, 2022

sjahr commented Aug 29, 2022 •

edited

Loading

codecov bot commented Aug 29, 2022 •

edited

Loading

sjahr Sep 28, 2022 •

edited

Loading

sjahr Oct 25, 2022 •

edited

Loading