ros-perception / image_common

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

Remove subscriber and plugin without options #252

Closed ijnek closed 2 years ago

ijnek commented 2 years ago

An unexpected side effect of #249 breaks behaviours in classes downstream that inherit simple_subscriber_plugin.hpp. Removing the deprecated methods and causing compile errors downstream would be preferred to ensure smooth transition.

Resolves this issue (_Originally posted by @ivanpauno in https://github.com/ros-perception/image_common/issues/249#issuecomment-1204485423_):

That's not intended, and if that's happening then that'll defnitely be a bug. It should be showing a warning and still behaving correctly.

CompressedPublisher::advertiseImpl() (with options) is not being called anymore if you're holding a pointer to base. i.e.:

PublisherPlugin * plugin_;
... // plugin is a CompressedPublisher instance
plugin_->advertiseImpl(node, topic, qos, options);

is calling this

https://github.com/ijnek/image_common/blob/74510a9ba6825dd0ddcbdd5a04ff9d34bad83da7/image_transport/include/image_transport/simple_publisher_plugin.hpp#L101

instead.

That's because the new method hasn't been overriden (only the old one without options), and SimplePublisherPlugin provides an implementation. Because of that, the parameters are not being declared and the plugin is being configured incorrectly.

ijnek commented 2 years ago

@ivanpauno It is unfortunate that there seemed to be no easy way to do a nice tik-tok deprecation, but do these changes ensure smoother transition?

ivanpauno commented 2 years ago

@ivanpauno It is unfortunate that there seemed to be no easy way to do a nice tik-tok deprecation, but do these changes ensure smoother transition?

I think so, thanks for the PR!

gbiggs commented 2 years ago

CI:

ivanpauno commented 2 years ago

@gbiggs could you release the repo to rolling to get the new changes in? Thanks!

gbiggs commented 2 years ago

Release done!