Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MessageFilter Generalizated Nodes #661

Closed
wants to merge 4 commits into from

Conversation

CursedRock17
Copy link
Contributor

This PR should entirely resolve #95 as pull request #96 didn't use a templated Node in the MessageFilter constructors. There were concerns of:

Retain[ing] the existing node-based interface for backwards compatibility.

which we can do with defaulted templated Nodes and node_interface methods.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@ahcorde
Copy link
Contributor

ahcorde commented Mar 15, 2024

@clalancette what do you think about this?

which we can do with defaulted templated Nodes and node_interface methods.

I can see both strategies in some other packages, any recomendation ?

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

conflicts

@sloretz
Copy link
Contributor

sloretz commented Aug 29, 2024

@CursedRock17 friendly ping to rebase the PR 🧇

@sloretz sloretz added the more-information-needed Further information is required label Aug 29, 2024
@CursedRock17
Copy link
Contributor Author

@sloretz after I saw issue #698, I began writing a fix which would essentially make this PR obsolete. Is it even still worth cleaning up and merging this in, if it's going to be deprecated in the future, I would feel it to be a dead feature.

@clalancette
Copy link
Contributor

@sloretz after I saw issue #698, I began writing a fix which would essentially make this PR obsolete. Is it even still worth cleaning up and merging this in, if it's going to be deprecated in the future, I would feel it to be a dead feature.

Agreed with your reasoning. In that case, I'm going to close this out. But feel free to reopen if you think we should pursue this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lifecycle node support for tf2_ros::MessageFilter
4 participants