Adding messages for sequences #65
Conversation
|
Could anyone review, please? We need these messages to proceed merging moveit/moveit#1893, so please review @mlautman @davetcoleman |
and address comment from review. Thanks to @v4hn!
the array contains resulting parts for every query
|
@gavanderhoorn @davetcoleman Could you please add a second review?! |
We just 'copied' move_action_capability.h and plan_service_capability.h and implemented them for sequences. Would you suggest to deprecate / remove the service capability? |
| @@ -0,0 +1,3 @@ | |||
| # List of motion planning request with a blend_radius for each. | |||
| # In the response of the planner all of these will be executable as one sequence. | |||
| MotionSequenceItem[] items | |||
There was a problem hiding this comment.
I would probably remove this abstraction and directly include lists of MotionPlanRequest and blend_radius
There was a problem hiding this comment.
Can you explain why? Having the two together in a structure enforces a 1-to-1 mapping of radii to requests.
With separate lists that is note as easily enforced.
There was a problem hiding this comment.
It's not really a 1-to-1 mapping since the last blend_radius is redundant. Also, message types usually have their own semantics with multiple use cases, but here the semantics already implies the use case of being an array item. So, while this abstraction removes a sanity check it makes the message structure a little more complicated than necessary. There are many messages that just use a convention for array lengths (i.e. Marker, RobotTrajectory..).
There was a problem hiding this comment.
It's not really a 1-to-1 mapping since the last
blend_radiusis redundant.
may be, but that's not a very strong argument to not do it.
Also, message types usually have their own semantics with multiple use cases, but here the semantics already implies the use case of being an array item.
Not sure I follow this: the semantics of MotionSequenceRequest or something else? Because the name does not necessarily imply that it's an array to me (but: not a native speaker).
So, while this abstraction removes a sanity check it makes the message structure a little more complicated than necessary.
Huh? If by abstraction you refer to the item struct, then I'm not sure how that removes a check.
It also doesn't seem very unreasonable for a MotionSequenceRequest to contain items, which themselves contain information about each item respectively. From that nested structure it's very clear that the information in those items belongs together.
While if there were just two plain arrays at the level of the request, interpretation of the message is actually less clear as the relationship between the information in the first array and the second array would only be apparent from and encoded in the code interpreting the message (ie: comes from an external and essentially unconnected source).
There are many messages that just use a convention for array lengths (i.e. Marker, RobotTrajectory..).
that may be, but this seems a bit like a tu-quoque: the fact that this is done in other messages does not necessarily make it a good pattern to apply here.
|
@rhaschke @henningkayser Are your comments addressed? Or should we incoporate further changes? |
This is a prerequisite for the Pilz Motion Planner.
This is a prerequisite for the Pilz Motion Planner.
These messages are required for motion sequences. See moveit/moveit#1893