Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Hybrid planning action definitions #114

Closed
wants to merge 3 commits into from

Conversation

sjahr
Copy link

@sjahr sjahr commented May 20, 2021

This PR adds the action interfaces for MoveIt 2's hybrid planning feature (more information about it can be found in these moveit2 issues: #300 & #433)

@sjahr sjahr changed the title Hybrid planning action definitions Add Hybrid planning action definitions May 20, 2021
@sjahr sjahr changed the base branch from master to ros2 May 20, 2021 09:32
@henningkayser henningkayser self-requested a review June 1, 2021 15:21
Copy link
Contributor

@AdamPettinger AdamPettinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These actions make sense for the hybrid planner, and this PR should probably be merged before moveit/moveit2#488

Just left one small nitpicking comment to consider making the action naming consistent

@@ -108,6 +108,9 @@ set(action_files
"action/MoveGroupSequence.action"
"action/Pickup.action"
"action/Place.action"
"action/LocalPlanner.action"
"action/GlobalPlanner.action"
"action/HybridPlanning.action"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a tiny comment, this is *Planner, *Planner, *Planning, maybe the last should be action/HybridPlanner?

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @AdamPettinger, the names could be a little bit more consistent. I'm wondering if there is a better name than action/HybridPlanning which indicates that the motion is executed at the same time...

action/HybridPlanning.action Show resolved Hide resolved
action/LocalPlanner.action Outdated Show resolved Hide resolved
action/LocalPlanner.action Outdated Show resolved Hide resolved
action/LocalPlanner.action Show resolved Hide resolved
action/GlobalPlanner.action Outdated Show resolved Hide resolved
@sjahr sjahr force-pushed the hybrid_planning branch 2 times, most recently from 77c53fd to b2a4f7a Compare August 29, 2021 09:54
@sjahr
Copy link
Author

sjahr commented Aug 29, 2021

The latest commit breaks the API in favor of the new plugin interface for the global planner (moveit2 #585)

@@ -0,0 +1,9 @@
# Motion planning request to the global planner
string planning_group
moveit_msgs/MotionSequenceRequest desired_motion_sequence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really convinced of this name. Simply request would be fine imo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calling this motion_sequence (As it is in the HybridPlanner.action) is actually clearer when used in code as it is a sub-message of the action goal.

@tylerjw tylerjw mentioned this pull request Oct 29, 2021
@tylerjw
Copy link
Member

tylerjw commented Oct 29, 2021

Closing in favor of #135 where I will continue this work.

@tylerjw tylerjw closed this Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants