ros / geometry2

A set of ROS packages for keeping track of coordinate transforms.
190 stars 275 forks source link

Add new tf_static broadcaster class which can handle more instances per node #484

Closed Timple closed 1 month ago

Timple commented 3 years ago

Solves #406

peci1 commented 3 years ago

I guess the implemented behavior should just become the default behavior for normal static TF broadcaster, as the C++ counterpart does exactly the same... Having this duality in the interface really seems wrong to me.

peci1 commented 3 years ago

And, the other way round, the statically printed warning could get into to C++ counterpart, as the same limitation applies there, too.

Timple commented 3 years ago

the C++ counterpart does exactly the same

Has the same bug? Or the same fix?

peci1 commented 3 years ago

It has the behavior of aggregating all received transforms and sending the aggregate every time.

peci1 commented 3 years ago

https://github.com/ros/geometry2/blob/c73b5939723db078c9bbe18523230ad54f859682/tf2_ros/src/static_transform_broadcaster.cpp#L46-L62

Timple commented 3 years ago

the statically printed warning could get into to C++ counterpart, as the same limitation applies there, too.

So then the warning is not required in C++?

peci1 commented 3 years ago

No, it can still happen that you e.g. compose two nodelets, each with its own static broadcaster. The warning should thus be added to C++, too.

Or, maybe, the solution with static (global) publisher object could be ported to C++, too. But I'm not 100% sure how would that play with the plugin/nodelet loading mechanism. And adding global variables isn't a nice thing, but it could actually solve a lot of headaches and make nodelets safely composable. Do you think you could work on the C++ part or should I try to grasp it?

Timple commented 3 years ago

Feel free to grasp it!

I won't put the effort in the c++ part since I haven't run into that yet :slightly_smiling_face:

peci1 commented 1 year ago

Before this PR is merged, I've released a standalone version with the proposed solution for Python:

http://docs.ros.org/en/noetic/api/cras_py_common/html/cras.html#module-cras.static_transform_broadcaster from package http://wiki.ros.org/cras_py_common .

Timple commented 1 year ago

Yeah, it's a bummer that we need to release custom packages with such minor fixes because they don't get merged or reviewed :slightly_frowning_face:

Timple commented 1 year ago

I've added this as a seperate class in the hope this would be easier to accept this change. Since that does not seem the case, should we fix the original implementation?


And if we're doing aggregation there should be at least some way to also delete/remove the transforms

Not sure I agree. The standard implementation is a latched topic. Here the tf could also only be cleared by deleting the publisher . It could not be revoked. I have not seen this in practice, so why would we need that feature here?


Also it would be good to add at least a basic test

Agreed! Will do

peci1 commented 1 year ago

And if we're doing aggregation there should be at least some way to also delete/remove the transforms besides unloading the python module.

As discussed in https://github.com/ros/geometry2/issues/370, deleting static TFs is not a supported feature (and will never be in ROS 1). To work around this limitation, you can disconnect the transform from the tree (which is a use-case that should be explicitly checked in the unit tests, e.g. transform body->wheel changing to nonexistent->wheel).