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

Modified Luanch files tutorial, added a note to the README. #1008

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Jakeisthesnake
Copy link
Contributor

Description

Launch File tutorial
Added explanations for the member function of MoveItConfigsBuilder.
Added note about launch nodes for custom .cpp files.
Having made these suggestions, I now wonder if I am being too verbose and just cluttering up the documentation when someone wanting to learn more can do what I did and read the source code. So maybe the launch file tutorial should remain unchanged.

README
Added a note that building MoveIt can be skipped if only adding explanations, note any executable code. This is mostly addressing my own pain of building MoveIt (about a 3-4 hour process) and I could see it stopping someone from adding a few helpful comments.

Please explain the changes you made, including a reference to the related issue if applicable

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

…mber funtions.

Added note to Launch FIles tutorial about nodes for custom cpp files.
Added note to README.md that building Moveit is not needed if only adding explinations, not exectuable code. This is mostly addressing the pain I feel of building MoveIt (3-4 hours on my laptop). Maybe I am an anomily.
@Jakeisthesnake
Copy link
Contributor Author

As an aside, I would have liked to add more about '.planning_scene_monitor()' regarding when you need to set its parameters to be true, but I haven't found good explanations regarding common uses cases. If you can point me in the direction of where to learn more, I would be happy to add that info to the tutorial.

Copy link
Contributor

@sea-bass sea-bass 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 for getting this started!

My first response here is that the information being added is largely already available in other tutorials. I would recommend reading through some of the other pages I've linked in the comments, and seeing if there's a better way to take advantage of cross-linking doc pages, but keeping each individual page focused on a specific subset of topics.

@@ -29,6 +29,8 @@ Below are some links to help with the ports.

## MoveIt Tutorials Source Build

(This section can be skipped if you aren't adding executable code. You don't need to build MoveIt to add an explanation about something.)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this

Comment on lines +48 to +49
``MoveItConfigsBuilder(package_name="package_name")`` will search for a package named "package_name".
``MoveItConfigsBuilder("robot_name")`` will search for an explicitly given package name.
Copy link
Contributor

Choose a reason for hiding this comment

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

might be more readable to have this be a bulleted list

@@ -44,10 +44,44 @@ A handy way to refer to a MoveIt configuration package is to use the ``MoveItCon
.to_moveit_configs()
)

``MoveItConfigsBuilder`` (`defined here <https://github.com/moveit/moveit2/blob/main/moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py>`_) can take a few different types of arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note there is already another doc page that talks about this -- maybe cross-link / consider putting some of this there?

https://moveit.picknik.ai/main/doc/how_to_guides/moveit_configuration/moveit_configuration_tutorial.html

Comment on lines +252 to +255
Launching a custom .cpp file
----------------------------

While not part of the Getting Started tutorial, another common node to launch is one that executes a custom .cpp file:
Copy link
Contributor

@sea-bass sea-bass Jan 20, 2025

Choose a reason for hiding this comment

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

A custom .cpp file doesn't sound technically precise -- I think what you may want to say is "a custom executable".

Also, this in isolation may not be entirely helpful if you also don't show a CMakeLists.txt snippet showing how to actually compile this executable.

But generally, there are other tutorials like https://moveit.picknik.ai/main/doc/tutorials/your_first_project/your_first_project.html which already cover this in full detail.

Comment on lines +264 to +266
parameters=[your,
parameters,
here],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't consider this too helpful because I always forget what the actual format of the parameters is -- it's a list of dictionaries or something? So I may consider using a real, valid input here instead of placeholder text.

@Jakeisthesnake
Copy link
Contributor Author

Ok, I'll make those edits. Do you know if any of the tutorials discuss the parameters for .planning_scene_monitor()? It seems lacking from the moveit_configuration_tutorial.

@sea-bass
Copy link
Contributor

Ok, I'll make those edits. Do you know if any of the tutorials discuss the parameters for .planning_scene_monitor()? It seems lacking from the moveit_configuration_tutorial.

Thanks!

Maybe those could be mapped to the concepts page at https://moveit.picknik.ai/main/doc/concepts/planning_scene_monitor.html and/or https://moveit.picknik.ai/main/doc/examples/planning_scene_monitor/planning_scene_monitor_tutorial.html ?

@Jakeisthesnake
Copy link
Contributor Author

Yeah, I can start with those.

@Jakeisthesnake
Copy link
Contributor Author

Jakeisthesnake commented Jan 24, 2025

If I wanted to cross link to the CMakeLists.txt file in the root moveit2_tutorials directory, how would I do that?
:doc:moveit2_tutorials/CMakeLists.txt (the backticks turn "moveit2_tutorials/CMakeLists.txt" into inline code)
doesn't seem to work. I assume that is because :doc: only works for files in the doc folder? I also assume that it would be poor form to directly link to "https://github.com/moveit/moveit2_tutorials/blob/main/CMakeLists.txt"

@sea-bass
Copy link
Contributor

If I wanted to cross link to the CMakeLists.txt file in the root moveit2_tutorials directory, how would I do that? :doc:moveit2_tutorials/CMakeLists.txt (the backticks turn "moveit2_tutorials/CMakeLists.txt" into inline code) doesn't seem to work. I assume that is because :doc: only works for files in the doc folder? I also assume that it would be poor form to directly link to "https://github.com/moveit/moveit2_tutorials/blob/main/CMakeLists.txt"

I believe you can use :codedir: -- you can see the definitions here, and also search for similar usages in the .rst files in these docs.

There are other handy ones for linking to e.g. source code in the MoveIt 2 repo, etc.

@Jakeisthesnake
Copy link
Contributor Author

Thanks!

@Jakeisthesnake
Copy link
Contributor Author

I may be reading the definition wrong, but it looks like codedir can only reference stuff in the doc folder?

"codedir": (
        "https://github.com/"
        + html_context["github_user"]
        + "/moveit2_tutorials/blob/"
        + html_context["github_version"]
        + "doc/%s",
        "",

@sea-bass
Copy link
Contributor

It does look that way -- maybe instead of linking to the existing repo's CMakeLists.txt, you can just copy-paste a small relevant snippet of what you intend to show?

I assume this is just the compilation of a specific executable given source files?

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.

2 participants