swri-robotics / marti_common

Common utility functions for MARTI
BSD 3-Clause "New" or "Revised" License
54 stars 63 forks source link

swri_transform_util: Transform manager needs transform listener in ROS2 foxy #616

Closed jonselling closed 2 years ago

jonselling commented 3 years ago

The Transform Manager does not include transforms from the tf tree since it only uses a tf2_ros::Buffer and will also need a tf2_ros::TransformListener to receive transforms over the ROS tf topics.

I was able to fork the repo and add the transform listener successfully here.

danthony06 commented 3 years ago

Sorry for the slow response, I can start taking a look at this.

matt-attack commented 3 years ago

How does mapviz work in ros2 without this?

pjreed commented 3 years ago

The intended way to use this is to create a single TransformListener and Buffer, then pass the Buffer into your TransformManager. Doing it this way is preferred so that a single process doesn't have multiple subscribers on a single topic and doesn't maintain multiple redundant buffers.

Here's how mapviz does it: https://github.com/swri-robotics/mapviz/blob/2477b3450cd6a698fea25ef9a9598969d3eeb424/mapviz/src/mapviz.cpp#L287

matt-attack commented 3 years ago

Should we make a function to get and initialize (if necessary) a singleton transform manager or buffer then? Seems like the method above pushes that optimization off to the user and complicates usage.

pjreed commented 3 years ago

To a degree, I think the added bit of complication here is a good thing; the user should have to think about how they're managing their listener and buffer, because if they have anything using it other than a single TransformManager, carelessly creating multiple listeners and buffers will cause performance issues. The buffer itself also needs to know what type of clock you're using and whether it has a thread dedicated to it. I'm not sure if there's a good way to hide all of that and still make it usable for anything but the most trivial of use cases.

jonselling commented 3 years ago

I think understanding that the tf2 buffer will be filled external from the transform manager is fine as long as it is conveyed. It was a little confusing to use at first since the ROS 1 version starts the transform listener for you. Adding prints might be good enough - I think these lines. Maybe include some text that the issue could be the buffer is not being filled? It would be ideal if you could check the buffer for how many transforms it has directly, but that doesn't seem to be in the API.

matt-attack commented 3 years ago

I think maintaining the same behavior by default as ros1 makes sense where it automatically generates the buffer and listener (the two are combined in tf1) unless one is specified.

danthony06 commented 2 years ago

@agyoungs did your recent PR incidentally fix this as well?

danthony06 commented 2 years ago

@jonselling I've made this PR which I think adds some of the warnings you're looking for. I also added an optional parameter so we can construct the transform manager with a buffer.

jonselling commented 2 years ago

For reference, you are talking about #666

agyoungs commented 2 years ago

Just noticed this. No @danthony06 my PR for swri_transform_util would not have fixed this.

jonselling commented 2 years ago

@danthony06 I think the changes are good to include but do not solve the issue described here. I believe the issue is that previously in ROS 1 the transform listener was created in the transform manager class. Now, with only the buffer being passed in, there is no way for someone to guarantee the buffer gets filled like it did previously.

I think some resolution can be that just amending these prints that I link to earlier, basically allowing for people to understand that the buffer could also not be filled even if the transforms are being published. I suggested this as a solution since there does not seem to be a public API for a buffer to get if it is empty or not.

From earlier comments, I think the decision was we want to convey the new functionality to users as best as possible where now they need to externally fill their buffers rather than have a listener created in the transform manager class.

danthony06 commented 2 years ago

Okay, I think I understand your comment about the prints. You just want us to change the prints to have additional text telling people to make sure that they have a valid listener that's filling those buffers?

danthony06 commented 2 years ago

Closed via: https://github.com/swri-robotics/marti_common/pull/666