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

Added plan_only flags to pick and place in Kinetic #862

Merged
merged 8 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@DavidWatkins
Copy link
Contributor

commented Apr 26, 2018

Description

The pick/place functions in moveit_commander did not have a way to only plan the action. I have added optional parameters onto the pick/place calls in moveit_commander which is defaulted to false to ensure no changes in existing code will be required.

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)
@davetcoleman
Copy link
Member

left a comment

Overall this seems like an improvement, thanks!

@@ -68,7 +68,7 @@ void demoPick(moveit::planning_interface::MoveGroupInterface& group)

grasps.push_back(g);
}
group.pick("bubu", grasps);
group.pick("bubu", grasps, false);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

for readability can you make this a const?

const bool plan_only = false;
(..., plan_only)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

I had actually meant to just change this line (and other similar lines) to not have a hidden false value but instead make it a const right before the call:

const bool plan_only = false;
group.pick("bubu", grasps, plan_only);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

But the approach you used of having default values in the function headers i actually like better, except please remove the const

@@ -795,7 +797,7 @@ class MoveGroupInterface::MoveGroupInterfaceImpl
return MoveItErrorCode(moveit_msgs::MoveItErrorCodes::FAILURE);
}

return pick(object.id, response.grasps);
return pick(object.id, response.grasps, false);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

same here for const

@@ -343,15 +343,15 @@ void MotionPlanningFrame::pickObject()
{
move_group_->setSupportSurfaceName(support_surface_name_);
}
if (move_group_->pick(pick_object_name_[group_name]))
if (move_group_->pick(pick_object_name_[group_name], false))

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

same here

@DavidWatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

I added the change to the arguments so that they are constant. Let me know if that doesn't line up with what you meant.

@davetcoleman
Copy link
Member

left a comment

Don't forget to run clang-format again

@@ -754,15 +754,15 @@ class MoveGroupInterface
/** \brief Pick up an object
This applies a number of hard-coded default grasps */
MoveItErrorCode pick(const std::string& object);
MoveItErrorCode pick(const std::string& object, const bool plan_only=false);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman May 14, 2018

Member

please remove the const here and throughout

@DavidWatkins

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Thanks for the reminder. Updated it again.

@mcevoyandy

This comment has been minimized.

Copy link

commented Oct 12, 2018

@davetcoleman looks like @DavidWatkins addressed all your feedback. Anything else stopping this PR from moving forward?

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

@mcevoyandy this PR needs a second review, do you +1 it?

@v4hn
Copy link
Member

left a comment

Some nitpick, otherwise this looks reasonable.

@DavidWatkins

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

I've addressed the new concerns, any additional thoughts?

@v4hn

v4hn approved these changes Oct 23, 2018

@v4hn v4hn merged commit b6beb00 into ros-planning:kinetic-devel Oct 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

v4hn added a commit that referenced this pull request Oct 23, 2018

Added plan_only flags to pick and place (#862)
Added optional plan_only flags to pick and place

Added plan_only flag to planGraspsAndPick
@v4hn

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Thank you for the swift cleanup!

Merged and cherry-picked to melodic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.