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

Use image_transport and cv_bridge targets #62

Merged
merged 2 commits into from
Jan 19, 2022

Conversation

sloretz
Copy link

@sloretz sloretz commented Jan 18, 2022

Fixes #61
⚠️ This needs to be targeted at a ROS Rolling specific branch. Done

@sloretz sloretz added the bug label Jan 18, 2022
@sloretz sloretz requested a review from mabelzhang January 18, 2022 16:56
@sloretz sloretz self-assigned this Jan 18, 2022
@sloretz
Copy link
Author

sloretz commented Jan 18, 2022

Rolling PR job failed because it used ros-rolling-image-transport 3.1.1, but the target is in 3.1.2. The PR has been merged: ros/rosdistro#31776 , so maybe the rolling image hasn't been updated yet?

@sloretz sloretz changed the base branch from foxy-devel to rolling-devel January 18, 2022 19:39
@sloretz sloretz marked this pull request as ready for review January 18, 2022 19:40
@mabelzhang
Copy link
Contributor

I approved but maybe the checks should be rerun?

Signed-off-by: Shane Loretz <[email protected]>
@sloretz sloretz changed the title Use image_transport::image_transport target Use image_transport and cv_bridge targets Jan 18, 2022
@sloretz
Copy link
Author

sloretz commented Jan 18, 2022

I approved but maybe the checks should be rerun?

Yeah, I'm not totally sure how to test this. The rolling jobs don't seem to have built the latest versions of it's dependencies yet. I pushed another commit since I think it will have the same issue with cv_bridge when vision_opencv gets a release. I'll try a local build and report back.

@sloretz
Copy link
Author

sloretz commented Jan 19, 2022

I'll try a local build and report back.

With the latest version of vision_opencv and this PR this package builds successfully. I tested that I can load the plugin in rqt and visualize an image using image_tools cam2image as a test source. I think that's the best I can test this PR, and the results LGTM

@sloretz sloretz requested a review from mabelzhang January 19, 2022 01:08
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

@sloretz sloretz merged commit 786a326 into rolling-devel Jan 19, 2022
@sloretz sloretz deleted the use_image_transport_modern_cmake branch January 19, 2022 16:23
@nuclearsandwich
Copy link

The rolling jobs don't seem to have built the latest versions of it's dependencies yet. I pushed another commit since I think it will have the same issue with cv_bridge when vision_opencv gets a release. I'll try a local build and report back.

This is a tradeoff we pay since PR jobs run based on the testing repository but the failure of rqt_image_view is blocking the sync to testing from building since ros-desktop can't be built without this package. 🙃

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.

3 participants