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

remove grasp service support from pick_place's fillGrasp #328

Merged
merged 1 commit into from Nov 5, 2016

Conversation

Projects
None yet
2 participants
@v4hn
Member

v4hn commented Oct 31, 2016

The combination of the service support here and an optional hard-coded default grasp
is problematic because if the service is not available for any reason, but should be called,
the pick-request will end up with the hard-coded grasp.
This can create unexpected trajectories.

With MoveGroupInterface::planGraspsAndPick, a replacement for the removed functionality
is available.

This removes the dependencies on manipulation_msgs and household_objects_database_msgs.
(These were not even consistently specified in package.xml and CMakeLists.txt)

Should not be back-ported to retain current behavior.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Oct 31, 2016

Member

For some unknown reason Travis did not receive a request to test this PR. I just Googled for why to no avail. @v4hn do you mind making a change to this PR so that it might re-trigger?

Member

davetcoleman commented Oct 31, 2016

For some unknown reason Travis did not receive a request to test this PR. I just Googled for why to no avail. @v4hn do you mind making a change to this PR so that it might re-trigger?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Oct 31, 2016

Member

As you've pointed out, there's not much value here with this single default grasp. Would we rather have this fillGrasps() function call the grasping service, rather than use the default crappy grasp pose? I think this change would be fine in Kinetic.

Long term: we need a default grasping service, even if its pretty basic.

Member

davetcoleman commented Oct 31, 2016

As you've pointed out, there's not much value here with this single default grasp. Would we rather have this fillGrasps() function call the grasping service, rather than use the default crappy grasp pose? I think this change would be fine in Kinetic.

Long term: we need a default grasping service, even if its pretty basic.

Show outdated Hide outdated ...on/move_group_pick_place_capability/src/pick_place_action_capability.cpp
@@ -576,7 +472,6 @@ void move_group::MoveGroupPickPlaceAction::fillGrasps(moveit_msgs::PickupGoal &g
-std::numeric_limits<double>::max());
}
goal.possible_grasps.push_back(g);

This comment has been minimized.

@davetcoleman

davetcoleman Oct 31, 2016

Member

I'm pretty sure this section needs to be un-indented, but all my clang-format efforts have failed me since Travis didn't run

@davetcoleman

davetcoleman Oct 31, 2016

Member

I'm pretty sure this section needs to be un-indented, but all my clang-format efforts have failed me since Travis didn't run

This comment has been minimized.

@v4hn

v4hn Oct 31, 2016

Member

You were right, I just changed that to trigger the check...
For some reason my clang-format (3.9) keeps reordering my list of includes alphabetically,
so I don't use it at the moment...
If you have an idea how to change that, that would be nice :-)

@v4hn

v4hn Oct 31, 2016

Member

You were right, I just changed that to trigger the check...
For some reason my clang-format (3.9) keeps reordering my list of includes alphabetically,
so I don't use it at the moment...
If you have an idea how to change that, that would be nice :-)

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Oct 31, 2016

Member

Actually, I do think the one default grasp is enough for this interface. (We could add more if someone provides reasonable grasps).
For the more sophisticated grasp planning planGraspAndPick should be used in my opinion.

Member

v4hn commented Oct 31, 2016

Actually, I do think the one default grasp is enough for this interface. (We could add more if someone provides reasonable grasps).
For the more sophisticated grasp planning planGraspAndPick should be used in my opinion.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Oct 31, 2016

Member

Think you could write up a quick tutorial on using grasping services and its integration to moveit? You or I could also then add a table with available open source grasp planning packages to that tutorial

Member

davetcoleman commented Oct 31, 2016

Think you could write up a quick tutorial on using grasping services and its integration to moveit? You or I could also then add a table with available open source grasp planning packages to that tutorial

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Oct 31, 2016

Member

Think you could write up a quick tutorial on using grasping services and its integration to moveit?

Not sure I'll have time for that in the near future, but it sounds like a good idea.

You or I could also then add a table with available open source grasp planning packages to that tutorial

I'm pretty interested in such a list myself. I'm only aware of your moveit_simple_grasps at the moment. :)

Member

v4hn commented Oct 31, 2016

Think you could write up a quick tutorial on using grasping services and its integration to moveit?

Not sure I'll have time for that in the near future, but it sounds like a good idea.

You or I could also then add a table with available open source grasp planning packages to that tutorial

I'm pretty interested in such a list myself. I'm only aware of your moveit_simple_grasps at the moment. :)

remove grasp service support from pick_place's fillGrasp
The combination of the service support here and an optional hard-coded default grasp
is problematic because if the service is not available for any reason, but should be called,
the pick-request will end up with the hard-coded grasp.
This can create unexpected trajectories.

With MoveGroupInterface::planGraspsAndPick, a replacement for the removed functionality
is available.

This removes the dependencies on manipulation_msgs and household_objects_database_msgs.
(These were not even consistently specified in package.xml and CMakeLists.txt)

@davetcoleman davetcoleman merged commit 2956439 into ros-planning:kinetic-devel Nov 5, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 5, 2016

Member

Thanks! The tutorial need only take 10 minutes of quick notes so that we have something started for the future...

Member

davetcoleman commented Nov 5, 2016

Thanks! The tutorial need only take 10 minutes of quick notes so that we have something started for the future...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment