Support image transport publishers in nodelet lazy

[efernandez]

[efernandez]

FYI @mikaelarguedas @dirk-thomas

[mikaelarguedas]

Thanks @efernandez I'm a bit hesitant about introducing a dependency on image_transport in nodelet (not sure how @dirk-thomas feels about it ?).

I will try to go over this PR soon and give feedback.

As this breaks API / ABI, it will need to be released in Melodic or ROS-N but not in the already released distributions. At which point it may make sense to use c++11/14 features rather extending our use of boost.

[efernandez]

@mikaelarguedas I can give it another thought to use some template magic and avoid breaking API / ABI, but at first sight looks hard to me, or at least it'd be too verbose. And after all, I'm happy if this gets into Melodic. I targeted indigo-devel branch here b/c it looks to me you're using that branch for >= Indigo. If you create melodic-devel I'd target it with a new PR and close this one.

Re. C++11/14, I'd be happy to contribute another PR with that, but I wouldn't do it in this one.

Re. image_transport dependency, I tried to inherit from this class and extend it for Image Transport publishers, but then I need another callback because the current one is taking a ros::Publisher that doesn't use and is also hidden by a local variable. And I think even in that case I need additional changes to avoid duplicating all the code in the callback to include the Image Transport publishers (in particular https://github.com/ros/nodelet_core/blob/indigo-devel/nodelet_topic_tools/include/nodelet_topic_tools/nodelet_lazy.h#L168, so it's only checked after checking there's no subscriber for all publishers, of both types). And I still need to make the ros::Publisher templated advertise method use the overridden callback from the child class, which means it should be virtual here in the base class. IMHO unless there's a strong reason to avoid the dependency on image_transport, I think this would be cleaner. Either way, I'm open to suggestions for an alternative implementation, like splitting the current callback for dis/connect.

[dirk-thomas]

unless there's a strong reason to avoid the dependency on image_transport, I think this would be cleaner

I haven't looked too much into the details of the API and this patch. But in my opinion the question is not if there is "a strong reason to avoid the dependency". The question is why should nodelet_topic_tools contains image_transport specific API? image_transport is just one custom thing - the same would potentially apply to others. I would argue that the API should be designed that it is possible to do what you are aiming for outside of this package. So image transport should be able to be implemented ontop rather then require to be integrated into nodelets.

If certain information is currently not exposed or additional API is needed that can certainly be changed / added in nodelet to accommodate the requirements.

[efernandez]

Thanks @dirk-thomas . I'll try to see how far I can go, b/c the current implementation is specific for ros::Publisher and ros::NodeHandle.

@mikaelarguedas Are you ok with me doing changes to this PR so it allows to extend NodeletLazy to support different types of publishers, and not only ros::Publisher?

I've just sent another commit the makes the connectCallback publisher-agnostic. It adds this helper templated method that would check all publishers before doing any action into the state of the object: https://github.com/ros/nodelet_core/pull/75/files#diff-dd9335fec549f588f8a69073e5e1f970R264 The idea would be to allow to overload this method or another one that calls it, from the children classes. But well, I still need to spend more time on this.

Do you mind pointing to things you'll be willing to accept from my changes as a separate PR, meanwhile?

[efernandez]

@mikaelarguedas @dirk-thomas I've updated the PR so now it doesn't depend on image_transport.

[efernandez]

I think the test for bionic is failing on something unrelated with this PR:

14:46:29 Traceback (most recent call last):
14:46:29   File "/usr/lib/python3/dist-packages/apt/cache.py", line 218, in __getitem__
14:46:29     return self._weakref[key]
14:46:29   File "/usr/lib/python3.6/weakref.py", line 137, in __getitem__
14:46:29     o = self.data[key]()
14:46:29 KeyError: 'ros-melodic-pluginlib'

If you don't consider the change on connectionCallback could impact existing code, it'd be great if we could get this in lunar, not only in melodic. It seems to me that only the NodeletLazy uses that method.

[efernandez]

@mikaelarguedas What do you think of my latest changes?

I've just rebased on top of indigo-devel so the test is kicked again (no new changes).

[efernandez]

ping