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

Restore tf_prefix support for ROS2 #159

Merged
merged 5 commits into from
May 25, 2021
Merged

Conversation

smnogar
Copy link
Contributor

@smnogar smnogar commented Mar 4, 2021

Relevant issue: #147
Relevant PRs: Noetic support: #139 Closed ROS2 PR: #127

This is a quick and dirty implementation of tf_prefix for foxy. Once the string formatting issues are resolved in #139, I recommend adding them here. If this is too duplicative feel free to close this.

smnogar and others added 2 commits May 24, 2021 20:47
Also make it so we don't handle / specially.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette changed the base branch from foxy to ros2 May 24, 2021 21:00
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor

@smnogar First of all, I apologize for the very long delay in looking at this.

What I ended up doing here was make some changes to your PR, partially based off of the discussion that has been going on in #139. In particular I did the following:

  1. I rebased this on top of the ros2 branch, which is where we land any new changes first. Once we have it there, we can consider backporting it to Galactic and Foxy.
  2. I renamed the prefix from tf_prefix to frame_prefix. See Restore tf_prefix support #139 (review) for why we think this should not just be tf_prefix.
  3. I removed any special handling of / here. Since this is now just a pure prefix, and the slash doesn't mean anything special to tf2 anymore, there is no reason to handle it specially here.
  4. I added in a test for this to ensure we are getting what we expect.

With that, I'm pretty happy with this. I'd appreciate a look from you to make sure this meets your needs. @sloretz When you have some time, a review here would be appreciated as well.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

A couple nitpicks, otherwise LGTM!

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

All right, nits are fixed, CI is happy. I'm going to go ahead and merge this one in. @smnogar If you'd like to have this in Foxy or Galactic, please pipe up here and/or feel free to open backport PRs for those. Thanks for the contribution.

@clalancette clalancette merged commit 8a25687 into ros:ros2 May 25, 2021
@padhupradheep
Copy link

Is it possible to have the frame_prefix support for foxy as well ? I guess this is a cool feature, that we would love to have!

@clalancette
Copy link
Contributor

Is it possible to have the frame_prefix support for foxy as well ? I guess this is a cool feature, that we would love to have!

So looking back over this code, I realize that the big problem with backporting is that it is not ABI compatible. That is, it changes the size of to RobotStatePublisher class, so we can't backport it as-is.

That said, we can probably do something slightly hacky where we don't declare the parameter, but instead allow undeclared parameters to be set and just get the parameter as needed. That won't change the ABI. Let me give that a shot and see if I can make it work.

clalancette added a commit that referenced this pull request Jul 6, 2021
quick and dirty frame_prefix support for foxy

Also make it so we don't handle / specially.
Add in tests and documentation as well.

Co-authored-by: Chris Lalancette <[email protected]>
clalancette added a commit that referenced this pull request Jul 6, 2021
quick and dirty frame_prefix support for foxy

Also make it so we don't handle / specially.
Add in tests and documentation as well.

Co-authored-by: Chris Lalancette <[email protected]>
clalancette added a commit that referenced this pull request Aug 2, 2021
quick and dirty frame_prefix support for foxy

Also make it so we don't handle / specially.
Add in tests and documentation as well.

Co-authored-by: Chris Lalancette <[email protected]>

Co-authored-by: Steve Nogar <[email protected]>
clalancette added a commit that referenced this pull request Aug 3, 2021
quick and dirty frame_prefix support for foxy

Also make it so we don't handle / specially.
Add in tests and documentation as well.

Co-authored-by: Chris Lalancette <[email protected]>

Co-authored-by: Steve Nogar <[email protected]>
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.

4 participants