Skip to content

Conversation

riv-tnoble
Copy link
Contributor

ROS2 version of this PR: ros/common_msgs#190

I won't have a chance to work on the Rviz PR to implement the ARROW_STRIP functionality for ROS2 until next month (April), so I suppose we can either leave this PR open until then, or merge with a "currently unimplemented" comment.

I've changed the description above the points field to include the new type, but have left the colours field unchanged as (at least in the ROS1 version of the Rviz PR) this functionality was unimplemented for ARROW_STRIP.

@riv-tnoble riv-tnoble requested a review from tfoote as a code owner March 28, 2023 07:57
@riv-tnoble riv-tnoble force-pushed the feature/add-arrow-strip-marker-type branch from eed9561 to a2bba62 Compare March 28, 2023 08:30
Copy link
Contributor

@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.

+1 for merging into rolling

I think it's safe for backporting too. Publishing of newer data to older implementations will break parsing (hopefully with an unknown datatype type of error) . That should gracefully degrade. If anyone knows of another case we should look into that more if there's strong interest in backporting. I suspect that backporting it will depend on the complexity of the relevant rviz pull request complexity.

@riv-tnoble riv-tnoble force-pushed the feature/add-arrow-strip-marker-type branch from 30b68e8 to a848561 Compare March 30, 2023 08:53
@riv-tnoble
Copy link
Contributor Author

Thanks @tfoote

I've added a "currently unimplemented" comment for clarity.

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.

I think that in order to move forward with this, we'd also want an implementation of the RViz change for RViz2. Then we can remove the unimplemented comment here and merge the two of them together.

@riv-tnoble
Copy link
Contributor Author

riv-tnoble commented Apr 17, 2023

@clalancette

Makes sense. I should have time this week to raise a PR with rviz2. Will update this PR when merged

@riv-tnoble
Copy link
Contributor Author

riv-tnoble commented Apr 24, 2023

@clalancette

Removed the unimplemented comment as the rviz2 PR is underway. In particular, it's waiting on whether the CI for that repo supports compiling against unmerged branches (i.e. this one) in the same way rviz1 does.

If that's not possible, would you be happy for us to reverse the order of the merges? i.e. common_interfaces -> rviz?

@riv-tnoble riv-tnoble requested a review from clalancette May 4, 2023 11:52
riv-tnoble and others added 8 commits May 4, 2023 12:53
Signed-off-by: Tom Noble <[email protected]>
Use format codes instead of ambiguous names.

Signed-off-by: Christian Rauch <[email protected]>
Signed-off-by: Tom Noble <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Tom Noble <[email protected]>
Signed-off-by: Tom Noble <[email protected]>
Signed-off-by: Tom Noble <[email protected]>
@ahcorde
Copy link
Contributor

ahcorde commented Jul 17, 2023

@clalancette are you happy with this PR ?

@ahcorde
Copy link
Contributor

ahcorde commented Jul 17, 2023

@rr-tom-noble this PR has some conflicts, can you solve it ?

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 should be rebased onto the latest, and all changes to files except for Marker.msg dropped. Then this will be a lot closer to be ready to merge (though I think we should wait for ros2/rviz#972 to be ready before we do that).

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.

Do you mind to rabse this with rolling ?

There are many changes not related with this PR

@tfoote
Copy link
Contributor

tfoote commented Apr 10, 2024

I've rebased this onto rolling in #242 Closing this as superseded.

@tfoote tfoote closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants