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

Reimplement tf_prefix in Noetic exactly as it was in Melodic #169

Merged
merged 5 commits into from Sep 30, 2021

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 1, 2021

This is an alternative to #139. The goal of this PR is to make ROS Noetic robot_state_publisher behave the same as the ROS Melodic version. It's different from #139 in these ways:

  1. It calls the parameter tf_prefix
  2. It reimplements the tf1 prefix and frame slash handling exactly as it behaves in ROS Melodic
  3. It does not break ABI

While it doesn't break ABI, it effectively breaks using JointStateListener with a custom RobotStatePublisher. In this PR JointStateListener calls non-virtual versions of publishFixedTransforms() and publishTransforms(). Users who subclass RobotStatePublisher, override publishFixedTransforms() or publishTransforms(), and pass their subclass instance to a vanilla JointStateListener will observe their callbacks never being called.

Note: ROS 2 robot_state_publisher uses a different approach. It has a parameter frame_prefix that only appends a string to the frame name - no tf1 style special handling of slashes. See #159 for details.

@sloretz sloretz requested a review from clalancette July 1, 2021 19:22
@sloretz sloretz self-assigned this Jul 1, 2021
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left one nit, but otherwise, this is a really good implementation. One other issue is that CI seems to be failing. I'm going to approve anyway, assuming that you'll fix both of these issues before merging.

CMakeLists.txt Show resolved Hide resolved
@jmirabel
Copy link

Any chances this will be merged soon ?

It has been more than a year since the issue has been raised and proposal have been made. I think this functionality is useful and many people would love to use this again.

CI on focal fails because it failed to open the bag file. Does it ring a bell ?

@mlanighan
Copy link

Any updates on a timeline for when this will be merged?

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Copy link
Contributor

@clalancette clalancette 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 assuming CI goes green.

@sloretz sloretz merged commit f00f95d into noetic-devel Sep 30, 2021
@sloretz sloretz deleted the noetic_reimplement_tf_prefix branch September 30, 2021 17:03
robustify added a commit to robustify/audibot that referenced this pull request Mar 18, 2022
In ROS Noetic, support for TF prefixing with robot_state_publisher was removed. However, it was re-implemented in this PR a few months ago: ros/robot_state_publisher#169
robustify added a commit to robustify/audibot that referenced this pull request Mar 18, 2022
In ROS Noetic, support for TF prefixing with robot_state_publisher was removed. However, it was re-implemented in this PR a few months ago: ros/robot_state_publisher#169
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.

None yet

4 participants