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

[Timple]

Solves #406

[peci1]

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]

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

[Timple]

the C++ counterpart does exactly the same

Has the same bug? Or the same fix?

[peci1]

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

[peci1]

geometry2/tf2_ros/src/static_transform_broadcaster.cpp

Lines 46 to 62 in c73b593

	void StaticTransformBroadcaster::sendTransform(const std::vector<geometry_msgs::TransformStamped> & msgtf)
	{
	for (const geometry_msgs::TransformStamped& input : msgtf)
	{
	auto predicate = [&input](const geometry_msgs::TransformStamped existing) {
	return input.child_frame_id == existing.child_frame_id;
	};
	auto existing = std::find_if(net_message_.transforms.begin(), net_message_.transforms.end(), predicate);
	if (existing != net_message_.transforms.end())
	*existing = input;
	else
	net_message_.transforms.push_back(input);
	}
	publisher_.publish(net_message_);
	}

[Timple]

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]

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]

Feel free to grasp it!

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

[peci1]

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]

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

[tfoote]

Sorry for the delay. This is still pretty basic and missing some features that will provide coverage for common use cases before it gets merged into the core implementation. We'll want to put it through and API review as anything that gets added here will need to then go directly into the tick tock release cycle and as there's no upcoming releases there's no cycles available for iterating. And new features in a stable released version are generally deprioritized.

And if we're doing aggregation there should be at least some way to also delete/remove the transforms besides unloading the python module. It might make more sense to keep a handle to each transform and reference count them to know when to start and stop the publishing any given transform.

Also it would be good to add at least a basic test so that we can know that this is working correctly and that it keeps working down the line.

[Timple]

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]

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 #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).