-
Notifications
You must be signed in to change notification settings - Fork 258
Compressed Image Display #1288
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
base: rolling
Are you sure you want to change the base?
Compressed Image Display #1288
Conversation
Added a property to manually override image transport type. Rviz determines the transport by checking the topic name against image_transport conventions, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, I have pushed a fix.
Please provide details so I can reproduce your issue. |
I would like to list the available image transports in a status message. However, The status message doesn't really matter except the transport->message type map is hard-coded, so we will want a warning if a new transport type is used which does not exist in the map. |
I noticed the Jazzy version of |
Can |
48133ae
to
40711b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I run a image publisher and there is an issue. I'm able to subscribe to raw
and compressed
but I'm not able to subscribe to zstd
.
rviz_default_plugins/include/rviz_default_plugins/displays/image/image_display.hpp
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/publishers/compressed_image_publisher.hpp
Show resolved
Hide resolved
{ | ||
publisher = this->create_publisher<sensor_msgs::msg::CompressedImage>(topic_name, 10); | ||
timer = this->create_wall_timer(100ms, | ||
std::bind(&CompressedImagePublisher::timer_callback, this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include <utility>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what? Do you mean <functional>
for std::bind
?
rviz_default_plugins/test/rviz_default_plugins/publishers/compressed_image_publisher.hpp
Outdated
Show resolved
Hide resolved
All the plugins are handled by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You included many changes which are not related with this PR, I mean the headers inclusion. Do you mind to split the PR in two ?
<test_depend>rviz_rendering_tests</test_depend> | ||
<test_depend>rviz_visual_testing_framework</test_depend> | ||
<test_depend>image_transport_plugins</test_depend> | ||
<test_depend>vision_opencv</test_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea here, vision_opencv
is not a core package, we can't use this one here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCV is used in compressed_image_publisher.hpp, to encode a jpeg image.
1 similar comment
Signed-off-by: Matthew Foran <[email protected]>
Pulls: #1288 |
The "unstable" builds are caused by GitHub rate limiting. I think they changed something recently because I have been seeing it frequently in my own CI processes. |
This is is related with RViz in particular with |
Oh, I see now. That must be the result of some changes outside of this PR because it was not failing in March. I will look into it. |
We would like to get this PR over the line. Is there anything we can do to help here? |
@ijnek sorry, I am in the middle of a cross-country move so I haven't been able to look at this for a while. As of May a new segfault was the only blocker for this PR. I can't provide much support right now but I'll be on it again in a month or so. |
I putting this PR on hold until the issues regarding abstract node interfaces are worked out. |
Addresses #738
I added
RosTopicMultiProperty
, which extendsRosTopicProperty
with a list of message types instead of one single type. This allows theImageDisplay
to show raw and compressed image topics together in its selector list.ImageDisplay
was altered to use animage_transport
subscriber so it can seamlessly display any installed image type. There is also a handful of frivolous changes to comply with the style linters.Tested with raw and compressed images. Assuming the other types work as well because the conversion happens in the
image_transport
library.Uncertainties:
RosTopicMultiProperty
: Is the name ok? Is there a simpler way to achieve my goal without adding a one-off property?plugin_description.xml
: I added the otherimage_transport
topic types here so Image Display appears in the "Add/By topic" menu. Adding the display will fail if the associatedimage_transport
plugin is not installed. Shouldros-$ROS_DISTRO-image-transport-plugins
be added topackage.xml
?