Skip to content

Fix SimpleSubscriberPlugin#195

Merged
mjcarroll merged 3 commits into
ros-perception:ros2from
ivanpauno:ivanpauno/fix-simple-subscriber-plugin
Jun 29, 2021
Merged

Fix SimpleSubscriberPlugin#195
mjcarroll merged 3 commits into
ros-perception:ros2from
ivanpauno:ivanpauno/fix-simple-subscriber-plugin

Conversation

@ivanpauno
Copy link
Copy Markdown
Contributor

@ivanpauno ivanpauno commented Jun 24, 2021

  • Do not override the subscribeImpl method taking a rclcpp::SubscriptionOptions object.
    Plugins that were not overriding it before won't notice that they have to be update.

That was done in #186.
To be more clear, the issue is that external plugins are usually inheriting from SimpleSubscriberPlugin and not from SubscriberPlugin.
e.g. this method was not called anymore after that patch, resulting in the compressed image transport subscriber plugin not being correctly initialized.

…aking a rclcpp::SubscriptionOptions object.

  Plugins that were not overriding it before won't notice that they have to be update.

* Allocate a SimpleSubscriberPlugin::Impl on construction.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Contributor Author

@jacobperron @audrow could you review?

@ivanpauno
Copy link
Copy Markdown
Contributor Author

That one is fun, I cannot understand how this code was working joy.

Now that I see the diff, I deleted a line without realizing and then added it back 😂.
Updated OP as that was not an issue.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Contributor Author

Now that I see the diff, I deleted a line without realizing and then added it back joy.

Reverted the accidental changes in a3d3d31.

…gin.

* Override subscribeImpl() method taking a rclcpp::SubscriptionOptions in all classes inheriting from SimpleSubscriberPlugin.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Copy Markdown
Contributor Author

I realized this is neither ideal unfortunately, see 8e4fc83.

We should either do this, or delete the subscribeImpl() method not taking a rclcpp::SubscriptionOptions argument and break API.
I don't see a better alternative ...

@jacobperron
Copy link
Copy Markdown
Contributor

ugh, yeah this use of inheritance is not very maintainable. Maybe we go forward with this workaround for Galactic and then break the API for Rolling by removing the subscribeImpl without the SubscriptionOptions (or changing the design more dramatically).

Copy link
Copy Markdown

@audrow audrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ivanpauno
Copy link
Copy Markdown
Contributor Author

@mjcarroll could you review?

@chapulina
Copy link
Copy Markdown
Contributor

This introduced a clang warning, fix in #196.

@ivanpauno ivanpauno deleted the ivanpauno/fix-simple-subscriber-plugin branch August 2, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants