ros / geometry

Packages for common geometric calculations including the ROS transform library, "tf". Also includes ROS bindings for "bullet" physics engine and "kdl" kinematics/dynamics package.
173 stars 275 forks source link

`tf.transformations.rotation_from_matrix` - Does not handle rotation matrix-only case #141

Closed eacousineau closed 7 years ago

eacousineau commented 7 years ago

It appears that there may be an out of index bug with rotation_from_matrix (L319):

>>> import tf.transformations as tform
>>> import numpy.matlib as nm
>>> R = nm.eye(3)
>>> angle, direction, point = tform.rotation_from_matrix(R)
Traceback (most recent call last):
  File "test.py", line 9, in <module>
    angle, direction, point = tform.rotation_from_matrix(R)
  File "/opt/ros/indigo/lib/python2.7/dist-packages/tf/transformations.py", line 346, in rotation_from_matrix
    point /= point[3]
IndexError: index out of bounds

The fix is to change the offending line to:

point /= point[-1]

That being said, it isn't really clear as to:

  1. What is special about direction versus point when R itself is a 3x3 rotation matrix. It also is not clear as to why a larger matrix is permitted.
  2. Why it is dividing by the last element. I'm a little rusty on linear algebra, but I believe this should be normalized with numpy.linalg.norm, but even that seems redundant.

Should this code be trimmed?

Tried git blame, but it appears that this code hunk hasn't been touched since it's first relevant commit back in 2009 (cf65bfc).

EDIT: Just looked at the example and at rotation_matrix, and it shows that this is a homogeneous matrix, oops lol :P

eacousineau commented 7 years ago

Saw the scope of the library. Closing the issue.

tfoote commented 7 years ago

This code is imported from: http://www.lfd.uci.edu/~gohlke/code/transformations.py.html

I'd recommend checking out transforms3d: https://github.com/matthew-brett/transforms3d and https://pypi.python.org/pypi/transforms3d as an updated fork.

tfoote commented 7 years ago

Also there's been some work toward packaging it for ROS too: https://github.com/ros/rosdistro/pull/12437