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

Split tf2_kdl and Port Python Functionality #665

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

CursedRock17
Copy link
Contributor

This is meant to resolve #208 by creating a tf2_kdl_py package and moving all the python parts from tf2_kdl to it.

The PR will be good preparation for #110 as PyKDL is now available.

@CursedRock17 CursedRock17 changed the title Tf2 kdl py Splitting tf2_kdl Apr 1, 2024

<test_depend>python3-pytest</test_depend>
<test_depend>rclpy</test_depend>
<test_depend>tf2_msgs</test_depend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see this used in the test

tf2_kdl_py/package.xml Outdated Show resolved Hide resolved
tf2_kdl_py/package.xml Outdated Show resolved Hide resolved
tf2_kdl_py/CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@clalancette clalancette left a 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 couple things that I think we should fix. Otherwise, this looks good to me.

tf2_kdl_py/test/test.py Outdated Show resolved Hide resolved
tf2_kdl_py/package.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left one more change inline.

Besides that, I realize that we should also update https://github.com/ros2/geometry2/blob/rolling/geometry2/package.xml . We haven't been totally consistent here, but I think we should add in tf2_kdl_py as well as tf2_ros_py as dependencies there.

And finally, I realize that the way that this is installed makes it so that import tf2_kdl no longer works (import tf2_kdl_py works, but we need to maintain backwards compatibility). This becomes obvious once you make the changes to make the tests work (inline). We need to fix that as well.

tf2_kdl_py/test/test_python.launch Outdated Show resolved Hide resolved
tf2_kdl_py/package.xml Outdated Show resolved Hide resolved
tf2_kdl_py/test/test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some things to do:

  • I can see the tf2_kdl CMakeLists.txt that you are installing the python package
  • Remove this comment from the CMakeLists.txt
  • As this PR splits the cpp and python implementation this dependency in the package.xml is not required
  • Finally you can remove this file src/tf2_kdl

@ahcorde
Copy link
Contributor

ahcorde commented Apr 3, 2024

@CursedRock17 do you mind to merge with rolling ? Then I can launch the CI

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Removing extra dependencies

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Updating package version

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Correcting dependencies

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Fixing imports

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Removing excess from tf2_kdl

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Fixing imports

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Removing excess from tf2_kdl

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17
Copy link
Contributor Author

@ahcorde it was rebased locally with rolling, so it should be good now.

@ahcorde
Copy link
Contributor

ahcorde commented Apr 4, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://ci.ros2.org/job/ci_linux-aarch64/15037/console

09:59:55 ImportError while importing test module '/home/jenkins-agent/workspace/ci_linux-aarch64/ws/src/ros2/geometry2/tf2_kdl_py/test/test_kdl.py'.
09:59:55 Hint: make sure your test modules/packages have valid Python names.
09:59:55 Traceback:
09:59:55 /usr/lib/python3.10/importlib/__init__.py:126: in import_module
09:59:55     return _bootstrap._gcd_import(name[level:], package, level)
09:59:55 test/test_kdl.py:37: in <module>
09:59:55     import tf2_kdl  # noqa(F401)
09:59:55 E   ModuleNotFoundError: No module named 'tf2_kdl'

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@ahcorde
Copy link
Contributor

ahcorde commented Apr 5, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are failing on Linux and there is a problem with PyKDL on Windows

@ahcorde
Copy link
Contributor

ahcorde commented Apr 10, 2024

Maybe the error on Windows is because PyKDL is not installed on Windows. @clalancette ?

@clalancette
Copy link
Contributor

Maybe the error on Windows is because PyKDL is not installed on Windows. @clalancette ?

We build it from source there: https://ci.ros2.org/job/ci_windows/21434/consoleFull#console-section-182

@CursedRock17
Copy link
Contributor Author

I might be mistaken, but I feel like the CI will continuously fail and show warnings until #110 is resolved, that being said I'm working on a port right now. Would it be more beneficial to comment out the pytest since it wasn't available anyways, merge this without it, then work fresh with the port. Or rebase this branch after we've ported a solution.

@clalancette
Copy link
Contributor

I might be mistaken, but I feel like the CI will continuously fail and show warnings until #110 is resolved, that being said I'm working on a port right now. Would it be more beneficial to comment out the pytest since it wasn't available anyways, merge this without it, then work fresh with the port. Or rebase this branch after we've ported a solution.

So I guess I'm confused; isn't the point of this PR to resolve #110? I guess if not, then what we should do is to both solve #110 and #208 in this PR simultaneously. Otherwise, there is no point to having a split tf2_kdl_py if it doesn't work.

@CursedRock17
Copy link
Contributor Author

Gotcha, the original plan was resolving just #208 but since I'm already prepping a port, I can add it to this PR. It makes it much easier this way anyway.

@CursedRock17 CursedRock17 changed the title Splitting tf2_kdl Split tf2_kdl and Port Python Functionality Apr 22, 2024
@ahcorde
Copy link
Contributor

ahcorde commented Apr 24, 2024

I was checking this on a Windows machine, the problem is on python_orocos_kdl_vendor on windows the PYTHONPATH is not extended, we need to patch this package first. Adding manually the path to the PYTHONPATH the test is working but failing.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
@CursedRock17
Copy link
Contributor Author

So it's been a while, but I've got a valid product in which the Python portion of KDL has been ported to match the C++ features. I still have not gotten the chance to change the path on windows, but I'm more concerned with a tier one solution for the mean time. Changes so far:

  1. To support stamped messages, a new file needed to be created which creates a stamped version of every PyKDL class used. This ended up being an indirect response to issue PyKDL tf2_ros.Stamped broken #624, as the dynamic attributes had gone away, but these frames still needed to be "stamped". I then removed the old stamped wrapper as the class-based system required the same amount of steps, but there would be no point in keeping an outdated feature, that being said if it breaks some sort of API, I'll add it back
  2. Honestly this system may not have even worked in the past, based on the conversion method which had to be changed, to support these new messages, but stay constant with the amount of function calls needed as to not raise runtime costs.
  3. A full set of tests has been added to ensure that each transform method works to a tee, and each of the 4 styles of conversion work as intended, either between types or as deep copies. The only thing I can't quite ensure are the Rotation values which are harder to get even values since converting from Rotation to Quaternion is convoluted
  4. A basic set of docs has been added, nothing crazy, it's quite a simple package, but should have a start up page

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

Successfully merging this pull request may close these issues.

Split the tf2_kdl package into tf2_kdl and tf2_kdl_py
3 participants