Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strict type checks (#197) on python interface of Buffer::setTransform make direct input from bagfile cumbersome #219

Closed
stwirth opened this issue Mar 30, 2017 · 8 comments

Comments

@stwirth
Copy link
Contributor

stwirth commented Mar 30, 2017

#197 introduced new type checks on the python interface of Buffer::setTransform to fix #159.
Unfortunately this makes filling a tf buffer from a bagfile cumbersome, as bagfiles have their own type definitions for messages.
Here is an example program that demonstrates the problem:

import tf2_ros
import rosbag
from geometry_msgs.msg import TransformStamped, Vector3, Quaternion

def set_transform_test(t):
    print "setting transform of type {}".format(type(t))
    tf2_buf = tf2_ros.Buffer(None, False)
    tf2_buf.set_transform(t, 'default_authority')

if __name__ == '__main__':
    # create dummy transform
    t = TransformStamped()
    t.header.frame_id = 'parent'
    t.child_frame_id = 'child'
    t.transform.translation = Vector3()
    t.transform.rotation = Quaternion(0, 0, 0, 1)

    set_transform_test(t)

    # write transform to bag and read it again
    wbag = rosbag.Bag('test.bag', 'w')
    wbag.write('transform', t)
    wbag.close()

    rbag = rosbag.Bag('test.bag', 'r')
    for topic, msg, t in rbag.read_messages(topics=['transform']):
        set_transform_test(msg)
    rbag.close()

This is the output:

$ python tf_bag_test.py 
setting transform of type <class 'geometry_msgs.msg._TransformStamped.TransformStamped'>
setting transform of type <class 'tmpTmLCns._geometry_msgs__TransformStamped'>
Traceback (most recent call last):
  File "tf_bag_test.py", line 28, in <module>
    set_transform_test(msg)
  File "tf_bag_test.py", line 9, in set_transform_test
    tf2_buf.set_transform(t, 'default_authority')
TypeError: transform.translation must be of type Vector3

To work around this you need to manually transform the type tmpTmLCns._geometry_msgs__TransformStamped to type geometry_msgs.msg._TransformStamped.TransformStamped.

I would propose to make the type checks less strict, e.g. just enforcing that the transform has a member translation which has three floating point values and a member rotation that has four.

/cc @spaghetti-

@spaghetti-
Copy link

Good catch, I think that enforcing members translation and rotation is good enough as well. I'll try to send in a PR this week.

@stwirth
Copy link
Contributor Author

stwirth commented Mar 30, 2017

awesome!

@spaghetti-
Copy link

Gave some thought to it, it would seem like we have to ensure that the members translation and rotation have their own sufficient members as well.

spaghetti- pushed a commit to spaghetti-/geometry2 that referenced this issue Apr 3, 2017
Previously introduced type check at the buffer::setTransform python
interface checks the supplied variable against the strict
geometry_msgs::Vector3 and geometry_msgs::Quaternion types[0]. This was
later reported to make input from bag files cumbersome because of
temporary intermediate types[1].

This relaxes the type check and only ensures that the expected members
are present. It still issues a UserWarning about the type mismatch.

[0] PR ros#197
[1] Issue ros#219
@spaghetti-
Copy link

After the above patch is applied, with your test program we have:

test.py:8: UserWarning: translation should be of type Vector3
  tf2_buf.set_transform(t, 'default_authority')
test.py:8: UserWarning: translation should be of type Quaternion
  tf2_buf.set_transform(t, 'default_authority')
setting transform of type <class 'geometry_msgs.msg._TransformStamped.TransformStamped'>
setting transform of type <class 'tmpApvwFZ._geometry_msgs__TransformStamped'>

@pklip
Copy link

pklip commented Apr 13, 2017

Hi, I also ran into this (not so obvious) problem. It is also hard to work with the Python bindings, because the tf_ros2 documentation (for Python) in virtually nonexistent: http://docs.ros.org/kinetic/api/tf2_ros/html/python/tf2_ros.html
Could someone please point me to where to add documentation as a pull request?

@tfoote
Copy link
Member

tfoote commented Apr 13, 2017

@ros-pull-request-builder test this please

@tfoote
Copy link
Member

tfoote commented Apr 13, 2017

@pklip A PR for filling out the documentation would be greatly appreciated. It's source is here: https://github.com/ros/geometry2/tree/indigo-devel/tf2_ros/doc

tfoote pushed a commit that referenced this issue Apr 13, 2017
)

Previously introduced type check at the buffer::setTransform python
interface checks the supplied variable against the strict
geometry_msgs::Vector3 and geometry_msgs::Quaternion types[0]. This was
later reported to make input from bag files cumbersome because of
temporary intermediate types[1].

This relaxes the type check and only ensures that the expected members
are present. It still issues a UserWarning about the type mismatch.

[0] PR #197
[1] Issue #219
@tfoote
Copy link
Member

tfoote commented Apr 13, 2017

Fixed in #221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants