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 to using cv_bridge type adapter #584

Closed
wants to merge 1 commit into from

Conversation

mjcarroll
Copy link
Member

Type type adapter is now available in cv_bridge, so it can be removed from here.

Closes #583

Signed-off-by: Michael Carroll [email protected]

@clalancette
Copy link
Contributor

So the big problem with this is that cv_bridge isn't currently in https://github.com/ros2/ros2/blob/rolling/ros2.repos .

We could think about pulling it in, but of course that makes it release critical and makes Open Robotics the defacto maintainer of it (since it has to work every night). I'm not sure that we want to do this.

@mjcarroll
Copy link
Member Author

I'm not sure that we want to do this.

Ah, I was picking up an easy issue here as I was in image_tools anyway. That's a good point about it not being in repos (still!)

@clalancette
Copy link
Contributor

We discussed this at the meeting today.

There were mixed feelings about this PR, split into 2 major topics.

The first topic is what demos are supposed to be. Originally, demos were meant to show off particular features of the ROS 2 core. The demos nowadays show off multiple features, and are used both as examples on how to do things, as well as show off particular features. As a repository full of examples, some felt that having code duplicated here didn't show best practices of reusing code from other parts of the ecosystem. In order to do that, though, we would have to bring in cv_bridge as a dependency to the ROS 2 core (essentially what is in https://github.com/ros2/ros2/blob/rolling/ros2.repos). That is the second topic.

The question is whether cv_bridge should be included in the core. On the one hand, it is undoubtedly an important part of the ROS ecosystem. On the other hand, adding it to the core makes it release-critical, and means the core team essentially become de-facto maintainers of it. It also adds in additional CI burden. One big open question is: does https://github.com/ros-perception/vision_opencv compile/work on Windows? The other big open question is how much additional compile/test time vision_opencv would add to the core. To try to find that out, I've run the following set of CI jobs, which includes vision_opencv:

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

With all of that said, in order to get this in, the first step here would be to open a PR to https://github.com/ros2/ros2 , adding vision_opencv to ros2.repos. If you'd like to pursue this, please feel free to open that PR and we can discuss more there. For now, I'm going to close this one out.

@ijnek
Copy link

ijnek commented Sep 30, 2022

I wasn't aware about this PR, thanks @mjcarroll.
vision_opencv was somewhat abandoned over the last couple of years and I've only just gone through the numerous issues and PRs.

With all of that said, in order to get this in, the first step here would be to open a PR to https://github.com/ros2/ros2 , adding vision_opencv to ros2.repos. If you'd like to pursue this, please feel free to open that PR and we can discuss more there. For now, I'm going to close this one out.

I believe vision packages are crucial for most robots out there, and it is more beneficial to improve the existing packages rather than developing something from scratch and asking users to transition. However, given its current state there's a lot of work to get the packages in vision_opencv up to a high quality level.

I'd like to proceed with your suggestion of opening a PR against ros2/ros2 once the repository is back at a higher quality level. Given that I only have my spare time to do this (amongst a lot of other things I work on), I'd appreciate any support from Open Robotics and the community to help achieve this.

@jacobperron
Copy link
Member

FWIW, cv_bridge depends on Boost, which we've historically avoided as a dependency in our ros2.repos file (I think to try and avoid dependency bloat). So we might want to look into removing the Boost dependency before adding it to the repos file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

image_tools: Use type adapter for cv::Mat from cv_bridge
4 participants