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

tf_prefix no longer used in ROS2 node #125

Closed
pbeeson opened this issue Jan 22, 2020 · 11 comments
Closed

tf_prefix no longer used in ROS2 node #125

pbeeson opened this issue Jan 22, 2020 · 11 comments

Comments

@pbeeson
Copy link

pbeeson commented Jan 22, 2020

Is there a reason why tf_prefix can no longer be sent to the robot_state_publisher in ROS2?

In ROS1, we use multiple robot_state_publishers as predictive displays for planning, each having a different namespace that maps /namespace/joint_states to the appropriate namespace in tf information. So one urdf ends up creating multiple namespaced TF trees for the robot.

Attempting to migrate to ROS2 seemingly makes such functionality un-migratable.
Is there an expected different way of doing this in ROS2?

@sloretz
Copy link
Contributor

sloretz commented Jan 22, 2020

tf_prefix was a feature of tf that was dropped in tf2. Only tf2 has been ported to ROS 2, so there's no tf_prefix feature that robot_state_publisher can use.

One alternative is to remap /tf and /tf_static to /namespace/tf and /namespace/tf_static. Then have a node subscribe to those and publish transforms with prefixed frame names to the global /tf and /tf_static topics. This is similar to what ARIAC 2019 does: https://bitbucket.org/osrf/ariac/src/master/osrf_gear/src/tf2_relay.cpp

I'll close this in favor of ros/geometry2#32

@sloretz sloretz closed this as completed Jan 22, 2020
@pbeeson
Copy link
Author

pbeeson commented Jan 22, 2020

I really don't understand this. If you look at the actual code for eloquent in robot_state_publisher, there is a tf_prefix variable that does get tacked onto both both the parent and child frame of the TFs being broadcast. This does not used tf2 methods at all. It's basically like a "namespace" that publishes on /tf without needing a messy TWO node solution like you suggest doing above (if only tf_preifix would read in as a parameter instead of being hard-coded to empty string).

That is, tf_prefix in robot_state_publisher DOES NOT depend on tf2 at all. Just because it has the same name as an old method doesn't mean that the unrelated functionality needs to be removed. RViz2 is still supporting tf_prefix in that when you enter a string there, it does exactly what I describe above and essentially prepends the string onto the frames. So if RViz2 is supporting it, and the code is in the ROS2 port to prepend, and all you need is to read tf_prefix in as a parameter instead of hard-coding it to "", then I don't understand why it can't be there. This seems 100% orthogonal to any tf2 deprecation.

@sloretz
Copy link
Contributor

sloretz commented Jan 22, 2020

If you look at the actual code for eloquent in robot_state_publisher, there is a tf_prefix variable that does get tacked onto both both the parent and child frame of the TFs being broadcast. This does not used tf2 methods at all.

I see what you're saying. In ROS 1 tf_prefix support was done via tf::resolve().

tf_transform.header.frame_id = tf::resolve(tf_prefix, seg->second.root);
tf_transform.child_frame_id = tf::resolve(tf_prefix, seg->second.tip);

You're right the eloquent branch has code to concatenate a prefix:

tf_transform.header.frame_id = tf_prefix + "/" + seg->second.root;
tf_transform.child_frame_id = tf_prefix + "/" + seg->second.tip;

It looks like it's still there in the ROS 2 branch. What do you mean by "tf_prefix no longer used"? What was removed?

tf_transform.header.frame_id = tf_prefix + "/" + seg->second.root;
tf_transform.child_frame_id = tf_prefix + "/" + seg->second.tip;

@sloretz sloretz reopened this Jan 22, 2020
@pbeeson
Copy link
Author

pbeeson commented Jan 22, 2020

tf_prefix is set to "" in the main and that's what's appended. There' s no way to actually assign tf_prefix a value (i.e. via a parameter at runtime).

@pbeeson
Copy link
Author

pbeeson commented Jan 22, 2020

So, I feel like I could make a quick PR that declares and gets tf_prefix, show that it works, and submit the PR. But, I didn't want to waste my time if there was 1) a BETTER way and 2) if there wasn't a change that PR would be accepted.

I see your original alternative as a way to achieve this as well, but all that is doing is making the prepending that much more complex (And adding latency).

@pbeeson
Copy link
Author

pbeeson commented Jan 22, 2020

This is what I mean by 'no longer used'

This used to be a ros param.

@clalancette
Copy link
Contributor

The original robot_state_publisher port was a basic port, and was missing some functionality of the ROS 1 version. I've mostly rectified that in #126

That being said, #126 completely removes the tf_prefix. The reasoning there is that tf_prefix used to actually use the tf functionality that @sloretz mentioned, and that was deprecated and removed in tf2. I'm inclined to stick with that decision for now; my understanding is that it was removed from tf2 because it was a failed experiment, and the ROS ecosystem as a whole didn't support prefixes on tf frames very well.

We could consider reintroducing that functionality (under a different name), but I'd first want to hear from @tfoote why it was removed from tf2 in the first place.

@pbeeson
Copy link
Author

pbeeson commented Jan 22, 2020

Well 1) it was VERY useful, 2) it did exactly what @sloretz suggests using 2 nodes for in a very simple way, and 3) RViz2 still supports this. If you want to change the name to tf_namespace, then fine, but the functionality to easily replicate full TF trees with different namespaces by running multiple robot_state_publishers was very much used by people.

@sloretz
Copy link
Contributor

sloretz commented Oct 12, 2020

FYI some comments on bringing prefixing behavior back as a robot_state_publisher feature here: #139 (review)

@pbeeson
Copy link
Author

pbeeson commented Nov 11, 2020

I believe that issue #139 supersedes this issue.

@pbeeson pbeeson closed this as completed Nov 11, 2020
@sloretz
Copy link
Contributor

sloretz commented Sep 30, 2021

Resolved by #169

ckennedy2050 added a commit to ckennedy2050/Azure_Kinect_ROS2_Driver that referenced this issue Sep 27, 2022
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

No branches or pull requests

3 participants