view_frames.py - missing else clause?

FSund commented 3 years ago

Should there be an else here?

https://github.com/ros/geometry2/blob/noetic-devel/tf2_tools/scripts/view_frames.py#L71-L72

I ran into an issue with this on a buggy config I had running just now. There are problems with this config, but having no else seems error-prone as well.

$ rosrun tf2_tools view_frames.py 
[INFO] [1617948744.273147]: Listening to tf data during 5 seconds...
[INFO] [1617948749.281064]: Generating graph in frames.pdf file...
Traceback (most recent call last):
  File "/opt/ros/noetic/lib/tf2_tools/view_frames.py", line 81, in <module>
    main()
  File "/opt/ros/noetic/lib/tf2_tools/view_frames.py", line 53, in main
    f.write(generate_dot(data))
  File "/opt/ros/noetic/lib/tf2_tools/view_frames.py", line 76, in generate_dot
    dot += '}->"'+root+'";\n}'
UnboundLocalError: local variable 'root' referenced before assignment

MatthijsBurgh commented 3 years ago

@FSund The scripts is based on the assumption that there is always minimal one root frame, which has no parent.

As the data contains all the childs. If the parent of a child exists in the data, then that parent, can't be the root. As that parent has a parent too. So there is something incorrect with the incomming data in your run.

tfoote commented 3 years ago

Indeed it looks like a small bug in that it's assuming that there's a root frame. If you're hitting this it does suggest that the tree is malformed and cyclic. Likely the simplest thing to do is to catch it and print the error. It's possible that the rendering could be extended to detect cycles and highlight it with coloring in the graph.

Either of these solutions would be appreciated. Though since this only occurs in invalid states it's a relatively low priority. We'd rather improve the logic in the ROS 2 version: https://github.com/ros2/geometry2/blob/ros2/tf2_tools/tf2_tools/view_frames.py

ros / geometry2

view_frames.py - missing else clause? #507