-
Notifications
You must be signed in to change notification settings - Fork 274
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
Fix tf2 compatibility of tf python implementation #134
Conversation
The original implementation is not fully compatible with tf2, e.g. it does not subscribe to /static_tf and thusly cannot support the new static transformations introduced by tf2. Similar to the C++ code, the Python implementation forwards all calls to the tf2 Python implementation now, ensuring that all implementation details of tf2 are properly hidden and the tf library will remain compatible.
Addresses #117 |
getLatestCommonTime() is needed to implement the TF API. See ros/geometry#134
getLatestCommonTime() is needed to implement the TF API. See ros/geometry#134
tf/package.xml
Outdated
<export> | ||
<roswtf plugin="tf.tfwtf" /> | ||
<!-- <roswtf plugin="tf.tfwtf" /> --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does tfwtf not work anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tfwtf is written for tf1 (e.g. it explicitly subscribes to /tf
), so IMHO this functionality should be implemented by tf2 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should still work though, since the implementation details remain compatible enough for now.
tf/test/testPython.py
Outdated
@@ -78,10 +83,10 @@ def elapsed_time_within_epsilon(t, delta, epsilon): | |||
epsilon = 0.1 | |||
|
|||
# Check for dedicated thread exception, existing frames | |||
self.assertRaises(tf.Exception, lambda: t.waitForTransform("PARENT", "THISFRAME", rospy.Time(), timeout)) | |||
###self.assertRaises(tf.Exception, lambda: t.waitForTransform("PARENT", "THISFRAME", rospy.Time(), timeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these asserts commented out?
@ros-pull-request-builder retest this please |
Apparently, the failed check is caused by the unit tests of |
Is there anything else you need me to fix before this can get merged? |
Now that tf2 has been released with the upstream changes, it needs a version dependency. Other than that I've just been starved for maintenance time. |
I took the liberty to fix the unstable build, even though it took me a bit longer than I care to admit ;-) |
Great, thanks for the contributions! I will shoot to merge and release this after we get the pending sync out. |
rebased and merged in #149 |
The Python code is completely replaced by forwarding calls to tf2_ros. This makes tf fully compatible with tf2, in particular it adds the missing support for static transformations.
A few API calls are no longer supported in the tf2 Python implementation. Most notably, the current pull request does not implement
chain()
,frameExists()
, andgetFrameStrings()
. I believe those methods are rarely used and therefore expendable.The
lookupTwist()
andgetLatestCommonTime()
implementations rely on ros/geometry2#192