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.

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

add goal tolerance examples #657

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

gautz
Copy link

@gautz gautz commented Jul 19, 2021

Description

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@jackcenter
Copy link

During the new demos, where does the object being gripped go? It looks like it doesn't get accounted for in the simulation I ran.

goalToleranceDemo.mp4

@gautz
Copy link
Author

gautz commented Jul 31, 2021

indeed the motion with goal tolerance is realized without object attached. Is this an issue?

@jackcenter jackcenter closed this Aug 4, 2021
@jackcenter jackcenter reopened this Aug 4, 2021
@jackcenter
Copy link

jackcenter commented Aug 4, 2021

Accidently closed this, it's open again.

In regard to the last comment, it's not really an issue so much as I wanted to make sure it's what you intended. Since the object is attached at this point (starting at step 7) would it make sense to plan with the object attached? Maybe the quickest fix is just to reorder the steps and put your new demos right after 6 instead of after 7, then we don't have to think about the attached object.

@gautz
Copy link
Author

gautz commented Aug 16, 2021

I changed the demo and updated the corresponding text to follow the video attached

move_group_tutorial_goal_tolerance.mp4

@gautz gautz requested a review from jackcenter August 16, 2021 15:48
@jackcenter
Copy link

Cool video. Looks good to me. If you post this video up on YouTube and link it to the tutorial page it would be a nice addition! But it looks like everything works and flows nicely.

Copy link

@jackcenter jackcenter left a comment

Choose a reason for hiding this comment

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

Everything built and ran correctly. Relates to moveit/moveit#2780

@gautz
Copy link
Author

gautz commented Sep 1, 2021

I posted the video on Youtube https://youtu.be/xwB7tpZK9-o

@gautz gautz requested a review from jackcenter September 30, 2021 09:18
@jackcenter
Copy link

Thanks for pinging me on this and sorry it has taken so long for a review. I'll take a look today or early next week and make sure this and the other PR moves forward.

Copy link

@jackcenter jackcenter left a comment

Choose a reason for hiding this comment

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

Depends on moveit #2780. That will need to be merged before this can be done.

@@ -28,18 +28,21 @@ After a short moment, the RViz window should appear and look similar to the one

Expected Output
---------------
See the `YouTube video <https://youtu.be/_5siHkFQPBQ>`_ at the top of this tutorial for expected output. In RViz, we should be able to see the following:
See the `YouTube video <https://youtu.be/xwB7tpZK9-o>`_ at the top of this tutorial for expected output. In RViz, we should be able to see the following:

Choose a reason for hiding this comment

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

New video shows the new functionality and looks good. Similar to the old video.

@jackcenter jackcenter requested review from AndyZe and tylerjw and removed request for AndyZe October 4, 2021 17:30
@gautz
Copy link
Author

gautz commented Oct 6, 2021

I slightly improved the Tutorial to put numbered headings corresponding to the overview list. I don't know how to test if the formatting is correct though (deploy Website locally).

The industrial CI test error happens because of the dependency to the corresponding MoveIt PR.

@tylerjw
Copy link
Member

tylerjw commented Oct 6, 2021

deploy Website locally

that is exactly how you test it. You run ./build_locally.sh and see what the formatting looks like.

@tylerjw
Copy link
Member

tylerjw commented Oct 6, 2021

Another thing, which I hope doesn't disappoint you is that I'm working on making the tutorial website more focused towards new users on foxy.

That means that new features like this will be on the main branch of the tutorial page and will go onto the site itself with the next release. The reason behind this is that github pages (what the tutorial is) is not designed to deploy from different branches. Long term (atleast by the Humble release) we will need a solution that resembles (or uses) the official ros buildfarm where we can have different versions of the tutorials come from different branches. If you use the drop down on the top left of the website to switch versions you'll notice that noetic points to the moveit_tutorials repo, and melodic points to a ros buildfarm website (docs.ros.org).

@gautz
Copy link
Author

gautz commented Oct 7, 2021

I checked and corrected the formatting. Thanks for the infos regarding the tutorials. No problem if the changes only appear for the next release :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants