Revamp move_group_interface_tutorial #45

davetcoleman · 2016-11-01T02:02:07Z

Major overhaul of tutorial - currently it does not work in Kinetic because of broken PR2 dependencies, planning time reliability issues, and old instructions

Reorder tutorial to have demo first, then code explanation
Add MoveItVisualTools to enable much better visuals (see video)
Add step-by-step pause functionality to allow user interaction of demo
Removes long and boring sleep calls
Added video

A PDF of the new tutorial is here: Move Group Interface Tutorial — moveit_tutorials Indigo documentation.pdf

A video is here: https://www.youtube.com/watch?v=4FSmZRQh37Q

@mcevoyandy I addressed almost all your feedback after going through the tutorial, please review PDF above

Fixes #34

Addresses results of benchmarking in moveit/moveit#337

Reorder tutorial to have demo first, then code explanation Add MoveItVisualTools to enable much better visuals Add step-by-step pause functionality to allow user interaction of demo Added video

v4hn

nice video!

Great improvements.

v4hn · 2016-11-01T14:10:36Z

doc/pr2_tutorials/planning/src/doc/move_group_interface_tutorial.rst


- roslaunch moveit_tutorials move_group_interface_tutorial.launch
+  git clone https://github.com/PR2/pr2_common.git -b kinetic-devel


I believe the indigo-devel branch is actually the better choice here for now.
There is one commit in indigo-devel that is currently not forward-ported to kinetic.

You use the PR2 - do you mind forward porting that commit in that repo, since this tutorial is targeted at kinetic?

Done: PR2/pr2_common#258
I also opened two other PRs to fix PR2 in Kinetic with xacro

I don't have commit access to these repos at this point in time.

is Clearpath still managing that? who is?

sigh

Just leave it be for another month or so, we'll see..

v4hn · 2016-11-01T14:11:18Z

CMakeLists.txt

@@ -17,6 +17,7 @@ find_package(catkin REQUIRED
    moveit_ros_planning_interface
    pluginlib
    geometric_shapes
+    moveit_visual_tools


If you would like to rely on moveit_visual_tools, shouldn't we move the package in with the rest of moveit?

I've considered this. We would also probably want to move rviz_visual_tools in too, since it depends on it. But I've poured many years of work into those two packages though and kind of like to keep it under my name. Otherwise I'd be more restricted in developing it further - its kind of my favorite pet project repo. I've released it into every version of ROS since Hydro and faster than MoveIt! gets released. I'd rather keep it separate in the short term.

btw have you tried it? its a really powerful package IMHO :)

But I've poured many years of work into those two packages though and kind of like to keep it under my name.

I believe this is a perspective you know very well from other programmers and you argued against it quite a lot in recent days. I don't see how you don't "keep it under your name" if we move it to ros-planning. You're the listed author and maintainer.
It just means that the ros-planning group officially supports/endorses the package and it gets a more "official" location.
I agree that we might not want to merge it into the moveit repository.
Maybe it's worth a thought to add it to the moveit meta-package.
After all, this patched tutorial is a lot of advertisement for your package and makes it look like an off-the-shelf moveit component imho. You don't get it on your machine if you install "moveit" (e.g. ros-kinetic-moveit) though.

You say it's a powerful and useful package that supports the MoveIt framework. I believe we should try to keep such packages under the ros-planning umbrella where possible.

Otherwise I'd be more restricted in developing it further - its kind of my favorite pet project repo.

If you don't want to keep the API you use in the tutorials stable, I'm definitely against using it in the tutorials at all! But I suppose you do.
If this just refers to merging it into the repo, see above.

I have always used deprecated warnings within a ROS release, but have sometimes changed API a little for each new ROS version. Now that my PhD is rounding out development will likely slow down and keep it very stable.

Assuming it is wanted, I've decided yes I'm happy to move it to ros-planning. I'll open a issue for it to discuss further. What are your thoughts on the dependent package rviz_visual_tools?

Is this the last issue blocking this PR from being merged?

Assuming it is wanted, I've decided yes I'm happy to move it to ros-planning. I'll open a issue for it to discuss further.

Awesome! Please do that. It clearly looks like a valuable addition.

Concerning rviz_visual_tools, the obvious candidate is https://github.com/ros-visualization .
I don't know what they think of it there.
At first glance it looks like your Swiss army knife and maybe does a bit too much. (at least by now there are other packages for converting Eigen and tf messages for example..)

We could either include this in ros-planning too, if ros-visualization is no option, or you keep it in your personal repo, as this is not "planning" related.

Is this the last issue blocking this PR from being merged?

Yes. The patched tutorial looks really great!

Proposed moveit/moveit#348

Ready to merge.

v4hn · 2016-11-01T14:14:13Z

doc/pr2_tutorials/planning/launch/move_group_interface_tutorial.launch

@@ -1,7 +1,5 @@
 <launch>

-  <include file="$(find pr2_moveit_config)/launch/demo.launch"/>
-


We should keep this consistant across the tutorials.
Some of the other launch files also include demo.launch.

I agree it should be consistent across the tutorials, but I don't have time to work on the other tutorials right now. This one alone took an entire 8 hours.

I really dislike the workflow of having a single roslaunch file that includes Rviz and whatever demo/script you are testing. I way prefer to launch Rviz and move it to the area on my screen I want, then be able to launch and relaunch the scripts that actually do the things I'm testing. Previously this tutorial had a 20 second sleep "to allow Rviz to launch" which I really detest - its typically a huge waste of time for the user and its confusing.

Note the python version of this tutorial now also uses two launch commands: #47

Note the python version of this tutorial now also uses two launch commands: #47

:D That's cheating, you asked them to change it to fit your request here. ;)

Fine by me. For now, let's keep in mind that this has to be adapted in the other tutorials too.

v4hn · 2016-11-01T14:17:10Z

doc/pr2_tutorials/planning/src/doc/move_group_interface_tutorial.rst


- roslaunch moveit_tutorials move_group_interface_tutorial.launch
+  git clone https://github.com/PR2/pr2_common.git -b kinetic-devel
+  git clone https://github.com/davetcoleman/pr2_moveit_config.git


where is the problem with relying on moveit_pr2 here?

moveit_pr2 has way too many dependencies/custom plugins, and they are not available in kinetic AFAIK. This new moveit_config for the PR2 uses all default MoveIt! components, which is the easiest to maintain and understand. Its up to date with the latest Setup Assistant template files and uses the default KDL solver. Maintenance on the moveit_pr2 repo seems to be lacking and I am not interested in getting involved in that. This new repo was quick and easy for me to setup and allowed me to fix a very broken tutorial. I am happy to move my version elsewhere (e.g. ros-planning org) and eventually switch back to moveit_pr2 if someone else puts in that effort.

ok. Makes sense

v4hn · 2016-11-01T14:18:47Z

doc/pr2_tutorials/planning/src/doc/move_group_interface_tutorial.rst


-After a short moment, the Rviz window should appear:
+Start Rviz


"Start RViz and Demo move_group" if you include the demo.launch here.

good clarification

v4hn · 2016-11-01T14:19:17Z

doc/pr2_tutorials/planning/src/doc/move_group_interface_tutorial.rst

+Running the demo
+^^^^^^^^^^^^^^^^
+
+In a new window, run the `move_group_interface_tutorial.launch <https://github.com/ros-planning/moveit_tutorials/tree/kinetic-devel/doc/pr2_tutorials/planning/launch/move_group_interface_tutorial.launch>`_ roslaunch file::


I would really prefer "In a new terminal" here.

v4hn · 2016-11-01T14:22:49Z

doc/pr2_tutorials/planning/src/move_group_interface_tutorial.cpp

+  visual_tools.deleteAllMarkers();
+
+  // Remove control is an introspection tool that allows users to step through Rviz via buttons and keyboard
+  // shortcuts in Rviz


Typo "Remove control" and what do you mean by "step through Rviz in Rviz"?

Thanks, revised to "Remote control is an introspection tool that allows users to step through a high level script via buttons and keyboard shortcuts in Rviz"

v4hn · 2016-11-01T14:35:42Z

doc/pr2_tutorials/planning/src/move_group_interface_tutorial.cpp

@@ -195,11 +213,21 @@ int main(int argc, char **argv)
  // start state that we have just created.
  move_group.setPoseTarget(target_pose1);

+  // Planning with constraints can be slow because every sample must call and inverse kinematics solver.


"must call and" ?

fixed to "an"

v4hn · 2016-11-01T14:41:58Z

doc/pr2_tutorials/planning/src/move_group_interface_tutorial.cpp

-  // to be sure the planner has enough time to plan around the box.  10 seconds
-  // should be plenty.
-  move_group.setPlanningTime(10.0);
+  // Sleep so we have time to see the object in RViz


Please add the second reason for the sleep here too: move_group needs time to receive and process the collision object message.

Fixed.

Side note: is the collision object ActionServer capability you added available yet in the PlanningSceneInterface? Would be great to use that instead of these crappy sleep timers, but I saved that project for a future improvement (maybe you could add it?)

v4hn · 2016-11-01T14:43:11Z

doc/pr2_tutorials/planning/src/move_group_interface_tutorial.cpp

+  visual_tools.publishText(text_pose, "Object attached to robot", rvt::WHITE, rvt::XLARGE);
+  visual_tools.trigger();
+
+  /* Sleep to give Rviz time to show the object attached (different color).*/


same as above.

davetcoleman · 2016-11-03T15:47:23Z

ready to merge

davetcoleman · 2016-11-10T18:36:50Z

ping

davetcoleman · 2016-11-17T18:11:50Z

ping @v4hn

v4hn · 2016-11-24T19:04:27Z

Oh gosh, sorry! Thanks a lot for the great contribution and thanks a lot for sharing the visualization tools! merging :)

huckl3b3rry87 · 2017-07-11T21:36:18Z

I am a complete newb to this, but I am curious, why does this not get pushed to the master branch so that moveit/moveit#110 is no longer an issue? Is it because the rest of the tutorials are based of indigo? If so, it could still be pushed to the packages that install when a kinetic user follows this http://moveit.ros.org/install/. Just curious and getting started. Thanks!

v4hn · 2017-07-12T08:26:32Z

Hey @huckl3b3rry87 which master branch do you refer to?
This repo has only indigo-devel and kinetic-devel branches.
And this patch has been merged into the latter.

Concerning your install question, iirc we decided not to release the moveit_tutorials package, because if you want to follow them, it makes more sense if you build it in your own workspace.
So it will not install with apt install ros-kinetic-moveit.

* move_group_python_interface fixup

Revamp move_group_interface_tutorial

7b2084b

Reorder tutorial to have demo first, then code explanation Add MoveItVisualTools to enable much better visuals Add step-by-step pause functionality to allow user interaction of demo Added video

v4hn requested changes Nov 1, 2016

View reviewed changes

davetcoleman mentioned this pull request Nov 2, 2016

Modified OMPL default planner handling. moveit/moveit#340

Merged

Wording improvements from @v4hn feedback

73d51d3

davetcoleman force-pushed the kinetic-move-group branch from 3d37922 to 73d51d3 Compare November 3, 2016 14:57

davetcoleman mentioned this pull request Nov 5, 2016

Add steps to run python code #47

Merged

This was referenced Nov 11, 2016

Move moveit_visual_tools into ros_planning moveit/moveit#348

Closed

Update pr2 tutorials to work with Kinetic moveit/moveit#110

Closed

v4hn approved these changes Nov 24, 2016

View reviewed changes

v4hn merged commit f05b11a into moveit:kinetic-devel Nov 24, 2016

davetcoleman deleted the kinetic-move-group branch July 21, 2017 16:28

RoBoLog mentioned this pull request Aug 9, 2017

moveit_tutorials planning_scene_ros_api_tutorial Object does not get removed. Moveit kinetic #98

Closed

BryceStevenWilley pushed a commit to BryceStevenWilley/moveit_tutorials that referenced this pull request May 5, 2018

Kinetic panda mgpi (moveit#45)

01dbe92

* move_group_python_interface fixup

davetcoleman pushed a commit that referenced this pull request May 15, 2018

Kinetic panda mgpi (#45)

c10ba47

* move_group_python_interface fixup


		roslaunch moveit_tutorials move_group_interface_tutorial.launch
		git clone https://github.com/PR2/pr2_common.git -b kinetic-devel

		@@ -1,7 +1,5 @@
		<launch>

		<include file="$(find pr2_moveit_config)/launch/demo.launch"/>


		After a short moment, the Rviz window should appear:
		Start Rviz

Revamp move_group_interface_tutorial #45

Revamp move_group_interface_tutorial #45

Conversation

davetcoleman commented Nov 1, 2016 • edited Loading

v4hn 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

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

v4hn Nov 11, 2016 • edited Loading

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

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

davetcoleman commented Nov 3, 2016

davetcoleman commented Nov 10, 2016

davetcoleman commented Nov 17, 2016

v4hn commented Nov 24, 2016

huckl3b3rry87 commented Jul 11, 2017

v4hn commented Jul 12, 2017

davetcoleman commented Nov 1, 2016 •

edited

Loading

v4hn Nov 11, 2016 •

edited

Loading