use image_transport to subscribe to image messages #523
Conversation
There was a problem hiding this comment.
Your changes look good to me. However, this PR depends on image_common, which is currently not passing CI (latest attempt) on the ros2 branch (the branch mentioned in your other PR: ros2/ros2#884). I would feel better about this new dependency if CI was passing on image_common's ros2 branch.
rviz_default_plugins/test/rviz_default_plugins/displays/image/get_transport_from_topic_test.cpp
Show resolved
Hide resolved
|
@audrow Regarding image_common I agree, this should first be in the general ros2.repos file. However, we need new CI - the latest attempt was 10 months ago. In ros2/ros2#884 there was a different CI attempt, but after that, I put in some effort to fix the CI failures, so Jenkins CI was green at one point. Maybe you could give CI another try? |
audrow
force-pushed
the
feature/reenable_image_transport
branch
from
284ff12
to
55160cd
Compare
|
Building for CI failed ( |
|
I added the Here's the new run: |
|
Here's the full CI:
|
There was a problem hiding this comment.
@audrow Thanks for taking a look. We're probably nearly there.
I don't have push rights to this repository, so I can't simply update this branch. I don't have a Windows machine right here to test, but the problem seems to be just symbol visibility (as always), so this should do the trick.
Also, it's necessary to include the new .hpp headers to get rid of warnings (that should fix mac CI).
rviz_default_plugins/include/rviz_default_plugins/displays/image/image_transport_display.hpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/camera/camera_display.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/image/get_transport_from_topic.cpp
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/image/get_transport_from_topic.cpp
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/image/get_transport_from_topic.cpp
Show resolved
Hide resolved
|
Rerunning after applying changes:
|
|
Ouch, my bad. We have to put the macros to the header files, not the |
|
No worries, I should have caught it in reviewing your suggestions. Here's the new Windows run: Windows |
|
I'll run CI once more to make sure all OSs build:
|
There was a problem hiding this comment.
LGTM, after ros2/ros2#884 is merged in.
Maybe @mjcarroll wants to take a look before this is merged in.
|
@ketatam ros2/ros2#884 has been merged, do you have some time rebasing this PR and seeing whether it builds? Otherwise I can try to do this over the holidays. |
|
Any updates on this? When can this be available to use? |
|
I tested this PR and it still seems to work. Since this is not my repository I cannot merge master into this PR (the conflict is trivial as it's only in the README). So it should be enough to rebase onto master + new CI for this to get merged. Maybe we can make it happen :). |
use image_transport in camera_display and image_display use image_transport to get camera_info_topic
Signed-off-by: Audrow <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com> Co-authored-by: Martin Idel <martin.idel@googlemail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com> Co-authored-by: Martin Idel <martin.idel@googlemail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
audrow
force-pushed
the
feature/reenable_image_transport
branch
from
3b2a554
to
9de7a97
Compare
|
Thanks for the ping, @Martin-Idel. Here's CI after doing a rebase and fixing the merge conflict:
|
|
Could you show me what the steps would be to implement a compressed image to this visualizer? |
ImageDisplay and CameraDisplay now use image_transport to subscribe to image messages, so that it is possible to subscribe to different types of image messages (compressed, theora..).
CameraDisplay also uses image_transport to get the CameraInfoTopic.
This needs image_common to be added, see corresponding PR: ros2/ros2#884
fixes #207