search

ros-perception / image_transport_plugins

A set of plugins for publishing and subscribing to sensor_msgs/Image topics in representations other than raw pixel data.
BSD 3-Clause "New" or "Revised" License
55 stars 120 forks source link

[ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher #145

Closed bmegli closed 1 year ago

bmegli commented 1 year ago

140 fix for CompressedDepthPublisher

108 like (runtime reconfigurable)

Builds up on top of #110 so if merged that one should also be closed.

Implementation is #143 like.

png_level default

I left default png_level the same as for compressed_image_transport.

Both plugins use png_level and clash on deprecated parameter name as mentioned in #140 and we don't want to consider race between plugin loaders (e.g. which default value wins).

We can set it again to something different once deprecated parameters are removed.

code repetition

There is some code repetition between:

This should be temporary until deprecated parameters are removed.

bmegli commented 1 year ago

There is one more thing that your review made me aware of.

As:

Setting deprecated png_level will be reflected both in values for compressed and compressedDepth

But maybe that is expected.

We will get 2x warnings from both compressed and compressedDepth that their values were set from deprecated attribute.

ijnek commented 1 year ago

There is one more thing that your review made me aware of.

As:

  • both compressed and compressedDepth declare <some_path>.<image>.png_level

    • clash on this parameter
  • and we synchronize back deprecated value to transport scoped

Setting deprecated png_level will be reflected both in values for compressed and compressedDepth

But maybe that is expected.

We will get 2x warnings from both compressed and compressedDepth that their values were set from deprecated attribute.

The "deprecated" code you have in this PR seems to reflect this current strange behavior correctly, and will allow those that depend on this behavior to gracefully take on the new API).

How about we be explicit about this png_level parameter, in both depth and compressedDepth. We could change: parameter xxx is deprecated, use transport qualified name aaa.xxx)

to the following if (the name of the topic is "png_level"): parameter xxx is deprecated and ambiguous, use transport qualified name, either compressed.xxx or compressedDetph.xxx.

In regards to getting 2x warnings, this is not a problem as there are actually two plugins complaining.

bmegli commented 1 year ago

How about we be explicit about this png_level parameter, in both depth and compressedDepth. We could change: parameter xxx is deprecated, use transport qualified name aaa.xxx)

to the following if (the name of the topic is "png_level"): parameter xxx is deprecated and ambiguous, use transport qualified name, either compressed.xxx or compressedDetph.xxx.

In regards to getting 2x warnings, this is not a problem as there are actually two plugins complaining.

A reasonable compromise is

parameter xxx is deprecated and ambiguous, use transport qualified name <transport>.xxx

Then we will get from plugins

Without hardcoding any transport names in warning messeges.

ijnek commented 1 year ago

That's a good compromise. Just one final thing, and we're good to go I think

ijnek commented 1 year ago

@Mergifyio backport iron

mergify[bot] commented 1 year ago

backport iron

✅ Backports have been created

* [#148 [ROS2] reconfigurable transport scoped parameters for CompressedDepthPublisher (backport #145)](https://github.com/ros-perception/image_transport_plugins/pull/148) has been created for branch `iron`