ros-visualization / rviz

ROS 3D Robot Visualizer
BSD 3-Clause "New" or "Revised" License
831 stars 463 forks source link

Avoid tf race conditions during update cycle of all displays and cameras #1692

Closed AndreasR30 closed 2 years ago

AndreasR30 commented 2 years ago

During update cycle of displays and views (cameras) the tf buffer should not be updated. This can be achieved by disabling the separate thread for tf listener.

Otherwise it is possible that displays or cameras, which are updated at a later point in time within the cycle, use a more recent transform. This leads to undesired jittering of visuals. This affects displays, which use the latest available transform with ros::Time(). And all views/cameras as they always use the latest available transform.

The videos below show a axes display and a robot model display, which should be fixed to each other.

Here is a video before:

https://user-images.githubusercontent.com/4062443/147690430-288f3d9b-01f8-449b-a432-5b233fd1b273.mp4

and after:

https://user-images.githubusercontent.com/4062443/147690459-ccfa7919-43ac-4692-8fb6-71ce563680b9.mp4

This disables the timeout feature in canTransform or lookupTransform of the tf buffer. But it is not used anywhere in Rviz. I also think that it is not used in any display plugin since these function are blocking then, which is not really desired in update() or in message callback. Furthermore, I don't think that there is too much overhead when the TF messages are processed in main thread.

rhaschke commented 2 years ago

I don't think that there is too much overhead when the TF messages are processed in main thread.

I don't agree with this statement. If you have many TFs published, it will definitely help to process them in a separate thread (on a multi-core machine). See send_lots_of_tf.py for an example.

As I don't see a difference between both videos yet (maybe you can explicitly point me to the jittering you observed), I don't see a particular reason to give up parallel TF processing.

AndreasR30 commented 2 years ago

Please have a look at the Axes Display and the chassis of the car. They should be fixed w.r.t. each other.

I understand your point. However, it is also a problem when die Displays and Cameras do not use the same TFs when stamp ros:Time(0) is used. Using another thread, It would be necessary to "freeze" the TF buffer while the update cycle is ongoing. Unfortunately, there seems to be no such function in tf listener.

rhaschke commented 2 years ago

Please have a look at the Axes Display and the chassis of the car.

Thanks. I noticed the tiny glitches now. Still, I have not convinced this warrants such a drastic change.

simonschmeisser commented 2 years ago

Are there other ways to snapshot the tf-data at the beginning of the render cycle? Or fix the timestamp of each render cycle? Both would be more invasive I fear

rhaschke commented 2 years ago

I think, rviz already provides an appropriate mechanism via FrameManager:

@AndreasR30, do you have resources to experiment with the FrameManager?

rhaschke commented 2 years ago

Fixed via #1698.