subframes tutorial by rhaschke · Pull Request #326 · moveit/moveit_tutorials

rhaschke · 2019-05-19T14:58:04Z

I cleaned @felixvd's initial tutorial for the new subframes feature a little bit to prepare it for inclusion into the main repo. Felix, please have a look.
This belongs to moveit/moveit#1439.

Also some minor changes

rhaschke · 2019-06-19T09:52:49Z

Ping @felixvd, can you handle the remaining Travis issues and finalizes this?

felixvd

I forgot to submit this comment when I took a look a few weeks ago apparently.

felixvd · 2019-05-19T20:27:11Z

doc/subframes/src/subframes_tutorial.cpp

  planning_scene_interface.applyAttachedCollisionObject(att_coll_object);

-  // Wait a bit for ROS things to initialize
-  ros::WallDuration(1.0).sleep();


Sure this is unnecessary? I took it from another tutorial.

Yes, it was probably needed before when addAttachedCollisionObject was used.
This is probably a leftover from before the apply* functions existed.

felixvd · 2019-06-20T08:25:21Z

Sorry for the long wait. I couldn't focus on this because I have a deadline for a few papers to meet at the end of the month, but I was planning on making this a proper tutorial instead of a stub before merging. First half of July I hope!

v4hn · 2019-06-20T16:26:37Z

doc/subframes/src/subframes_tutorial.cpp

+  return false;
+}
+
+void addCollisionObjects(moveit::planning_interface::PlanningSceneInterface& planning_scene_interface)


Could you maybe rename this function to setupCollisionObjects or prepareCollisionObjects instead to avoid name collision with the PSI functions?

Personally I would prefer setting up a vector of collision objects here instead of passing the PSI, but that's personal preference.

I renamed it but kept the PSI as an argument, because the point is to keep the collision object definition inside the function.

v4hn · 2019-06-20T16:27:26Z

doc/subframes/src/subframes_tutorial.cpp

+  planning_scene_interface.applyCollisionObject(co);
+  co = cylinder;
+  co.operation = moveit_msgs::CollisionObject::ADD;
+  planning_scene_interface.applyCollisionObject(co);


Please use applyCollisionObjects here to perform the updates in one ROS service call.

davetcoleman · 2019-07-03T00:08:06Z

doc/subframes/src/subframes_tutorial.cpp

+/*********************************************************************
+* Software License Agreement (BSD License)
+*
+*  Copyright (c) 2019, who?


Add your name

davetcoleman · 2019-07-03T00:08:10Z

doc/subframes/src/subframes_tutorial.cpp

+*     copyright notice, this list of conditions and the following
+*     disclaimer in the documentation and/or other materials provided
+*     with the distribution.
+*   * Neither the name of Willow Garage nor the names of its


Add your name

davetcoleman · 2019-07-03T00:09:01Z

doc/subframes/src/subframes_tutorial.cpp

+  ROS_INFO_STREAM("Attaching cylinder to robot.");
+  planning_scene_interface.applyAttachedCollisionObject(att_coll_object);
+
+  //---


Especially since this is a tutorial, please add a lot more inline documentation

davetcoleman · 2019-07-03T00:09:18Z

doc/subframes/src/subframes_tutorial.cpp

+  planning_scene_interface.applyAttachedCollisionObject(att_coll_object);
+
+  //---
+  int c;


please use full-word variable names

davetcoleman · 2019-07-03T00:09:25Z

doc/subframes/src/subframes_tutorial.cpp

+  //---
+  int c;
+  tf2::Quaternion orientation, orientation2, orientation3;
+  geometry_msgs::PoseStamped ps, ps_0;


better variable names

davetcoleman · 2019-07-03T00:10:05Z

doc/subframes/src/subframes_tutorial.cpp

+             "\n11 to move to a certain point in space with end effector (for visualizing the subframe pose)"
+             "\n12 to move to a certain point in space with cylinder/tip ");
+    std::cin >> c;
+    if (c == 0)


not a big deal, but a switch statement would be best, right?

davetcoleman · 2019-07-03T00:11:28Z

doc/subframes/subframes_tutorial.rst

+Subframes Tutorial
+============================
+
+This is just to test the subframes in a easy to run format.


I'd be fine merging this tutorial as-is now (after addressing feedback), and improving it in the future when you have time. Please add note here that its a stub, though.

Thanks for all the comments in a very rough file that wasn't meant to be in a PR yet :)
I would prefer to finish this up properly before merging it. Otherwise it won't really be displayable on the website at all. Give me a week or two until things cool down and I'll write up something proper.

From many experiences, the statement "I'll get to it when things cool down" rarely happens, so even this would be better than nothing ;-)

felixvd · 2019-07-11T22:54:02Z

True! I already have a newer local version though and a talk at Roscon JP about this, so I'll push an update soon.

…

On Fri, Jul 12, 2019, 06:31 Dave Coleman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/subframes/subframes_tutorial.rst <#326 (comment)> : > @@ -0,0 +1,29 @@ +Subframes Tutorial +============================ + +This is just to test the subframes in a easy to run format. From many experiences, the statement "I'll get to it when things cool down" rarely happens, so even this would be better than nothing ;-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#326>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABCTLOLJHJ3L6NLFJOQIZX3P66RBNANCNFSM4HN4VE6Q> .

davetcoleman · 2019-07-30T00:28:03Z

friendly ping @felixvd

felixvd · 2019-07-31T13:33:22Z

I addressed all the changes except the switch statement in this branch in my fork, but I don't know how to add it here. With these changes it should be an actual tutorial. Can you cherry-pick the two commits @rhaschke ? Otherwise I can do a PR from the other branch as well.

I added a ~2 MB gif, but I can also send the video file for uploading to Youtube. You decide.

davetcoleman · 2019-08-01T15:38:50Z

Closed by #365

felixvd and others added 6 commits May 3, 2019 08:02

Add named_frames test package

3de9396

Change naming to "subframes"

fcbad6b

Also some minor changes

Update separator to slash, clean up

000d614

avoid extra variables

e80b23c

use a single try block

b5a5663

remove commented code

c257ada

rhaschke mentioned this pull request Jun 2, 2019

Add named frames to CollisionObjects (v2) moveit/moveit#1439

Merged

11 tasks

felixvd reviewed Jun 20, 2019

View reviewed changes

v4hn requested changes Jun 20, 2019

View reviewed changes

davetcoleman reviewed Jul 3, 2019

View reviewed changes

felixvd mentioned this pull request Aug 1, 2019

Add subframe tutorial #365

Merged

2 tasks

davetcoleman closed this Aug 1, 2019

rhaschke deleted the add-named-frames branch August 28, 2019 07:08

Conversation

rhaschke commented May 19, 2019

Uh oh!

rhaschke commented Jun 19, 2019

Uh oh!

felixvd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixvd commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixvd commented Jul 11, 2019 via email

Uh oh!

davetcoleman commented Jul 30, 2019

Uh oh!

felixvd commented Jul 31, 2019

Uh oh!

davetcoleman commented Aug 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants