search

ros-perception / image_common

Common code for working with images in ROS
http://www.ros.org/wiki/image_common
129 stars 223 forks source link

Fix SimpleSubscriberPlugin #195

Closed ivanpauno closed 3 years ago

ivanpauno commented 3 years ago

That was done in https://github.com/ros-perception/image_common/pull/186. To be more clear, the issue is that external plugins are usually inheriting from SimpleSubscriberPlugin and not from SubscriberPlugin. e.g. this method was not called anymore after that patch, resulting in the compressed image transport subscriber plugin not being correctly initialized.

ivanpauno commented 3 years ago

@jacobperron @audrow could you review?

ivanpauno commented 3 years ago

That one is fun, I cannot understand how this code was working joy.

Now that I see the diff, I deleted a line without realizing and then added it back :joy:. Updated OP as that was not an issue.

ivanpauno commented 3 years ago

Now that I see the diff, I deleted a line without realizing and then added it back joy.

Reverted the accidental changes in https://github.com/ros-perception/image_common/pull/195/commits/a3d3d31f0ef57d37d2e2a3e9ef20e77efb09c404.

ivanpauno commented 3 years ago

I realized this is neither ideal unfortunately, see https://github.com/ros-perception/image_common/pull/195/commits/8e4fc83641ec01d240b3494b229923b1ef20d116.

We should either do this, or delete the subscribeImpl() method not taking a rclcpp::SubscriptionOptions argument and break API. I don't see a better alternative ...

jacobperron commented 3 years ago

ugh, yeah this use of inheritance is not very maintainable. Maybe we go forward with this workaround for Galactic and then break the API for Rolling by removing the subscribeImpl without the SubscriptionOptions (or changing the design more dramatically).

ivanpauno commented 3 years ago

@mjcarroll could you review?

chapulina commented 3 years ago

This introduced a clang warning, fix in https://github.com/ros-perception/image_common/pull/196.