Feat/Add LifecycleNode Support

[elsayedelsheikh]

Closes (#107)
In progress ...

[ahcorde]

Two main ideas:

• Don't apply style modifications because linter will complain
• Keep old methods and deprecated them

[elsayedelsheikh]

@DasariChaitanya
All unit tests have passed.
I've also tested the plugins (raw, draco, etc.), and everything is working fine. However, I'm unsure whether we might encounter issues with sub-namespaces in the future without get_effective_namespace().

Could you review the latest changes, along with the overall architecture? Additionally, please check which methods should be marked with the [[deprecated]] attribute and whether any visibility control macros are missing.

Edit:
Also, consider adding additional methods for LifecycleNode calls, such as on_activate(), etc.

[elsayedelsheikh]

Should we deprecate the old methods and enforce using NodeInterfaces, /or turn them into templates would be better to preserve the existing API?

Option 1: Direct NodeInterfaces

pct = std::make_shared<point_cloud_transport::PointCloudTransport>(
  point_cloud_transport::create_node_interfaces(this)
);

std::shared_ptr<point_cloud_transport::PublisherPlugin> pub;
pub->advertise(
  point_cloud_transport::create_node_interfaces(this),
  out_topic, rmw_qos_profile_default, pub_options);

Option 2: Templates

pct = std::make_shared<point_cloud_transport::PointCloudTransport>(this->shared_from_this());

std::shared_ptr<point_cloud_transport::PublisherPlugin> pub;
pub->advertise(
  this->shared_from_this(),
  out_topic, rmw_qos_profile_default, pub_options);

CC @ahcorde

[elsayedelsheikh]

@ahcorde
Could you please review this and LMK if there's anything I need to do?
Thanks :)

[ahcorde]

sorry the late response, I was taking a look to this suggestion made by @methylDragon in geometry2 ros2/geometry2#698
and I think we should use rclcpp::node_interfaces::NodeInterfaces instead of point_cloud_transport::NodeInterfaces

[DasariChaitanya]

@ahcorde
Could you please review this PR?

[ahcorde]

@DasariChaitanya point_cloud_transport_plugins are not compiling

point_cloud_transport/simple_publisher_plugin.hpp:152:23: error: ‘struct point_cloud_transport::SimplePublisherPlugin<point_cloud_interfaces::msg::CompressedPointCloud2_<std::allocator<void> > >::SimplePublisherPluginImpl’ has no member named ‘node_interface_’; did you mean ‘node_interfaces_’?
  152 |         simple_impl_->node_interface_->get_node_parameters_interface()
      |         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
      |         node_interfaces_

[elsayedelsheikh]

Sorry for late response

@DasariChaitanya point_cloud_transport_plugins are not compiling

Fixed the variable name and tested the plugins, everything is working 👍

[elsayedelsheikh]

@ahcorde
Could you please check this PR?

[ahcorde]

This comment is still open

• Keep old methods and deprecated them

Still need to deprecate the rclcpp::Node methods

[DasariChaitanya]

@ahcorde
Do I need to deprecate the methods that have been templatized with a default to rclcpp::Node? The usage and API remain unchanged for users, so I wanted to confirm if formal deprecation is required in this case.

[elsayedelsheikh]

@DasariChaitanya
Check this ros2/geometry2#698 which recommends avoiding templates in favor of rclcpp::node_interfaces::NodeInterfaces to reduce recompilation, improve clarity, and promote reuse.

sorry the late response, I was taking a look to this suggestion made by @methylDragon in geometry2 ros2/geometry2#698 and I think we should use rclcpp::node_interfaces::NodeInterfaces instead of point_cloud_transport::NodeInterfaces

I'll start working on it removing templates and deprecating rclcpp::Node methods

[elsayedelsheikh]

This comment is still open

• Keep old methods and deprecated them

Still need to deprecate the rclcpp::Node methods

We can close this comment now :)
@ahcorde

[ahcorde]

there are many warning that I can see in the tests. Do you mind to create:

• Add new test for the new APIs
• silent current warnings which will be removed in the next release

[ahcorde]

you can use -Wdeprecated-declarations inside the tests file to avoid the warning message:

#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable : 4996)
#else
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#endif

// deprecated code 

#ifdef _MSC_VER
#pragma warning(pop)
#else
#pragma GCC diagnostic pop
#endif

[elsayedelsheikh]

Two minor changes, otherwise LGTM

Perfect! 💯

[ahcorde]

Pulls: #109
Gist: https://gist.githubusercontent.com/ahcorde/10ae278905fe15414ef02b5a4c9de2f6/raw/f707b68267eb0bd419d3f1f0d940cabca531292c/ros2.repos
BUILD args: --packages-above-and-dependencies point_cloud_transport point_cloud_transport_py
TEST args: --packages-above point_cloud_transport point_cloud_transport_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16108

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

Windows is failing

republish.obj : error LNK2019: unresolved external symbol "const point_cloud_transport::PointCloudTransport::`vftable'" (??_7PointCloudTransport@point_cloud_transport@@6B@) referenced in function "public: __cdecl point_cloud_transport::PointCloudTransport::PointCloudTransport(class std::shared_ptr<class rclcpp::Node>)" (??0PointCloudTransport@point_cloud_transport@@QEAA@V?$shared_ptr@VNode@rclcpp@@@std@@@Z) [C:\ci\ws\build\point_cloud_transport\pc_republish_node.vcxproj]

[elsayedelsheikh]

Windows is failing

republish.obj : error LNK2019: unresolved external symbol "const point_cloud_transport::PointCloudTransport::`vftable'" (??_7PointCloudTransport@point_cloud_transport@@6B@) referenced in function "public: __cdecl point_cloud_transport::PointCloudTransport::PointCloudTransport(class std::shared_ptr<class rclcpp::Node>)" (??0PointCloudTransport@point_cloud_transport@@QEAA@V?$shared_ptr@VNode@rclcpp@@@std@@@Z) [C:\ci\ws\build\point_cloud_transport\pc_republish_node.vcxproj]

Is there any GitHub workflow that runs on Windows to test before pushing to the PR?

[ahcorde]

These two changes should fix the issue

[ahcorde]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[elsayedelsheikh]

@ahcorde
All builds passing 🥇 💯

[ahcorde]

Awesome! Thank you for iterating!