ros-perception / image_common

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

[ROS2] image_transport does not support LifecycleNode #108

Open Myzhar opened 5 years ago

Myzhar commented 5 years ago

Hi, I'm trying to integrate the new image_transport from Crystal in my LifecycleNode, but the functions image_transport::create_camera_publisher and image_transport::create_camera_publisher only take rclcpp::Node* node as parameter.

Is there any workaround?

mjcarroll commented 5 years ago

Not currently, I believe that this would require some further work to integrate the LifecycleNode.

Myzhar commented 5 years ago

I think that using a rclcpp::node_interfaces::NodeBaseInterface::SharedPtr instead of a rclcpp::Node* is the right way.

There is a similar problem with the TF Broadcaster

The rclcpp::executor::Executor uses the Base Interface: https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/executor.hpp#L124

marting87 commented 4 years ago

Any updates on this one?

Michael-Equi commented 4 years ago

Any progress on this? It looks like the above issue mention on this has been partially resolved as I am now able to do std::make_shared<rclcpp::AsyncParametersClient>( this->get_node_base_interface(),this->get_node_topics_interface(), this->get_node_graph_interface(), this->get_node_services_interface()); on Ubuntu 20.04 Foxy which is a bit more verbose than would be desired but gets the job done. Would something like that work here?

eeberhard commented 1 year ago

This issue is increasingly relevant as managed lifecycle nodes are used more widely. In my opinion it makes a lot of sense for an image processing node to have lifecycle states, so that we can configure it with image parameters, and then activate / deactivate potentially expensive processing and perception steps through the lifecycle interfaces.

More related issues like the ones above will keep arising, and the workarounds are generally less efficient (such as giving up on the image transport layer and just using raw pub / sub of sensor_msg Image instead).

Thanks @Rayman for the initiative to solve this. I believe #190 is a great demonstration of the concept of using base interfaces rather than a Node instance to allow for lifecycle nodes and really any other node-like classes. It sets the precedent for the slightly larger #167 (but which ultimately follows the same concept).

Is there anything that can be done to encourage these PRs and this issue to be closed?

Rayman commented 1 year ago

I don't know why it takes so long to merge these PRs. It's a relatively simple change. There should be more activity in such a core ros package.

gbiggs commented 1 year ago

190 fell off my todo list for some reason. I've started it moving forward again.

167 cannot be merged as it is still a draft PR. Until the author does the necessary work to finish the PR, it will remain that way.

ilidar commented 1 year ago

@gbiggs created another draft PR https://github.com/ros-perception/image_common/pull/258

SamerKhshiboun commented 1 year ago

Any update on this ? we want to use lifecycle nodes in our package, but still blocked by image_transport component.

SamerKhshiboun commented 1 year ago

Any progress on this? It looks like the above issue mention on this has been partially resolved as I am now able to do std::make_shared<rclcpp::AsyncParametersClient>( this->get_node_base_interface(),this->get_node_topics_interface(), this->get_node_graph_interface(), this->get_node_services_interface()); on Ubuntu 20.04 Foxy which is a bit more verbose than would be desired but gets the job done. Would something like that work here?

@Michael-Equi , Can you advice on how to call image_transport::create_publisher with lifecycle node ? Any conversion that I can do to the lifecycle node to get a raw node from it ?

Thanks

Benjamin-Tan commented 8 months ago

Any update on this? Is this package still being maintained/developed?

mjcarroll commented 8 months ago

Any update on this? Is this package still being maintained/developed?

No update that I'm aware of. Generally, I think maintenance is limited to bugfixes and releases. We are always happy to take/review contributions, but I believe that the PR in question here is still in draft?