search

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

[ADD] std::optional for PublisherOptions #243

Closed wodtko closed 2 years ago

wodtko commented 2 years ago

fixing https://github.com/ros-perception/image_common/issues/240 this PR uses std::optionals in order to allow advertising publishers without specifying PublisherOptions. By this, the error message 

PublisherPlugin::advertiseImpl with four arguments has not been overridden

is avoided. However, if PublisherOptions are specified, it still occurs.

gbiggs commented 2 years ago

Is this PR still valid?

ijnek commented 2 years ago

Does the same issue exist for subscriptions? I.e. should we make a symmetric change to create_subscription and friends?

I can confirm the issue exists for subscriptions that don't implement subscribeImpl with five arguments (eg. theora).

[ERROR] [1658131036.817718727] [image_republisher]: SubscriberPlugin::subscribeImpl with five arguments has not been overridden
ijnek commented 2 years ago

I don't know if I 100% agree with the changes in this PR. The changes would result in the transports not adhering to the specified PublisherOptions/SubscriptionOptions without printing a warning (or error as it currently is).

For example, if there are three transport_plugins (ie.raw, compressed, theora) where only theora doesn't have the method with the extra argument implemented.

The current behaviour prints an error for theora if the user requests something through Options, but with the changes here, it would silently work without logging any warning/error.

I feel that we need to encourage all image transports to handle the options correctly in the first place.

gbiggs commented 2 years ago

@ijnek Just so I'm clear, your position is that we reject this PR and instead have all image transports correctly handle the passed-in options?

ijnek commented 2 years ago

@gbiggs Correct, I've raised a PR with my suggestions: https://github.com/ros-perception/image_common/pull/249

gbiggs commented 2 years ago

After some thought and discussion with colleagues, I think the best approach is to document that some of the functions (the ones without options) are deprecated, rather than adding std::optional to everything. Adding std::optional makes the API harder to use correctly.