Update subscriber filter by elsayedelsheikh · Pull Request #126 · ros-perception/point_cloud_transport

elsayedelsheikh · 2025-06-26T19:51:01Z

Related to #124

Work Description

Initially, New FullSubscriberFilter class inherited from message_filters::SubscriberBase and message_filters::SimpleFilter<sensor_msgs::msg::PointCloud2> then I thought why not inherit from existing implementation SubscriberFilter and avoid repeatition.
So I've changed a few things in the original SubscriberFilter and voila new FullSubscriberFilter
I'm also suggesting renaming classes to change the old SubscriberFilter name to SimpleSubscriberFilter and the new one from FullSubscriberFilter to SubscriberFilter

About inheriting from SubscriberBase , We need to override these 2 functions but our subscriber needs more node_interfaces so I left them empty and shouldn't be used

  void subscribe(
    RequiredInterfaces /*node_interfaces*/, const std::string & /*topic*/,
    const rclcpp::QoS & /*qos*/) override
  {}
  void subscribe(
    RequiredInterfaces  /*node_interfaces*/,
    const std::string & /*topic*/,
    const rclcpp::QoS & /*qos*/,
    rclcpp::SubscriptionOptions /*options*/) override
  {}

Edit:

Updated the existing SubscriberFilter:

Inherits from message_filters::SubscriberBase as well as message_filters::SimpleFilter
New constructor that accepts Node or/ LifecycleNode
New subscriber(...) that accepts Node or/ LifecycleNode
New subscibe() method to re-subscribe if this subscriber has previously been initialized.

Example Usage

// During initialization
auto sub = std::make_shared<point_cloud_transport::SubscriberFilter>(
  node, topic, transport_type, custom_qos, sub_opt);
/*or*/
auto sub = std::make_shared<point_cloud_transport::SubscriberFilter>();
sub->subscribe(node, topic, transport_type, custom_qos, sub_opt);

sub->unsubscribe();  // Arguments still stored internally

// On activate
if (sub != nullptr) {
  sub->subscribe();  // Reuses stored arguments to re-create a subscriber
}

// On deactivate (already implemented)
sub->unsubscribe();

FYI @ahcorde @SteveMacenski

SteveMacenski · 2025-06-26T20:25:27Z

I think this should modify the existing subscriber filter, not add a new one, no?

elsayedelsheikh · 2025-06-26T20:28:57Z

I think this should modify the existing subscriber filter, not add a new one, no?

It adds a new one with slight changes on the old one:

change access modifier on the callback function from private to protected
change subscribe(node_interfaces, ...) method to be virtual so I can override to save passed args

elsayedelsheikh · 2025-06-26T20:31:40Z

Should we rid of the existing subscriber filter? 🤔

SteveMacenski · 2025-06-26T21:02:25Z

I think this should be applied to the existing filter so we only have a diff of the changes needed rather than a whole new implementation :-) Those changes seem pretty minimal to me to not just update the existing implementation

elsayedelsheikh · 2025-06-26T21:49:54Z

I think this should be applied to the existing filter so we only have a diff of the changes needed rather than a whole new implementation :-) Those changes seem pretty minimal to me to not just update the existing implementation

Got it! :)

elsayedelsheikh · 2025-06-26T22:37:33Z

I think this should be applied to the existing filter so we only have a diff of the changes needed rather than a whole new implementation :-) Those changes seem pretty minimal to me to not just update the existing implementation

Done!

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh · 2025-06-27T11:33:15Z

We're ready for review here :) @ahcorde

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

SteveMacenski · 2025-06-27T17:39:31Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp

+
+  template<typename NodeT = rclcpp::Node::SharedPtr>
+  POINT_CLOUD_TRANSPORT_PUBLIC
+  SubscriberFilter(


I think this could remove the previous 2x constructors in this class. The NodeT will be deduced automatically since its a direct constructor argument, so you both don't actually need to specify a default and it'll automatically deduce it at compile time :-)

Also, we should have some default for the QoS - what's the default used if not specified by the base class?

Added a default QoS and removed the old constructor.
As for the node_interfaces constructor, I think we should keep this one preserving the general style followed in point_cloud_transport, no?

is there any chance to deprecate old constructors?

I think rclcpp::Node constructor would work here so there's no need to deprecate it, right? -> Should remove it
The one that should be deprecated is the node_interfaces constructor if you're ok with it😀

SteveMacenski · 2025-06-27T17:39:56Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp

+  template<typename NodeT = rclcpp::Node::SharedPtr>
+  POINT_CLOUD_TRANSPORT_PUBLIC
+  void subscribe(
+    NodeT node,


Ditto, now that its templated the previous method for rclcpp node is not required - rclcpp::Node's will just automatically work here

SteveMacenski · 2025-06-27T17:40:14Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp


 private:
+  //! Don't use this method
+  void subscribe(


Then remove if it shouldn't be used? 😆

About inheriting from SubscriberBase , We need to override these 2 functions but our subscriber needs more node_interfaces so I left them empty and shouldn't be used
void subscribe( RequiredInterfaces /*node_interfaces*/, const std::string & /*topic*/, const rclcpp::QoS & /*qos*/) override {} void subscribe( RequiredInterfaces /*node_interfaces*/, const std::string & /*topic*/, const rclcpp::QoS & /*qos*/, rclcpp::SubscriptionOptions /*options*/) override {}

Won't be able to create a class object if not implemented, Check this
So that I put them under private so no one uses them :D

Ugh. I think that should be reverted too with the subscribe function similarly templated by NodeT that can be auto-deduced. Before I think it was a pain because the entire class was templated rather than just the method that can autodeduce the type from the arguments.

@ahcorde what do you think?

Exposing the RequiredInterfaces to the application code is super nasty and can be populated internally to the message filter from the argument provided NodeT. I think merging this in March was not the best solution ros2/message_filters#113. Using RequiredInterfaces as an input if the method is templated will still work as a NodeT as long as it provides the same get_X_base_interface() method. That appears to be the case https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/node_interfaces/node_interfaces.hpp#L108-L110

This is very nice documented issue ros2/geometry2#698 why we shouldn't use templates and why we should use rclcpp::node_interfaces::NodeInterfaces

There is a ppt too https://docs.google.com/presentation/d/1bdXOOZPhR9yAnyGNxoLhuO_bU4RW5IjnzvCtrVlpe_g/edit?slide=id.g24afec4abf4_0_0#slide=id.g24afec4abf4_0_0

Maybe we can still improve something in message filter, but I prefer rclcpp::node_interfaces::NodeInterfaces instead of NodeT

Would we need to update message filters too?

I think I'm lost here... 🤔
So you guys are proposing to use f(*NodeT) for implicit conversion to node_interfaces, similar to
https://github.com/ros2/message_filters/blob/4f17c17813e7b84e34cc287143ddf70471c78d6c/include/message_filters/subscriber.hpp#L168-L173

Then, Would it be ok to leave SubscriberBase pure virtual methods under private without implementation { }?

Let me create the PR with my suggestions, and we can move forward from there

Here my suggestions #129 @SteveMacenski @elsayedelsheikh

That looks good to me! I think after rebasing onto that, whatever changes are still required for aligning with message filters are still related but simplifies things quite a bit in the best ways.

Just to check, all this works with a rclcpp_lifecycle::LifecycleNode or even a nav2::LifecycleNode in auto-populating that interfaces objcet, correct? All the docs make me think that's the case, but just verifying since I still see the specialized rclcpp::Node constructors in your PR. Couldn't those be removed?

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh · 2025-06-28T18:57:08Z

@SteveMacenski Hey Steve, any update?

ahcorde · 2025-07-01T10:09:17Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp

+
+  template<typename NodeT = rclcpp::Node::SharedPtr>
+  POINT_CLOUD_TRANSPORT_PUBLIC
+  SubscriberFilter(


is there any chance to deprecate old constructors?

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh · 2025-07-01T17:49:34Z

Please check the current implementation!
And LMK if there are any final notes🥲

SteveMacenski · 2025-07-01T18:36:11Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp

+  /// \param options Subscriber options
+  ///
  POINT_CLOUD_TRANSPORT_PUBLIC
  void subscribe(


Can this be depreciated?

SteveMacenski · 2025-07-01T18:39:25Z

point_cloud_transport/include/point_cloud_transport/subscriber_filter.hpp


 private:
+  //! Don't use this method
+  void subscribe(


Ugh. I think that should be reverted too with the subscribe function similarly templated by NodeT that can be auto-deduced. Before I think it was a pain because the entire class was templated rather than just the method that can autodeduce the type from the arguments.

@ahcorde what do you think?

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

ahcorde

Do you mind to fix the conflicts ?

elsayedelsheikh · 2025-07-17T09:49:00Z

Do you mind to fix the conflicts ?

Working on it 😁

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>

ahcorde · 2025-07-18T08:07:02Z

Pulls: #126
Gist: https://gist.githubusercontent.com/ahcorde/885d35438a0c1b0fe7da399ea6aebfe9/raw/18be47193ab26936a191b9e8710ee84e8c6f40cf/ros2.repos
BUILD args: --packages-above-and-dependencies point_cloud_transport
TEST args: --packages-above point_cloud_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16529

Linux
Linux-aarch64
Linux-rhel
Windows

elsayedelsheikh · 2025-07-18T09:59:04Z

@ahcorde I'd like to add one more thing before we merge

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh · 2025-07-18T10:16:14Z

Modified the Class Constructor
Throw run time exception for NodeInterfaces<NodeParametersInterface, NodeTopicsInterface> functions

ahcorde · 2025-07-18T20:13:15Z

Linux
Linux-aarch64
Linux-rhel
Windows

elsayedelsheikh mentioned this pull request Jun 26, 2025

API Proposed changes - Nice to have #124

Closed

6 tasks

elsayedelsheikh changed the title ~~New subscriber filter~~ Update subscriber filter Jun 26, 2025

elsayedelsheikh added 5 commits June 27, 2025 14:31

initial thoughts

9a396ae

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

Make useful for derived classes

c520324

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

New subscriberfilter class

613d980

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

make copyright test happy

b77e5e1

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

update subscriber_filter

da90eff

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh force-pushed the advanced_subscriber_filter branch from 0762c9b to da90eff Compare June 27, 2025 11:31

elsayedelsheikh added 2 commits June 27, 2025 14:39

Hide methods from users

dc5077d

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

Make linters happy

34431b3

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

SteveMacenski suggested changes Jun 27, 2025

View reviewed changes

Feedback

e4ab912

Signed-off-by: elsayedelsheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh requested a review from SteveMacenski June 28, 2025 01:11

ahcorde assigned elsayedelsheikh Jul 1, 2025

ahcorde requested changes Jul 1, 2025

View reviewed changes

Refactor

1eacb27

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh requested a review from ahcorde July 1, 2025 17:49

SteveMacenski reviewed Jul 1, 2025

View reviewed changes

No more node_interfaces

8c4ef81

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

ahcorde requested changes Jul 17, 2025

View reviewed changes

Fix conflicts

0a0372f

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh requested review from SteveMacenski and ahcorde July 17, 2025 20:45

Added missing include

a847096

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>

ahcorde approved these changes Jul 18, 2025

View reviewed changes

final touch

02b9490

Signed-off-by: ElSayed ElSheikh <elsayed.elsheikh97@gmail.com>

elsayedelsheikh requested a review from ahcorde July 18, 2025 10:16

ahcorde approved these changes Jul 18, 2025

View reviewed changes

ahcorde merged commit 4d502a3 into ros-perception:rolling Jul 21, 2025
3 checks passed

elsayedelsheikh deleted the advanced_subscriber_filter branch July 21, 2025 13:27

Conversation

elsayedelsheikh commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Description

Edit:

Example Usage

Uh oh!

SteveMacenski commented Jun 26, 2025

Uh oh!

elsayedelsheikh commented Jun 26, 2025

Uh oh!

elsayedelsheikh commented Jun 26, 2025

Uh oh!

SteveMacenski commented Jun 26, 2025

Uh oh!

elsayedelsheikh commented Jun 26, 2025

Uh oh!

elsayedelsheikh commented Jun 26, 2025

Uh oh!

elsayedelsheikh commented Jun 27, 2025

Uh oh!

SteveMacenski Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsayedelsheikh Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsayedelsheikh Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsayedelsheikh commented Jun 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsayedelsheikh commented Jul 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahcorde left a comment

Choose a reason for hiding this comment

Uh oh!

elsayedelsheikh commented Jul 17, 2025

Uh oh!

elsayedelsheikh commented Jun 26, 2025 •

edited

Loading

SteveMacenski Jun 27, 2025 •

edited

Loading

elsayedelsheikh Jul 1, 2025 •

edited

Loading

SteveMacenski Jun 27, 2025 •

edited

Loading

SteveMacenski Jun 27, 2025 •

edited

Loading

elsayedelsheikh Jun 28, 2025 •

edited

Loading

SteveMacenski Jul 1, 2025 •

edited

Loading