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.

Sign up for GitHub

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

Jump to bottom

visualization tutorial is now kinetic compatible #92

Merged
merged 5 commits into from
Aug 10, 2017
Merged

visualization tutorial is now kinetic compatible #92

merged 5 commits into from
Aug 10, 2017

Conversation

maxgittelman
Copy link
Contributor

I walked through this myself. I only have a few questions:

  1. I couldn't find the equivalent kinetic install package for ros-indigo-moveit-pr2 so instead introduced a sourced download of the pr2_moveit_config package mentioned to me by @davetcoleman .
  2. There are quite a few warnings when running the launch file but maybe this is a side effect of @davetcoleman slimming down the config package. It didn't seem to effect the tutorial in any way.
  3. In Step 2, numbered list 3. the option to select Scene Robot > Show Scene Robot doesn't exist, so I changed the tutorial to state that you should select Scene Robot > Show Robot Visual. Oddly the planned group (in this case the arm) only becomes highlighted in green when you deselect Scene Robot > Show Robot Visual . I didn't remove this from the tutorial but I believe this shouldn't be selected at all and would like a second opinion.

0ldham
0ldham reviewed Aug 1, 2017
View reviewed changes
doc/ros_visualization/visualization_tutorial.rst Outdated
cd src
git clone https://github.com/davetcoleman/pr2_moveit_config.git
cd ..
catkin_make
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be catkin build, not catkin_make, because the rest of the tutorials use catkin tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, what exactly is the difference? I'm quite new to ROS and would appreciate an explanation to the new guy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is some documentation on catkin build and catkin_make's history. I'm not very familiar with catkin tools myself, but build is used throughout the other tutorials so it would be more consistent to continue its usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see moveit's main source install instructions for how to use catkin build - its much better! http://moveit.ros.org/install/source/

@davetcoleman
Copy link
Member

I couldn't find the equivalent kinetic install package for ros-indigo-moveit-pr2 so instead introduced a sourced download of the pr2_moveit_config package mentioned to me by @davetcoleman .

There is no Kinetic package for that, because the maintainers of the PR2 have been slow to release it for Kinetic.

There are quite a few warnings

Those warnings are likely from from the pr2_description package, which I have fixed but the changes have never been released. I just created an issue: PR2/pr2_common#265

Oddly the planned group (in this case the arm) only becomes highlighted in green when you deselect Scene Robot > Show Robot Visual .

This doesn't sound right. Try unchecking all of the various robot displays until the robot disappears. Then enable the planned group - does it appear correctly?

davetcoleman
davetcoleman requested changes Aug 2, 2017
View reviewed changes
Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your effort on this!

doc/ros_visualization/visualization_tutorial.rst Outdated
mkdir moveit_ws
cd moveit_ws
mkdir src
cd ..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to the same two-liner used in the source install instructions?

mkdir -p ~/ws_moveit/src
cd ~/ws_moveit/src

doc/ros_visualization/visualization_tutorial.rst Outdated
cd src
git clone https://github.com/davetcoleman/pr2_moveit_config.git
cd ..
catkin_make
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see moveit's main source install instructions for how to use catkin build - its much better! http://moveit.ros.org/install/source/

maxgittelman
maxgittelman commented Aug 2, 2017
View reviewed changes
Copy link
Contributor Author

@maxgittelman maxgittelman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this install page for how to install catkin build to use? Not how to clone a remote repo as a package?

@davetcoleman
Copy link
Member

The MoveIt! source install page allows you to quickly download ~10 repos at once and build them using catkin. It uses git clone. I'm not sure what your question is.

@davetcoleman davetcoleman merged commit cdc30c5 into moveit:kinetic-devel Aug 10, 2017
@jwhendy
Copy link
Contributor

jwhendy commented Aug 24, 2017 •
edited
Loading

In #91 I added an image to show how to re-add the interact button to this tutorial. It was merged, but afterward this was merged which appears to have undone that. Or maybe it was intentional, as some of the "add the interact tool" wording was changed. The .png is still present, so if it was considered unhelpful, maybe we should just purge it.

If this was a conflicting PR issue, can you help me understand how that should work? If I was doing this to my own branch from two separate computers, I think git would complain that HEAD/master/origin (blanking on the actual error) had changed since my last pull. How does that work when contributing PRs?

Do I need to "re-PR" #91 to get it re-added on top of the current branch status?

This was referenced Aug 24, 2017
Merged
Closed
Merged
Open
@maxgitt
Copy link

maxgitt commented Aug 24, 2017

You could try creating a new PR to add the image back in after #92

@jwhendy jwhendy mentioned this pull request Aug 24, 2017
Merged
@jwhendy
Copy link
Contributor

jwhendy commented Aug 24, 2017

@maxgitt I committed, but looks like github treats the original branch as sort of "dead" after the merge, so I did a new PR, #111. I wasn't sure if the image was unhelpful/unneeded and intentionally removed, or if it was just an accidental delete during your re-arranging? If they want it, it's now available :)

davetcoleman pushed a commit to davetcoleman/moveit_tutorials that referenced this pull request Oct 19, 2017
@maxgittelman @davetcoleman
visualization tutorial is now kinetic compatible (moveit#92)
0121fd2 
* added kinetic compatability to setup_assistant

* make visualization tutorial kinetic compatable

* corrected visualization tutorial to RViz checkbox that exists

* catkin build instead of catkin_make
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0ldham 0ldham 0ldham left review comments

@davetcoleman davetcoleman davetcoleman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maxgittelman @davetcoleman @jwhendy @maxgitt @0ldham