-
Notifications
You must be signed in to change notification settings - Fork 194
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
Drop PyKDL dependency in tf2_geometry_msgs #509
Conversation
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
I'm torn on whether we should take this. On the plus side, this gets rid of the PyKDL dependency which has been nagging this package for a long time, and makes this package available to users. On the negative side, we are hoping to get PyKDL into Humble (via ros2/orocos_kdl_vendor#1), and this ends up dropping the public API Here are some options I can see for this:
There may be other options. @ros2/team @Flova Thoughts? |
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
I understand the implications and that this is a tough choice. I also appreciate the efforts regarding PyKDL/orocos_kdl which I am following for some time now. As all introduced methods are private, I would see no issue except the administrative overhead, in releasing this fix as it is for now (with your feedback + some testing) and replacing it with the PyKDL implementation later on if it is available again. I am not that worried about the I would suggest opening a new PR to merge things like the improved typing/docstrings of the existing functions as well as the fixes applied to the tests, if we choose to close this one. |
Any other thought on this? |
I think we can go ahead here and review this. We can always add back the |
Nice, thank you :) |
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.
I've left a few minor things to change. Looks good overall; once those things are fixed, I'll fire up CI on it.
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
I think I fixed everything. Let me know if there is something left I overlooked or if I should do something different. |
This PR closes #360, right? |
Yeah, it should. I think it will also close one or two open issues, but I'll have to check on that. |
I also tested in 20.04, but I guess my NumPy version was replaced/overlaid with a newer one by installing some pip package that depended on it. https://packages.ubuntu.com/focal/python3-numpy shows that the current version is 1.17 and Typing was introduced in 1.20. Replacing the more general |
I will address these flake8 errors, but my flake8 checks passed. Is there a config I don't know about for ROS2 and flake8 which is not applied automatically? Or is it because of the first point you mentioned? |
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
I fixed the first and third issue you observed @clalancette, but the flake8 one seems kind of weird. The first point regarding the order of the imported elements from import numpy as np
from geometry_msgs.msg import (PointStamped, Pose, PoseStamped,
PoseWithCovarianceStamped, TransformStamped,
Vector3Stamped)
import tf2_ros This seems of as normally the
Do you use a special flake8 config? From the output, it seems like you have built the |
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Unfortunately flake8 is a bit annoying. Basically it runs with the flake8 plugins you currently have installed locally. The practical upshot of this is that you happen to have one missing, it will just silently ignore it. As for the weird import order, yeah, I don't always agree with how flake8 orders things. But it's easier to just conform to it. I ended up fixing it with the latest commit. While testing, I found a small bug in tf2_ros_py, where it isn't depending on sensor_msgs properly. That's now fixed in here as well. With all of that done, I'm happy with this so I'll do one more pass and then run CI on it. |
Nice, the CI seem to look good. |
Hi! Is there a timeline for getting these changes into a release? Currently Thanks! |
It is currently available in Humble and Rolling. It's probably possible to backport to Foxy, though it might be a bit tricky; we had to do a couple of additional patches on top of this one to get it working. What I'll do here is to ask the bot to try to backport it. That said, I'm unlikely to have time myself to fix it up, so it would be appreciated if you can fix any conflicts that arise, test it out, and generally make sure it will work in Foxy. |
@Mergifyio backport foxy galactic |
* Drop PyKDL dependency Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> (cherry picked from commit 6b18a40) # Conflicts: # tf2_geometry_msgs/CMakeLists.txt # tf2_geometry_msgs/package.xml # tf2_geometry_msgs/src/tf2_geometry_msgs/tf2_geometry_msgs.py # tf2_geometry_msgs/test/test_tf2_geometry_msgs.py # tf2_ros_py/package.xml
* Drop PyKDL dependency Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> (cherry picked from commit 6b18a40) # Conflicts: # tf2_geometry_msgs/CMakeLists.txt # tf2_geometry_msgs/package.xml # tf2_geometry_msgs/src/tf2_geometry_msgs/tf2_geometry_msgs.py # tf2_geometry_msgs/test/test_tf2_geometry_msgs.py
✅ Backports have been created
|
@clalancette Thank you! I'll take a look at testing the foxy backport and move discussion over to |
#533) * Drop PyKDL dependency in tf2_geometry_msgs (#509) * Drop PyKDL dependency Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> (cherry picked from commit 6b18a40) # Conflicts: # tf2_geometry_msgs/CMakeLists.txt # tf2_geometry_msgs/package.xml # tf2_geometry_msgs/src/tf2_geometry_msgs/tf2_geometry_msgs.py # tf2_geometry_msgs/test/test_tf2_geometry_msgs.py * Fix merge conflicts Co-authored-by: Florian Vahl <florian@flova.de>
) * Drop PyKDL dependency in tf2_geometry_msgs (#509) * Drop PyKDL dependency Signed-off-by: Florian Vahl <git@flova.de> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> (cherry picked from commit 6b18a40) # Conflicts: # tf2_geometry_msgs/CMakeLists.txt # tf2_geometry_msgs/package.xml # tf2_geometry_msgs/src/tf2_geometry_msgs/tf2_geometry_msgs.py # tf2_geometry_msgs/test/test_tf2_geometry_msgs.py # tf2_ros_py/package.xml * Clean up merge conflicts. Builds and tests pass! Co-authored-by: Florian Vahl <florian@flova.de>
Currently, the python
tf2_geometry_msgs
are excluded because of the PyKDL/orocos issue.This pull request drops the dependency all together and replaces the transformations with ones done in pure NumPy.