tf2_ros is not built for Python

[vinnamkim]

This PR targeting #26 to handle the issue #87

[tfoote]

Thanks for pushing on this.

I had a few quick comments, but can't really review this as it has conflicts and is not in a mergeable state. You'll need to rebase against the latest ros2 branch to get it to be mergable.

[vinnamkim]

Thanks for pushing on this.

I had a few quick comments, but can't really review this as it has conflicts and is not in a mergeable state. You'll need to rebase against the latest ros2 branch to get it to be mergable.

Thanks for your comment. I've found that there are a lot of works to make tf2_ros(python) properly working as follows.

Task 1. Make tf2.BufferCore(tf2_py) working correctly
Task 2. Add python unit tests for tf2.BufferCore to verify
Task 3. Port tf2_ros(python) with rclpy
Task 4. Add python unit tests for tf2_ros(python) to verify

I'm trying to update this PR. Task 1 and 2 are finished and updated in this PR. It seems to need more time to finish Task 3 and 4.

[vinnamkim]

Thanks for pushing on this.
I had a few quick comments, but can't really review this as it has conflicts and is not in a mergeable state. You'll need to rebase against the latest ros2 branch to get it to be mergable.

Thanks for your comment. I've found that there are a lot of works to make tf2_ros(python) properly working as follows.

Task 1. Make tf2.BufferCore(tf2_py) working correctly
Task 2. Add python unit tests for tf2.BufferCore to verify
Task 3. Port tf2_ros(python) with rclpy
Task 4. Add python unit tests for tf2_ros(python) to verify

I'm trying to update this PR. Task 1 and 2 are finished and updated in this PR. It seems to need more time to finish Task 3 and 4.

@tfoote I guess it is ready to review.

[ahcorde]

@tfoote did you have the chance to review this? we need to merge this before merging this other PR.

Can I help in any way?

[nuclearsandwich]

@vinnamkim this pull request needs to be rebased against the latest ros2 branch in order to be easily reviewable. Is that something you can do? Thanks.

[vinnamkim]

@vinnamkim this pull request needs to be rebased against the latest ros2 branch in order to be easily reviewable. Is that something you can do? Thanks.

Rebased it to the latest ros2.

[clalancette]

The geometry2 and geometry_experimental packages shouldn't be removed by this PR; if you are interested in doing that, please propose it in a separate PR.

For any new code you've added, please make sure there are no trailing whitespace; I saw that in a few places. For any old code, let's not make whitespace changes in this PR so we can concentrate on the code changes.

There is a potential circular dependency in this port since tf2_py could benefit from using some functions from tf2_ros. But currently in this port, tf2_ros depends on tf2_py. I think what I'd do instead is to have:

tf2_ros -> C++ only library
tf2_py -> depends on tf2_ros
tf2_ros_py -> python library depending on tf2_py.

@tfoote what do you think of that proposal?

[vinnamkim]

@clalancette Thanks for your review. I updated this PR. Would you mind checking again please?

[clalancette]

This is looking generally pretty good to me. I'll kick off CI so we can see how Windows breaks.

I'd also like to get @tfoote 's opinion on both the code presented here and my proposal to split this into multiple packages. Once we have consensus, we can revisit this.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[vinnamkim]

@clalancette I missed geometry_msgs dependency. I added geometry_msgs dependency to tf2_py like tf2_ros. However, how about exporting geometry_msgs dependency from the tf2 package? I guess it seems more natural since tf2 has geometry_msgs messages as parameters of it's functions.

[clalancette]

However, how about exporting geometry_msgs dependency from the tf2 package? I guess it seems more natural since tf2 has geometry_msgs messages as parameters of it's functions.

Arg. Well, there are a couple of things here:

1. tf2_py does depend directly on geometry_msgs, so adding that dependency here is correct.
2. But you are correct that the tf2 package also depends on geometry_msgs, and doesn't export that fact.
3. However, part of the reason it doesn't do that is because tf2 is really meant to be ROS-agnostic. So this is a bug that we should eventually fix.

So points 2 and 3 mean that there is a bug, but point 1 means that this should compile now. So we'll worry about the tf2 bug separately. Now that you've fixed the geometry_msgs problem, I'll kick off another CI.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

I've rebased on the latest, and did some minor cleanup. We still need to find out what is going on on macOS and Windows here. I'm on vacation for the next week, so you won't hear much from me until after that. If nobody else gets to it, I'll pick this up when I'm back.

[clalancette]

The latest code I pushed should make it so that it compiles on both Windows and macOS. And now the tests pass. But I've been unable to actually make it lookup a transform with code like:

import time

import rclpy
import tf2_ros

rclpy.init()

tf_buf = tf2_ros.Buffer()
listener = tf2_ros.TransformListener(tf_buf)

class mytime:
    def __init__(self, ns):
        self.nanoseconds = ns

zerotime = mytime(0.0)

while rclpy.ok():
    trans = tf_buf.lookup_transform('child', 'parent', zerotime, rclpy.duration.Duration(seconds=5))

    time.sleep(0.1)

(in another terminal, I did ros2 run tf2_ros static_transform_publisher 0 0 0 0 0 0 1 parent child)

Now I am truly out of time. We'll want to fix whatever is causing it not to be able to lookup transforms, and add a test for that, before merging.

[clalancette]

I've got a number of comments; most of them are pretty simple/straightforward, with the exception of error handling which I think can be done better.

Besides that, there are 3 follow-up issues that should be created:

1. We should have a follow-up where we fixup the style in this code, including switching to nullptr.
2. We should have a follow-up where we split tf2_ros into tf2_ros and tf2_ros_py (see #99 (review))
3. We should have a follow-up where we discuss https://github.com/ros2/geometry2/pull/99/files#diff-c7c674ae33e71f2cfe70fa723b501d4fR159

[clalancette]

This is looking good to me. I'm going to approve. I'll go ahead and create the follow-up issues and run CI, and assuming that is all happy, I'll merge this.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[pleconte87]

Can you tell me the status of this? We would like to use tf2_ros with python in our ros2 project but we are at this moment unable to import it properly. We use the following release :https://github.com/ros2/ros2/releases/download/release-dashing-20190806/ros2-dashing-20190806-linux-bionic-amd64.tar.bz2

[clalancette]

Can you tell me the status of this?

This was merged into master, and will be part of the Eloquent release: https://index.ros.org//doc/ros2/Releases/Release-Eloquent-Elusor/

The changes were too invasive to backport into Dashing.

[RoboticsYY]

Hi, I have successfully compiled but unable to import it. When I tried to import tf2_ros, I came into an error: No module named _tf2_py.

Seems python path is OK and I can see there is _tf2_py.cpython-36m-x86_64-linux-gnu.so and __init__.py in the ~/ros2_handeye/install/tf2_py/lib/python3.6/site-packages/tf2_py folder. Is there anyone met the same problem?

[sloretz]

@RoboticsYY Please open an issue for the problem if there isn't one already. Comments on closed PRs are hard to find.