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

Building a custom rviz display #3384

Merged
merged 11 commits into from
Apr 15, 2024
Merged

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Mar 14, 2023

I hate RST.

@DLu
Copy link
Contributor Author

DLu commented Apr 10, 2023

Review please?

@DLu
Copy link
Contributor Author

DLu commented Jul 11, 2023

@clalancette ? @audrow ? Any movement?

@LeweC LeweC mentioned this pull request Aug 24, 2023
@clalancette
Copy link
Contributor

Sorry I never looked back at this.

I think this would be a good tutorial to have, but I also think it belongs in a series with other RViz changes. I'm going to hold off on looking at this one until we land #4040 , at which point this will have a natural place to slot into.

@clalancette
Copy link
Contributor

OK, we've now landed #4040. So I'm going to suggest that this gets moved into a tutorial under the new Tutorials/Intermediate/RViz directory.

@DLu DLu force-pushed the rviz_plugin_tutorial branch from 397e1be to 4afcbb5 Compare April 3, 2024 16:18
@DLu
Copy link
Contributor Author

DLu commented Apr 3, 2024

Rebased, and moved to the new RViz folder, and I even changed things up to follow the same directory structure as the existing tutorial in that folder.

.. code-block:: c++


#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind to use include guards? as we are doing on RViz ?

If you are using the common linters probaby it will require this

#ifndef BLAH_H
#define BLAH_H

#endif  // BLAH_H

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to argue with you about using modern standards, but then I dug and found this bit of the Google guide.

That's not going to stop me from posting a gif that came to mind.

I recognize the council has made a decision, but given that it's a stupid-ass decision, I've elected to ignore it.

(I'm not calling this a stupid decision, because upon further research, pragma is generally supported but is not official lore)

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
DLu added a commit to MetroRobots/rviz_plugin_tutorial that referenced this pull request Apr 5, 2024
@DLu DLu requested a review from ahcorde April 5, 2024 17:59
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.

Thank you sir!

200w

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.

Overall, this is great. Thanks for moving it around.

I left a bunch of stuff to cleanup.

DLu and others added 3 commits April 5, 2024 16:34
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
@DLu
Copy link
Contributor Author

DLu commented Apr 10, 2024

I will update the code style if you can provide a .clang-format file that is to be used canonically for tutorials.

I find this process frustrating, to have a PR sit untouched for a year, then get held up in the end for the placement of curly braces. This seems like perfect is the enemy of the good. If you want to do something to increase the number of community generated tutorials, you need to lower the bar a little.

@clalancette
Copy link
Contributor

I will update the code style if you can provide a .clang-format file that is to be used canonically for tutorials.

Unfortunately, that is impossible; clang-format can't be configured to follow our coding style currently.

@clalancette
Copy link
Contributor

Unfortunately, that is impossible; clang-format can't be configured to follow our coding style currently.

Sorry, that was a little terse. We can't do clang-format, but the right solution here is to enable uncrustify on the end repository, and then fix all of the uncrustify problems. That will then conform to the style that we want. I'm willing to do that work if the rviz_plugin_tutorial repository is available somewhere (that's a blocking issue anyway).

@clalancette
Copy link
Contributor

Sorry, that was a little terse. We can't do clang-format, but the right solution here is to enable uncrustify on the end repository, and then fix all of the uncrustify problems. That will then conform to the style that we want. I'm willing to do that work if the rviz_plugin_tutorial repository is available somewhere (that's a blocking issue anyway).

All right, see MetroRobots/rviz_plugin_tutorial#1

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.

Thanks for sticking with this one. I've left a few more grammatical/layout things to change, but this is looking close.

DLu and others added 2 commits April 14, 2024 15:10
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
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.

This is great. Thanks @DLu for sticking with this. I'm going to go ahead and merge this in, and also backport to iron and humble.

@clalancette clalancette merged commit d9bca49 into ros2:rolling Apr 15, 2024
4 checks passed
@clalancette
Copy link
Contributor

@Mergifyio backport humble iron

Copy link
Contributor

mergify bot commented Apr 15, 2024

backport humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 15, 2024
* Building a custom rviz display

* Move to RViz folder

Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9bca49)
mergify bot pushed a commit that referenced this pull request Apr 15, 2024
* Building a custom rviz display

* Move to RViz folder

Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9bca49)
clalancette pushed a commit that referenced this pull request Apr 15, 2024
* Building a custom rviz display

* Move to RViz folder

Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9bca49)

Co-authored-by: David V. Lu!! <davidvlu@gmail.com>
clalancette pushed a commit that referenced this pull request Apr 15, 2024
* Building a custom rviz display

* Move to RViz folder

Signed-off-by: David V. Lu!! <davidvlu@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9bca49)

Co-authored-by: David V. Lu!! <davidvlu@gmail.com>
@DLu DLu deleted the rviz_plugin_tutorial branch July 30, 2024 15:12
@DLu DLu mentioned this pull request Nov 15, 2024
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.

4 participants