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

Feature/add arrow strip marker #1786

Conversation

rr-tom-noble
Copy link

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

Description

Fixes #1785
Depends on: ros/common_msgs#190

This PR adds a new ARROW_STRIP Marker type for more efficient / reliable rendering of large batches of arrows.

When using ARROW_STRIP, The points field of the Marker message should be populated with N points, where each pair of consecutive points defines the start and end of an arrow, connected tip to tail. Every other property has the same semantics as ARROW, and is applied to all arrows in the strip.

This PR also adds a new test script, arrow_strip_marker_test.py, which renders an ARROW_STRIP around the circumference of a circle, and modifies the orientation over time to test that the pose field applies to all arrows in the strip.

Since ros/common_msgs#190 is unmerged, I hijacked the LINE_STRIP Marker type in marker_utils.cpp createMarker() to return an ArrowStripMarker in order to get the script working for the video. These changes have not been comitted, and the lines of code using the new ARROW_STRIP msg constant are left commented out for until that PR is merged.

Video

The following video shows test_arrow_strip_marker.py in action:

arrow_strip_marker.webm

Documentation

  • ROS Wiki Marker page to update after merging: Wiki access granted

  • Let me know if there are any other docs that need updating.

@rr-tom-noble rr-tom-noble marked this pull request as ready for review March 22, 2023 08:47
@rr-tom-noble
Copy link
Author

@rhaschke Should I be concerned about the build failing? It's not super clear to me what the cause is 😅

@rhaschke
Copy link
Contributor

Since ros/common_msgs#190 is unmerged, I hijacked the LINE_STRIP Marker type in marker_utils.cpp createMarker() to return an ArrowStripMarker in order to get the script working for the video. These changes have not been comitted, and the lines of code using the new ARROW_STRIP msg constant are left commented out for until that PR is merged.

To enable your msg changes, just add your fork+branch of common_msgs as an upstream dependency in ci.yaml.
How do you handle arrows of different length? Do they get different thicknesses, i.e. are they simply scaled?

@rhaschke
Copy link
Contributor

Should I be concerned about the build failing?

Don't be concerned about the ROS build farm failure, which just reports on warnings. However, the GHA jobs should pass.

@rr-tom-noble
Copy link
Author

rr-tom-noble commented Mar 23, 2023

How do you handle arrows of different length? Do they get different thicknesses, i.e. are they simply scaled?

Only the length is changed, all of the arrows have the same head and shaft thickness, controlled through scale e.g.

Screenshot from 2023-03-23 09-40-20

To enable your msg changes, just add your fork+branch of common_msgs as an upstream dependency in ci.yaml.

Thanks, I'll do that now. How exactly does it work? Does UPSTREAM_WORKSPACE take a list of values, and shall I uncomment the lines using the ARROW_STRIP constant throughout my changes?

@rhaschke
Copy link
Contributor

UPSTREAM_WORKSPACE takes a list of values:
UPSTREAM_WORKSPACE: github:rhaschke/python_qt_binding#silent-external-warnings github:rr-tom-noble/common_msgs#feature/add-arrow-strip-marker-type
Yes, please uncomment your final code. That's the idea of using the correct upstream - to test the final code.

@rr-tom-noble
Copy link
Author

UPSTREAM_WORKSPACE takes a list of values: UPSTREAM_WORKSPACE: github:rhaschke/python_qt_binding#silent-external-warnings github:rr-tom-noble/common_msgs#feature/add-arrow-strip-marker-type Yes, please uncomment your final code. That's the idea of using the correct upstream - to test the final code.

Thanks. I've made those changes now 👍

@rr-tom-noble
Copy link
Author

@rhaschke The ABI build seems to be failing due to missing the ARROW_STRIP constant. Am I right in thinking that adding github:rr-tom-noble/common_msgs#feature/add-arrow-strip-marker-type to .github/workflows/ci.yaml should result in that version of common_msgs being built against? Any ideas why it might not be working?

@rr-tom-noble
Copy link
Author

rr-tom-noble commented Mar 24, 2023

Run rhaschke/industrial_ci@master
  with:
  env:
    UPSTREAM_WORKSPACE: github:rhaschke/python_qt_binding#silent-external-warnings
    CCACHE_DIR: /home/runner/work/rviz/rviz/.ccache
    BASEDIR: /home/runner/work
    DOCKER_IMAGE: rhaschke/ici:rviz-noetic-ros
    CACHE_PREFIX: noetic
    ABICHECK_URL: github:ros-visualization/rviz#1.1[4](https://github.com/ros-visualization/rviz/actions/runs/4499797469/jobs/7928292247?pr=1786#step:5:4).1[5](https://github.com/ros-visualization/rviz/actions/runs/4499797469/jobs/7928292247?pr=1786#step:5:5)
    GHA_CACHE_SAVE: always

Ah, I think I forgot to add it somewhere

@rr-tom-noble
Copy link
Author

Okay, I've added that dependency to abi.yaml too

@rhaschke
Copy link
Contributor

Please merge rivelinrobotics#1

Minor adaptions for arrow strip marker
@rr-tom-noble
Copy link
Author

Thanks for the changes. Done 👍

@rr-tom-noble
Copy link
Author

Hey @rhaschke, I noticed the ABI workflow failed here: https://github.com/ros-visualization/rviz/actions/runs/4510995093/jobs/7945031975

Is that something your changes should've fixed, or shall I investigate that?

@rhaschke
Copy link
Contributor

The ABI failure is unrelated and stems from a previous PR. Don't care about that one. The major hurdle for this PR is now to get the message changes merged.

@rr-tom-noble
Copy link
Author

@tfoote Has recommended merging the visualization_msgs changes into ROS2 first. We're planning to migrate to ROS2 in April, so a ROS1 backport would not be super useful to us by then. I'll leave the PR open in case it's of interest to anyone else, but feel free to close if not. I'll raise an equivalent PR with rviz2 next month.

This pull request was closed.
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.

Suggestion: Arrow List Marker
2 participants