Discuss some force-pushed changes #545
Conversation
| @@ -63,11 +63,11 @@ class CartesianPath : public PlannerInterface | |||
|
|
|||
| void init(const moveit::core::RobotModelConstPtr& robot_model) override; | |||
|
|
|||
| Result plan(const planning_scene::PlanningSceneConstPtr& from, const planning_scene::PlanningSceneConstPtr& to, | |||
| MoveItErrorCode plan(const planning_scene::PlanningSceneConstPtr& from, const planning_scene::PlanningSceneConstPtr& to, | |||
There was a problem hiding this comment.
Why did you decide to introduce Result instead of using error codes? Aren't we loosing information by reducing the error code to a boolean?
There was a problem hiding this comment.
Originally, #525 introduced tl::expected<bool, std::string> to return both, a boolean result and an error message. Later, this was changed to MoveItErrorCode, which - in ROS2 - supports the error message and an error source. As this extended MoveItErrorCode is not available in ROS1 and the purpose was just to return an additional string message, I introduced an explicit data type that is uniformly usable in ROS1 and ROS2.
If at all, the source information is lost (it was never fed into the error code).
There was a problem hiding this comment.
I don't consider reverting to MoveItErrorCode and I don't see something missing so far.
There was a problem hiding this comment.
If at all, the source information is lost (it was never fed into the error code).
Good point that should be done, otherwise that field is pointless.
I see, have you considered enhancing the error code with a string message in moveit1? The main motivation here was to improve the debug-ability of moveit in general. The source field was introduced to infer the planning pipeline that created the error message when using parallel planning. Without that, I think it is just possible to tell that one of the planners failed with the given error message. Not which one. Alternatively, the string could be enhanced with that information.
There was a problem hiding this comment.
Yes, I shortly considered augmenting MoveItErrorCode in MoveIt1 as well. However, I decided against, because the error message is redundant as we already have a fixed mapping from the error code to a human-readable message (MoveItErrorCode::toString()). Further, the source member was not filled regularly - only in planning adapters. As far as I can see, the planning pipeline name is not yet stored in that field.
Note, that there are two MoveItErrorCode header files (I think one should be removed):
moveit_core/utils/include/moveit/utils/moveit_error_code.h(regularly used)moveit_ros/hybrid_planning/hybrid_planning_manager/hybrid_planning_manager_component/include/moveit/hybrid_planning_manager/moveit_error_code_interface.h(used only once)
core/include/moveit/task_constructor/solvers/pipeline_planner.h
Outdated
Show resolved
Hide resolved
| @@ -78,10 +78,10 @@ PlannerInterface::Result JointInterpolationPlanner::plan(const planning_scene::P | |||
| // add first point | |||
| result->addSuffixWayPoint(from->getCurrentState(), 0.0); | |||
| if (from->isStateColliding(from_state, jmg->getName())) | |||
| return { false, "Start state is in collision!" }; | |||
| return MoveItErrorCode(MoveItErrorCodes::START_STATE_IN_COLLISION, "Start state is in collision!"); | |||
There was a problem hiding this comment.
These returned in ros-old just FAILURE, I changed them so that the error code actually provides value
There was a problem hiding this comment.
Note, that the error code is never actually considered in code - we just check for success.
It might be useful for inspection during debugging though.
There was a problem hiding this comment.
Yes, we did that in some custom applications. Given that MoveIt generates this error code, it would be great not to loose this information in the MTC solution.
There was a problem hiding this comment.
How did you access that code in custom applications? It is never exposed from any stage!?
There was a problem hiding this comment.
You're right, I think was wrong about that, sorry
|
Thanks for your questions. I hope I answered all of them. Unfortunately, the diff link I provided originally, doesn't allow for comments. I just noticed that now. |
|
@rhaschke Thanks for your detailed explanations. That is very helpful and going through you changes was insightful too!
I can take a look into it. With Michael's change ported to moveit2 the MTC pick and place demo worked for me without needing to adjust the threshold but maybe I am missing something here. |
The code using this threshold was only recently added in the master branch. So, if pick+place worked before on the old ros2 branch, this doesn't come at a surprise for me. |
sjahr
force-pushed
the
pr-revert_some_cleanup_changes
branch
from
48d97b1 to
e11f89c
Compare
|
Dear @sjahr, can we close this? I think all changes were discussed. |
@rhaschke I have some questions around the changes you created on the ros2 branch. I'd like to use this PR to understand them better and potentially make some updates.