search

ros-perception / vision_opencv

Apache License 2.0
549 stars 601 forks source link

feat: add type adapter for cv::Mat #441

Closed wep21 closed 2 years ago

wep21 commented 2 years ago

I add type adapter for cv::Mat. I just import the feature from image_tools.

wep21 commented 2 years ago

@audrow Could you review this? I just import the feature you created from image_tools.

wep21 commented 2 years ago

@audrow @gaoethan I fixed python test in https://github.com/ros-perception/vision_opencv/pull/444. I appreciate it if you could review it, too.

audrow commented 2 years ago

@ros-pull-request-builder retest this please

wep21 commented 2 years ago

@audrow Is it better to create a new branch for foxy or galactic?

ijnek commented 2 years ago

@audrow Is it better to create a new branch for foxy or galactic?

Related: #393 foxy and galactic branches?

wep21 commented 2 years ago

@audrow friendly ping

ijnek commented 2 years ago

I've gone ahead and split off foxy, galactic and humble branches, and once https://github.com/ros/rosdistro/pull/34314, CI should only run on rolling.

@audrow Since you've said you're fine with the changes in a previous comment, I'm going to proceed with merging the PR in (but not backport to humble since its a new feature). Over in demos/image_tools, a PR can be reaised to use the class from cv_bridge instead. If you're fine with that, could you simply leave a reaction? Otherwise, we can discuss.

audrow commented 2 years ago

@audrow Since you've said you're fine with the changes in a previous comment, I'm going to proceed with merging the PR in (but not backport to humble since its a new feature). Over in demos/image_tools, a PR can be reaised to use the class from cv_bridge instead. If you're fine with that, could you simply leave a reaction? Otherwise, we can discuss.

That all sounds good to me - thanks, @ijnek!

I've gone ahead and split off foxy, galactic and humble branches, and once https://github.com/ros/rosdistro/pull/34314, CI should only run on rolling.

By the way, if you want to be more consistent with most of the ROS 2 core repositories, you can change the ros2 branch to rolling.

ijnek commented 2 years ago

By the way, if you want to be more consistent with most of the ROS 2 core repositories, you can change the ros2 branch to rolling.

Yep, that's the plan. I'm not sure what the unintended consequences are of renaming the branch are, so I'm going to be careful about it. You'd probably know more about this since you did the changes for the core repositories.

ijnek commented 2 years ago

@ros-pull-request-builder retest this please

ijnek commented 2 years ago

Branches have been updated, so only the output of Rolling CI is important here. Since it's passing, I'll merge.

ijnek commented 2 years ago

Thanks for the contribution @wep21 and reviewing @audrow !

ijnek commented 2 years ago

Over in demos/image_tools, a PR can be raised to use the class from cv_bridge instead.

Done: https://github.com/ros2/demos/issues/583

ZhenshengLee commented 1 year ago

Any plan to merge this pr to humble branch?

ijnek commented 1 year ago

@ZhenshengLee feature additions won't be backported to already released distributions, only bug fixes will be backported.