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

Adding RViz tutorials #3908

Closed
wants to merge 6 commits into from
Closed

Adding RViz tutorials #3908

wants to merge 6 commits into from

Conversation

LeweC
Copy link
Contributor

@LeweC LeweC commented Sep 7, 2023

This PR would close issue #3864

I have marked this as a draft because there are several different ways of including community contributed tutorials.
This is an idea that still needs to be fleshed out. But I wanted to get some feedback on whether this is a place where they could be included.

The files can be filled with better information later.
I am interested in whether this is a suitable structure to include community contributed tutorials. Especially keeping in mind that this can be expanded later.

@clalancette
Copy link
Contributor

I've got a couple of comments, and then a question:

  1. We already have a way to mark community contributed tutorials, using the [community-contributed] suffix on the individual articles. While this doesn't put them all in the same spot, it does mean that they can fit into our structure and still be marked as such.
  2. In this PR in particular, all of the tutorials are for the visualization_tutorials. Given that those are in the ros organization, we could consider that they aren't "community-contributed" at all, and are just a new series of tutorials (though it looks like the code and the documentation in https://github.com/ros-visualization/visualization_tutorials/tree/ros2 probably needs some cleaning up).

So with that said, can you explain more what your overall goal is?

@LeweC
Copy link
Contributor Author

LeweC commented Sep 8, 2023

Thanks for the information! That is what I was for looking for when I said I needed some pointers in the inital Issue.

Im fairly new to this but want to start contributing.

My overall goal is to put these tutorials here somewhere because its very hard to find small working RViz plugins as examples or tutorials.

I dont necessarily want to change your structure or create a new place for it. That was just my first thought when you said they need to be clearly marked as community contributed. But now I see what you meant.

I can change them to fit into the existing structure.

Do you think its fine to have a bit of background information, prerequisite, and then just link to the community page for the actual code, or do I need to transfer the existing tutorial from their repo into the style of: read from top to bottom with instructions and code snippets?

Also:

  • Put all of the repo's tutorials together under the name Visualization Tutorials/Examples?

  • or split the tutorials into beginner, advanced, etc.?

  • Just point to the repo link plus some additional information?

  • or convert the tutorial into similar structure of other tutorials here and not use their way of tutorial structure?

Like I said, I want to contribute but I am not sure how and where it would fit. I also do not want to disturb your structure.
If you have the time you can just tell me where and how it should look and I will do the work.

@LeweC
Copy link
Contributor Author

LeweC commented Sep 8, 2023

Looking more into it, I found this ROS1 RViz Tutorial which seems to be very similar to the mentioned repo.
This has probably not been made for ROS2 yet.
But I think this changes things on how we want to include the ROS2 version since there is already a well documented version.

@clalancette
Copy link
Contributor

So, I apologize in advance but I'm going to backtrack a bit on previous statements.

First of all, these tutorials are all part of a more-or-less official repository, so we don't need to mark them as community-contributed. I had looked too quickly earlier and I missed the exact place that the repository lived in. So let's not worry too much about that bit.

Next, let's think about the structure a bit. What I think would be most useful would be to place all of the tutorials in one place, probably in a subdirectory (similar to how http://docs.ros.org/en/rolling/Tutorials/Intermediate/URDF/URDF-Main.html is done). I would further suggest that these aren't Beginner tutorials, so that leaves us with putting it in either "Intermediate", or "Advanced" (or splitting them up, but I don't think that is a great idea). It feels like at least some of them are "Intermediate", so I'll suggest making an "Intermediate/RViz" directory and putting the tutorials in there.

And that leaves us with the question on how to write the tutorials themselves. Personally I think we should write whole new pages for each of these, following the existing tutorial format. That way it will be consistent with everything else here, and won't rely on the user having to go to another repository and build documentation. I know this is more work, but we don't have any "official" ROS 2 RViz tutorials yet, and this will be a great start towards that.

Let me know if this answers your questions, and if you need any more guidance.

@LeweC
Copy link
Contributor Author

LeweC commented Sep 8, 2023

Great! I totally agree with you about the structure ideas.
I also thought about bundling these tutorials similar to urdf-main. That is why I included a -main.rst in my initial commit.

I have no problem writing new pages for each one.
I really like the idea of getting the ball rolling for better official RViz documentation.

I will start working on it and ask for some feedback from time to time so that I do not go in the wrong direction.
Thanks for your great input and help in getting me started.

@LeweC LeweC changed the title Add first version of community-contributed tutorials Adding RViz tutorials Sep 8, 2023
@LeweC
Copy link
Contributor Author

LeweC commented Oct 9, 2023

Hey, so I think this is the structure we agreed on.

I have ported the two tutorials for the standard marker, but I have not done any fine tuning yet.
If you have no big complaints, I would start with the others soon, but before that I ran into some questions I wanted to clarify with you.

Since these will be under "Intermediate", I initially did not want to explain every detail on how to create and setup the packages, but rather let them clone it via git or wget and just explain the code and concepts. My problem with this is that the existing codebase for this has its own tutorial .rst file in it and it would be quite confusing and unnecessary to have it in the package while following "our" tutorial.

I managed to avoid this problem in the two finished marker tutorials because I included an explanation of how to set up the package, cmake, etc.
In the small standard marker tutorials this is fine I think, also because they are the ones that could be in the beginner section aswell, in my opinion.

But for example the interactice markers have a lot of files and a big CMake. I think we should not do it the same way as the standard marker, but rather have a way to get the finished package that is guaranteed to work.
Similarly, the old tutorials are also more focused on explaining the concept and just providing the packages.

So do you think it is better to provide the packages for the other tutorials as well? If so, how should we do this?

  • We could use the existing ones with their .rst file included
  • You could create an official ROS repo for this.
  • I could create a repo for this, but probably not the official way(?)

@clalancette
Copy link
Contributor

Since these will be under "Intermediate", I initially did not want to explain every detail on how to create and setup the packages, but rather let them clone it via git or wget and just explain the code and concepts.

Yes, that seems reasonable, and is how we do it for other Intermediate tutorials.

My problem with this is that the existing codebase for this has its own tutorial .rst file in it and it would be quite confusing and unnecessary to have it in the package while following "our" tutorial.

Sorry, which codebase is this? The tutorials in https://github.com/ros-visualization/visualization_tutorials/tree/ros2/interactive_marker_tutorials don't seem to have their own .rst files (unless I'm missing something).

@LeweC
Copy link
Contributor Author

LeweC commented Oct 9, 2023

True, the interactive_marker_tutorials package is the only one without. I missed that.

But every other package has at least one. Some are in src/doc in their respective package instead of directly top-level /src of the package. E.g https://github.com/ros-visualization/visualization_tutorials/tree/ros2/rviz_python_tutorial/doc-src

It's not many files in some, others have a few more. Like this one: https://github.com/ros-visualization/visualization_tutorials/tree/ros2/rviz_plugin_tutorials/src/doc
I am worried that it will confuse new people who do the tutorial if we just clone all of the package.

@clalancette
Copy link
Contributor

But every other package has at least one.

Ah, thanks. I had missed that.

I am worried that it will confuse new people who do the tutorial if we just clone all of the package.

My perspective on this is that we have been most successful with people finding the tutorials that are hosted here, rather than in individual packages. There is definitely a place for tutorials in individual packages, but for things that are so core to ROS 2 (like RViz), I think they should be hosted here.

So my suggestion is that we do actually migrate the content here. After that is all done, we can also consider removing it from https://github.com/ros-visualization/visualization_tutorials/tree/ros2 just to keep things easier to understand.

@LeweC
Copy link
Contributor Author

LeweC commented Oct 10, 2023

Okay, great.
So I will write the next tutorials with a placeholder gitclone or wget link (until the code is migrated) and the user will just use the provided package for the tutorial.

I can help migrate the content. I am just not sure where to open the PR for it.
ROS2 Examples and ROS2 Demos both mention that they host code for the tutorial page.

@clalancette
Copy link
Contributor

So I will write the next tutorials with a placeholder gitclone or wget link (until the code is migrated) and the user will just use the provided package for the tutorial.

I don't think this is necessary. Just point to the existing https://github.com/ros-visualization/visualization_tutorials/tree/ros2 files as necessary.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I added in most of my suggestions only one ` but probably you should use two ``

@LeweC
Copy link
Contributor Author

LeweC commented Oct 16, 2023

Thanks for your feedback @ahcorde !
I have almost finished the next part of the tutorials (the 3 about the RViz plugins).
I plan to commit them soon.

As you noticed with the suggestions, they are all still "drafty", there are a few formatting things I need to do.
So I will commit the plugins and then do another "fine tuning" commit for all the tutorials.

After that I will request review from you guys, so that hopefully I will catch most of these and you have less to do the next time.

@LeweC
Copy link
Contributor Author

LeweC commented Oct 18, 2023

I believe this could be a nice working version, after review and a few more careful reads myself.

I'm still not very comfortable pointing to the code file in the visualization_tutorials repository.
I did some more digging and found that the existing .rst, tutorialfomatter.py and conf.py files scattered around the repo are probably from these old tutorials. The page sources of the html file look very similar, if not exactly the same, as the tutorial .rst files in the repo, and if you build it locally with sphinx it uses the same old theme.

There are also a lot of problems when you follow the instructions in the ReadMe to build your own local tutorial files:

  • Interactive Marker does not have the files to build tutorial pages.
  • Standard Marker has the files, but the resulting local files are a bit off. The code in the code block is shifted a bit, so the explanation sometimes does not fit to the code.
  • The plugin package has the necessary files, but there is a syntax error with sphinx when building its html files.

In the first place, this has nothing to do with the tutorials I wrote/parsed, but perhaps we should not point to a repo where the initial ReadMe instructions give wrong results.

Im also thinking about adding the python tutorials for some of these topics in the future, but it feels wierd adding them to this repo because that is the state of the old (and its seems not maintained) tutorial files.

We can keep pointing to it, but in my opinion its not the best option for the future.

@LeweC LeweC marked this pull request as ready for review October 18, 2023 15:06
@clalancette
Copy link
Contributor

In the first place, this has nothing to do with the tutorials I wrote/parsed, but perhaps we should not point to a repo where the initial ReadMe instructions give wrong results.

I think we should just fix the instructions in that repository. In particular, if we are "moving" the tutorials here, then I'd be fine with removing them from that repository altogether. Then we can just add a link in the README over there that says "look at docs.ros.org"

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

First of all, thanks again for doing this.

I've done a very quick first pass, and left a bunch of things to fix throughout the documents. Once those are fixed, I'll do another, more thorough pass.


Backround
---------
Unlike other displays, the `Marker Display <http://wiki.ros.org/rviz/DisplayTypes/Marker>`_ lets you visualize data in RViz without RViz knowing anything about interpreting that data.
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't point to the ROS 1 wiki; someday that will go away. If there is content there that we need to point to, we should consider adding it to this PR.

The same comment goes for any references to wiki.ros.org in these tutorials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I think the "http://wiki.ros.org/rviz/DisplayTypes/Marker" page is pretty important for understanding the basics of the Display Marker. I think there is a lot of useful information there that is not documented anywhere else.

So I have included a new tutorial page in this PR where I have tried to replicate that page.
This page definitely needs to be checked for what information is still relevant / correct for ROS2. I am not 100% sure for some facts in there.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

@LeweC First of all, thanks for starting to whip this into shape. There is a lot of content here.

Secondly, though, looking at this set of tutorials holistically, it seems like we just throw the reader into the deep end. That is, there is no explanation about RViz, what it does, how it works, some of the basic things you can do with it, etc. Without all of that context, just jumping straight into Markers seems a little premature.

In short, what I'm suggesting is that we port something like http://wiki.ros.org/rviz/UserGuide into the ROS 2 documentation first. Once we have that, I think it is much more natural to add in this content.

I know this is a big ask, but are you willing to take a crack at porting that over first (and in a separate PR)? Once that is done, then we can rebase this content on top and I think it will all come together. Please let me know what you think.

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

**Time:** 15 Minutes

.. contents:: Contents
:depth: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:depth: 3
:depth: 2

Backround
---------
The Markers display allows programmatic addition of various primitive shapes to the 3D view by sending a
`visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/Marker.msg>`_ or
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/Marker.msg>`_ or
`visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/{REPOS_FILE_BRANCH}/visualization_msgs/msg/Marker.msg>`_ or

---------
The Markers display allows programmatic addition of various primitive shapes to the 3D view by sending a
`visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/Marker.msg>`_ or
`visualization_msgs/msg/MarkerArray <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/MarkerArray.msg>`_ message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`visualization_msgs/msg/MarkerArray <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/MarkerArray.msg>`_ message.
`visualization_msgs/msg/MarkerArray <https://github.com/ros2/common_interfaces/blob/{REPOS_FILE_BRANCH}/visualization_msgs/msg/MarkerArray.msg>`_ message.


auto marker_pub = node->create_publisher<visualization_msgs::msg::Marker>("visualization_marker", 1);

After that it is as simple as filling out a `visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/Marker.msg>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
After that it is as simple as filling out a `visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/rolling/visualization_msgs/msg/Marker.msg>`_
After that it is as simple as filling out a `visualization_msgs/msg/Marker <https://github.com/ros2/common_interfaces/blob/{REPOS_FILE_BRANCH}/visualization_msgs/msg/Marker.msg>`_

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

:local:


Backround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Backround
Background

@LeweC
Copy link
Contributor Author

LeweC commented Nov 21, 2023

@clalancette No problem, I am glad to have found a useful contribution. By the way, sorry for all the little spelling mistakes and annoying slip-ups. As you said, there is a lot of content and it is hard to catch everything, but I will try to catch a few more next time.

Secondly, though, looking at this set of tutorials holistically, it seems like we just throw the reader into the deep end. That is, there is no explanation about RViz, what it does, how it works, some of the basic things you can do with it, etc. Without all of that context, just jumping straight into Markers seems a little premature.

I agree with you. I think it is a smart thing to do, I will start a separate PR.
I will continue this PR after the UserGuide merge and then we can link between them.

@LeweC
Copy link
Contributor Author

LeweC commented Nov 21, 2023

I will probably get around to opening it tomorrow.
Should I mark this as draft again until the User Guide is finished?

@clalancette
Copy link
Contributor

Should I mark this as draft again until the User Guide is finished?

Yeah, I think that would be best. Thank you.

@LeweC LeweC marked this pull request as draft November 22, 2023 07:52
@LeweC LeweC mentioned this pull request Nov 30, 2023
@clalancette
Copy link
Contributor

@LeweC Now that we've landed #4040 (and to a lesser extent #3384), do you want to try to resurrect this one and port more of the tutorials over? If you have time, of course.

If you want to do that, then I'm going to suggest making one PR per tutorial (with the associated images and text). That will make it much easier to get them in one at a time, and make sure they create a cohesive set.

@LeweC
Copy link
Contributor Author

LeweC commented Apr 16, 2024

Yes, I definitely plan on porting these tutorials soon, I still enjoy working on this project. My new semester just started so I dont really know how much time I will find for it. I do like the idea of having a PR for each one, it makes it easier to work on with limited time and easier to review as well I would imagine.

I will open them as soon as I think they are ready for the first round of review, I learned a lot from the merged PR on how the format should look so hopefully the review phase will be a little quicker in the future.

How do you plan to handle this PR if we do each tutorial separately now?

@clalancette
Copy link
Contributor

Yes, I definitely plan on porting these tutorials soon, I still enjoy working on this project. My new semester just started so I dont really know how much time I will find for it. I do like the idea of having a PR for each one, it makes it easier to work on with limited time and easier to review as well I would imagine.

Fantastic, thank you!

How do you plan to handle this PR if we do each tutorial separately now?

It is kind of up to you. Personally I would close this one, and just deal with the new articles as they come in one-by-one. But if you find this PR useful as a coordination point, we can leave it open for that. Just let me know either way.

@LeweC
Copy link
Contributor Author

LeweC commented Apr 17, 2024

Yeah okay, I will close this one later today.

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