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

Migrate InteractiveMarkerDisplay #457

Merged
merged 15 commits into from
Sep 10, 2019
Merged

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Aug 31, 2019

Closes #98

Depends on ros2/ros2#770

Some changes since ROS 1 based on discussion in ros-visualization/interactive_markers#42:

  • The feedback publisher is now maintained by the interactive marker client (not RViz)
  • Since clients and servers now communicate with a mix of services and topics, the ROS topic property should be replaced with custom property for interactive marker server discovery.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Opening for visibility. This isn't quite ready for review as there are some things I'd still like to do:

  • Add a custom property object for selecting interactive marker servers to connect to (as eluded to in the description).
  • Review code for areas that can be refactored for the better
    • So far I've done the minimum to get the code to compile and run without crashing, but there may be places in the code that might be ripe for updating.
  • Add tests. Visual tests would be nice.

For those interested in trying out the display, you can check out this branch and clone the interactive_markers package to your workspace: https://github.com/ros-visualization/interactive_markers/tree/ros2.

I've ported the demos too. You can find them here: https://github.com/jacobperron/visualization_tutorials/tree/jacob/ros2

@jacobperron jacobperron added the in progress Actively being worked on (Kanban column) label Aug 31, 2019
@wjwwood wjwwood mentioned this pull request Aug 31, 2019
34 tasks
Added some simple tests for the new property.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This follows the tf2::BufferCoreInterface API, avoiding issues where third-party
libraries are expecting to deal with tf2 exceptions rather than rviz specific
exceptions (e.g. like interactive_markers).

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

Planning to add some tests still, but the rest is ready for review.

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 6, 2019
The full communication reset isn't necessary.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Not necessary in ROS 2 port of interactive_markers.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

I've been working on tests, but it looks like we should add/extend mock objects.
@wjwwood If it's alright with you I'll merge this (pending CI) and follow-up with improving test coverage.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Sep 10, 2019

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate InteractiveMarkerDisplay
2 participants