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

Adds ARROW_STRIP Marker type #190

Conversation

rr-tom-noble
Copy link

@rr-tom-noble rr-tom-noble commented Mar 21, 2023

Adds a new ARROW_STRIP Marker type for rendering batches of arrows.

Rviz Issue: ros-visualization/rviz#1785
Rviz PR: ros-visualization/rviz#1786

@rr-tom-noble
Copy link
Author

Hey @tfoote, could I get you to take a look at this? The corresponding Rviz PR is ready for review

Copy link
Member

@tfoote tfoote 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 seems like a good addition. My first reaction is to suggest that it be targeted for https://github.com/ros2/common_interfaces not here. This is a bit of a feature enhancement going into a stable release. But this is small enough that I would consider making it an exception.

However I would like to see it reviewed and merged to the new development version: https://github.com/ros2/common_interfaces and then once that's reviewed backported here. That way anyone who starts using it here won't get caught flat footed when they want to move forward.

@@ -12,6 +12,7 @@ uint8 POINTS=8
uint8 TEXT_VIEW_FACING=9
uint8 MESH_RESOURCE=10
uint8 TRIANGLE_LIST=11
uint8 ARROW_STRIP=12
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to call this a DIRECTED_LINE_STRIP as that's an already determined thing and we're just adding direction.

There's a few places below that reference LINE_STRIP that should be augmented.

Copy link
Author

Choose a reason for hiding this comment

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

The geometry shares some functionality with ARROW, most notably the appearance and semantics of the scale field. I think ARROW_STRIP makes this connection more explicit, and conveys the same information as DIRECTED_LINE_STRIP in fewer characters.

@rr-tom-noble
Copy link
Author

Overall this seems like a good addition. My first reaction is to suggest that it be targeted for https://github.com/ros2/common_interfaces not here. This is a bit of a feature enhancement going into a stable release. But this is small enough that I would consider making it an exception.

However I would like to see it reviewed and merged to the new development version: https://github.com/ros2/common_interfaces and then once that's reviewed backported here. That way anyone who starts using it here won't get caught flat footed when they want to move forward.

That makes sense. The reason for targeting ROS1 was simply that it's the platform we're currently using, though we're planning to migrate to ROS2 next month. I'd be happy to raise the PR in https://github.com/ros2/common_interfaces, although due to software restrictions on my work machine, I likely won't be able to work on a ROS2 version of the corresponding Rviz PR until we start the migration in April.

I'll raise that PR now 👍

@rr-tom-noble
Copy link
Author

Closed in favour of ros2/common_interfaces#218

@tfoote
Copy link
Member

tfoote commented Mar 29, 2023

Thanks for jumping it over there.

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.

None yet

2 participants