documenting instructions for the pick-and-place task #238

FabianSchuetze · 2021-02-23T07:32:20Z

This PR attempts to embellish the pick-place demo by adding comments to several sections of the task construction.

I have been trying to understand the pick-place demo in the last few days and had benefited from a bit more verbose description. In particular, I did not comprehend how the grasp_frame_transform, specified in the panda config file, influence the outcome. I asked the same question on ros anwers but failed to garner an answer. @felixvd encouraged me via Discord to try to understand it myself and submit a PR with code comments.

I do not know if my comments are helpful or whether the PR has the appropriate structure. If there is anything you think I can improve, I'd happily do that.

codecov · 2021-02-23T07:41:53Z

Codecov Report

Merging #238 (c9f3c9f) into master (a6fa452) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #238   +/-   ##
=======================================
  Coverage   33.86%   33.86%           
=======================================
  Files          93       93           
  Lines        5491     5491           
=======================================
  Hits         1859     1859           
  Misses       3632     3632

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6fa452...aeb8aa4. Read the comment docs.

felixvd

Sorry for the late review, and thanks for the PR. I think this demo deserves detailed comments, so I hope this doesn't come across the wrong way, but I would actually appreciate if you could make this text shorter and more concise. The comments are verbose in a way that often makes them confusing instead of clarifying.

Consider this sentence: "The GenerateGraspPose circles in AngleDelta increments along the object creating one target pose in each instance."

Is "instance" a class instance, or a stage instance, or just a word that could be eliminated? When you read "The GenerateGraspPose circles", is "circles" a verb or noun? It could be either, and the reader can't be sure until almost the end of the sentence. The reference "the GenerateGraspPose" without the word "stage" reads strangely, almost like writing "the cancer" instead of "the cancer patient".

Consider this alternative: "The GenerateGraspPose stage adds grasp pose candidates in angle increments around the z-axis."

I've had this review half-done in my tabs for a while, so I'd rather submit this potentially unhelpful comment before this gets stale. To reiterate, this doesn't need to win a literary nobel prize and it already makes the demo better, but I think it's unnecessarily confusing in many parts. Would be glad if you could try improving on some of those points and update the PR.

felixvd · 2021-02-24T02:31:58Z

demo/src/pick_place_task.cpp

+// "high level" movements: Checking if the object to be picked is already attached to the robot arm, opening the robot's
+// hand, moving the robot to the pre-grasp position, grasping the object, moving the robot to a pre-place position,
+// placing the object, and finally, moving the robot to a standstill position. These six sequences are listed on the
+// least indented column of the "Motion Planning Tasks" widget in Rviz. These sequences are not planned or executed but


"least indented" --> "top-level", although since technically that's not true either I'd skip it altogether. "These sequences are shown in the widget..."

felixvd · 2021-02-24T02:34:56Z

demo/src/pick_place_task.cpp

+// hand, moving the robot to the pre-grasp position, grasping the object, moving the robot to a pre-place position,
+// placing the object, and finally, moving the robot to a standstill position. These six sequences are listed on the
+// least indented column of the "Motion Planning Tasks" widget in Rviz. These sequences are not planned or executed but
+// defined as part of moveit's task constructor object below. The stages are be described in more detail below.


I'd call it a Task object.

felixvd · 2021-02-24T02:51:08Z

demo/src/pick_place_task.cpp

@@ -117,6 +126,7 @@ void PickPlaceTask::init() {
 	 *               Current State                      *
 	 *                                                  *
 	 ***************************************************/
+    // Before beginning with the operation, one needs to verify if the object is not already attached to the robot. 


I would try to keep it shorter, in general. E.g. "We verify that the object is not already attached to the robot."

Does every task need to start with a state? Would help to say something about that here instead (otherwise there are 2 (3) comments within 10 lines about the verification).

felixvd · 2021-02-24T02:51:13Z

demo/src/pick_place_task.cpp

@@ -153,6 +163,9 @@ void PickPlaceTask::init() {
 	 *               Move to Pick                       *
 	 *                                                  *
 	 ***************************************************/
+    // This is the second major phase of picking the object. This phase moves to the start of the picking location. The


Would keep it shorter here, too. E.g. "Moves to a pre-grasp pose from where the gripper approaches the object. Generated by a stage::Connect object which does not plan a movement, but connects two stages that do."

Also, remove the comment in the next line

felixvd · 2021-02-24T02:55:08Z

demo/src/pick_place_task.cpp

-			auto stage = std::make_unique<stages::MoveRelative>("approach object", cartesian_planner);
+            // This movement approaches the object and stops right before it through a MoveRelative stage. The stage
+            // resembles the Connect because it does not define an end or starting point but move across space as defined
+            // as a geometryMsgs. In contrast to the Connect stage, however, the planner does not have any flexibility in


geometry_msgs::Pose?

felixvd · 2021-02-24T02:55:42Z

demo/src/pick_place_task.cpp

+            // This movement approaches the object and stops right before it through a MoveRelative stage. The stage
+            // resembles the Connect because it does not define an end or starting point but move across space as defined
+            // as a geometryMsgs. In contrast to the Connect stage, however, the planner does not have any flexibility in
+            // the cartesian structure of the movement as this is precisely defined.


I'm not sure what you mean by "cartesian structure of the movement"

felixvd · 2021-02-24T02:56:30Z

demo/src/pick_place_task.cpp

@@ -176,7 +193,11 @@ void PickPlaceTask::init() {
  ---- *               Approach Object                    *
 		 ***************************************************/
 		{
-			auto stage = std::make_unique<stages::MoveRelative>("approach object", cartesian_planner);
+            // This movement approaches the object and stops right before it through a MoveRelative stage. The stage


Misses a comment that this stage is inserted into the grasp/PickObject SerialContainer.

felixvd · 2021-03-01T07:40:47Z

demo/src/pick_place_task.cpp

-			stage->setMonitoredStage(current_state_ptr);  // Hook into current state
-
-			// Compute IK
+            // The object grasp is defined by combining two stages, a GenerateGraspPose and a ComputeIK pose. The


"latches onto the object" implies a connection that I don't really see. "connects" seems to make more sense.

"The generateGraspPose" should be "The generateGraspPose stage" or "...container". The phrase "a ComputeIK pose" seems to be used to mean "a ComputeIKpose stage". This is confusing.

GenerateGraspPose is written both with and without capital first letter.

FabianSchuetze · 2021-03-01T14:29:43Z

Thank you so much, @felixvd for your detailed and very thoughtful comments to the PR. Your suggestions help me to improve the PR an I will update the it accordingly!

FabianSchuetze · 2021-03-03T09:27:09Z

Thank you again for your detailed and kind comments! I benefited a lot from them and hope the changed PR reflects this. I shortened the explanations and believe the text is more precise and succinct. I am curious to hear what you think of it and appreciate any feedback. Furthermore, should we maybe integrate these comments in a full tutorial for the moveit website as the current MTC tutorial is a bit short?

felixvd

Thanks, it's a lot better. I made some suggestions, but I'm good with this overall.

felixvd · 2021-03-03T10:04:40Z

demo/src/pick_place_task.cpp

 void PickPlaceTask::init() {
 	ROS_INFO_NAMED(LOGNAME, "Initializing task pipeline");
 	const std::string object = object_name_;

 	// Reset ROS introspection before constructing the new object
 	// TODO(henningkayser): verify this is a bug, fix if possible
+	// Movements are collected inside a Task object.


A bit confusing with the previous two lines. Make this a line-end comment (put it at the end of the next line with two separating spaces).

felixvd · 2021-03-03T10:04:53Z

demo/src/pick_place_task.cpp

@@ -82,20 +82,22 @@ void PickPlaceTask::loadParameters() {
 	rosparam_shortcuts::shutdownIfError(LOGNAME, errors);
 }

+// The init function declares all movements for the pick-and-place task. These movements are described below.


Suggested change

// The init function declares all movements for the pick-and-place task. These movements are described below.

// The init function declares all movements for the pick-and-place task.

felixvd · 2021-03-03T10:06:07Z

demo/src/pick_place_task.cpp

 	task_.reset();
 	task_.reset(new moveit::task_constructor::Task());

 	Task& t = *task_;
 	t.stages()->setName(task_name_);
 	t.loadRobotModel();

-	// Sampling planner
+	// Different planners plan robot movement: a cartesian , pipeline, or joint interpolation planner.


Suggested change

// Different planners plan robot movement: a cartesian , pipeline, or joint interpolation planner.

// Different planners are available to plan robot motions, e.g. cartesian, pipeline, or joint interpolation.

Since you can choose any planner. These are just examples.

felixvd · 2021-03-03T10:09:21Z

demo/src/pick_place_task.cpp

 			auto stage = std::make_unique<stages::GenerateGraspPose>("generate grasp pose");
 			stage->properties().configureInitFrom(Stage::PARENT);
 			stage->properties().set("marker_ns", "grasp_pose");
 			stage->setPreGraspPose(hand_open_pose_);
 			stage->setObject(object);
-			stage->setAngleDelta(M_PI / 12);
+			stage->setAngleDelta(M_PI / 4);


Why did this value change?

If it was up to me, I would change this to M_TAU/24 as per this PR of mine, but let's not open that discussion.

felixvd · 2021-03-03T10:11:01Z

demo/src/pick_place_task.cpp

 			stage->setMonitoredStage(current_state_ptr);  // Hook into current state

-			// Compute IK
+			// Compute joint parameters for a target frame of the end effector. Each target frame equals a grasp pose
+			// from the previous stage times the inverse of the user-defined grasp_frame_tansform.


What is the grasp_frame_transform? Why times the inverse?

felixvd · 2021-03-03T10:13:39Z

demo/src/pick_place_task.cpp

@@ -217,6 +225,7 @@ void PickPlaceTask::init() {
  ---- *               Allow Collision (hand object)   *
 		 ***************************************************/
 		{
+			// Modify the planning scene (does not alter the robot's position) to permit picking up the object.


Suggested change

// Modify the planning scene (does not alter the robot's position) to permit picking up the object.

// Modify the planning scene (does not alter the robot's position) to allow collision between hand and object. Otherwise, the robot would not be able to move after picking the object.

felixvd · 2021-03-03T10:15:28Z

demo/src/pick_place_task.cpp

+		// Attaching the object to the hand and lifting the object while guaranteeing that it does not touch the
+		// ground happens in the next four stages.


Suggested change

// Attaching the object to the hand and lifting the object while guaranteeing that it does not touch the

// ground happens in the next four stages.

// The next four stages attach the object to the hand and lift it while ensuring that it does not touch the ground.

felixvd · 2021-03-03T10:21:04Z

demo/src/pick_place_task.cpp

@@ -153,7 +156,8 @@ void PickPlaceTask::init() {
 	 *               Move to Pick                       *
 	 *                                                  *
 	 ***************************************************/
-	{  // Move-to pre-grasp
+	// Move to a pre-grasp pose. The Connect stage does not declare any movements but connects two stages that do.


Suggested change

// Move to a pre-grasp pose. The Connect stage does not declare any movements but connects two stages that do.

// Move to the pre-grasp pose. The Connect stage connects two other stages with a motion.

Not perfectly happy with this, but it sounded too much like there's no robot motion going on in this stage

felixvd · 2021-03-03T10:22:55Z

demo/src/pick_place_task.cpp

@@ -290,6 +301,7 @@ void PickPlaceTask::init() {
 	 *                                                    *
 	 *****************************************************/
 	{
+		// Fourth, define the `move to place` (as the `move to pick`) stage as a Connect object.


Suggested change

// Fourth, define the `move to place` (as the `move to pick`) stage as a Connect object.

// Fourth, define the `move to place` stage as a Connect stage connecting the end of the pick to the start of the place phase (like the `move to pick` stage further up)

felixvd · 2021-03-03T10:30:11Z

Oh, and yes, an actual MTC tutorial page based on this example is certainly welcome. AFAIK there's no reason that it doesn't exist yet, except that no one has the time to write it. It would probably fit best on the current page.

FabianSchuetze · 2021-03-03T18:14:45Z

Thank you very much indeed, Felix, for your comments and your help. Your assistance made the PR much better and working on it very enjoyable. I will try to find a new moveit issue to work on (a good first issue) and hope to contribute!

cpetersmeier · 2021-03-09T19:27:43Z

Hello to all, I am working on the moveit_task_constructor tutorial. I enjoy the additions that this PR contributes to the demo code and would love to integrate them into the tutorial!

I think moving / duplicating the demo into the moveit_tutorials package was already touched on here. Would you suggest to pick the text / code bits manually from the demo files in this repository?

FabianSchuetze · 2021-03-12T18:11:24Z

Hi @cpetersmeier - it is great to hear you are working on a tutorial for the MTC! I do not understand what you mean by saying:

Would you suggest to pick the text / code bits manually from the demo files in this repository?

However, if I were you, I would try to write a tutorial and select all the parts that you need for it as you see it fit. Please ping me if you have a PR ready - I am excited to read it!

cpetersmeier · 2021-05-05T11:37:12Z

Dear @FabianSchuetze , @felixvd ,

I have created a first draft of an improved Moveit Task Constructor tutorial here. I would love some feedback from you that I could incorporate in the text!

codecov-commenter · 2024-07-06T15:20:09Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 45.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 57.31%. Comparing base (6b0f2c8) to head (c83c99e).
Report is 26 commits behind head on master.

Files	Patch %	Lines
demo/src/pick_place_task.cpp	45.00%	11 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   58.82%   57.31%   -1.50%     
==========================================
  Files          91      131      +40     
  Lines        8623    10684    +2061     
  Branches        0      954     +954     
==========================================
+ Hits         5072     6123    +1051     
- Misses       3551     4514     +963     
- Partials        0       47      +47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

documenting instructions for the pick place task

2b18369

apply clang-format

162f66c

felixvd reviewed Mar 1, 2021

View reviewed changes

FabianSchuetze added 2 commits March 1, 2021 18:27

Merge branch 'master' into illustrate_pick_place_task

f1d0f8b

adjust PR based on comments

aeb8aa4

felixvd approved these changes Mar 3, 2021

View reviewed changes

Merge branch 'master' into illustrate_pick_place_task

2086b04

rhaschke added 2 commits July 6, 2024 17:47

Cleanup comments

b093270

minor change

c83c99e

rhaschke merged commit 702710d into moveit:master Jul 6, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

documenting instructions for the pick-and-place task #238

documenting instructions for the pick-and-place task #238

FabianSchuetze commented Feb 23, 2021

codecov bot commented Feb 23, 2021 •

edited

Loading

felixvd left a comment

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Feb 24, 2021

felixvd Mar 1, 2021

FabianSchuetze commented Mar 1, 2021

FabianSchuetze commented Mar 3, 2021

felixvd left a comment

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd Mar 3, 2021

felixvd commented Mar 3, 2021

FabianSchuetze commented Mar 3, 2021

cpetersmeier commented Mar 9, 2021

FabianSchuetze commented Mar 12, 2021

cpetersmeier commented May 5, 2021

codecov-commenter commented Jul 6, 2024 •

edited

Loading

	// The init function declares all movements for the pick-and-place task. These movements are described below.
	// The init function declares all movements for the pick-and-place task.

	// Different planners plan robot movement: a cartesian , pipeline, or joint interpolation planner.
	// Different planners are available to plan robot motions, e.g. cartesian, pipeline, or joint interpolation.

	// Modify the planning scene (does not alter the robot's position) to permit picking up the object.
	// Modify the planning scene (does not alter the robot's position) to allow collision between hand and object. Otherwise, the robot would not be able to move after picking the object.

		// Attaching the object to the hand and lifting the object while guaranteeing that it does not touch the
		// ground happens in the next four stages.

	// Attaching the object to the hand and lifting the object while guaranteeing that it does not touch the
	// ground happens in the next four stages.
	// The next four stages attach the object to the hand and lift it while ensuring that it does not touch the ground.

	// Move to a pre-grasp pose. The Connect stage does not declare any movements but connects two stages that do.
	// Move to the pre-grasp pose. The Connect stage connects two other stages with a motion.

	// Fourth, define the `move to place` (as the `move to pick`) stage as a Connect object.
	// Fourth, define the `move to place` stage as a Connect stage connecting the end of the pick to the start of the place phase (like the `move to pick` stage further up)

documenting instructions for the pick-and-place task #238

documenting instructions for the pick-and-place task #238

Conversation

FabianSchuetze commented Feb 23, 2021

codecov bot commented Feb 23, 2021 • edited Loading

Codecov Report

felixvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabianSchuetze commented Mar 1, 2021

FabianSchuetze commented Mar 3, 2021

felixvd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixvd commented Mar 3, 2021

FabianSchuetze commented Mar 3, 2021

cpetersmeier commented Mar 9, 2021

FabianSchuetze commented Mar 12, 2021

cpetersmeier commented May 5, 2021

codecov-commenter commented Jul 6, 2024 • edited Loading

Codecov Report

codecov bot commented Feb 23, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2024 •

edited

Loading