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

Draft: Break geometry_msgs dependency in tf2 #732

Draft
wants to merge 10 commits into
base: rolling
Choose a base branch
from

Conversation

kyle-basis
Copy link

@kyle-basis kyle-basis commented Nov 21, 2024

This is a fairly big, fairly messy PR. It isn't expected that this be merged, nor that the changes here are the correct way to implement this.

This PR implements more of #726 and completely removes the dependency tf2 has on geometry_msgs. Some implementation notes:

As far as I can tell, "just copying the headers from geometry_msgs" isn't enough in the current rolling - there's dependencies on rclcpp, too. That's somewhat neither here nor there, in any case.

The old dependency chain was something like

flowchart TD
tf2_geometry_msgs --> tf2_ros --> tf2 -->geometry_msgs
tf2 --> builtin_interfaces
Loading

tf2_geometry_msgs->tf2_ros->tf2->geometry_msgs. The new dependency chain is (roughly)

flowchart TD
    tf2_ros --> tf2_geometry_msgs --> geometry_msgs
    tf2_geometry_msgs --> tf2
    tf2_geometry_msgs --> builtin_interfaces
Loading

In general, tf2_geometry_msgs is responsible for doing conversions - I did a bunch of renames of tf2_ros::toMsg to tf2::toMsg, but this is likely wrong from an API breakage point of view - it would have been cleaner just to rename the namespace. On the plus side, a bunch of duplicated conversion functions are now gone. Of note: doTransform and the implementation of convert designed for ROS msgs had to move there, as well.

Due to a few places in tf2_ros taking an interface rather than a real type, I had to introduce a new interface BufferCoreROSConversionsInterface, which re-adds the ROS types to BufferCoreInterface. In doing so I had to use virtual inheritance, sorry about that.

tf2_py now depends on tf2_ros as that's where BufferCoreROSConversionsInterface lives.

Various libraries like tf2_bullet could likely depend on tf2_geometry_msgs instead of tf2_ros, at runtime.

I've broken several tests, due to dependencies on setTransform or msg types. Most of these shouldn't be hard to fix. I think tf2_eigen broke due to a needless change to the dependencies, easily fixed. In general, I'm not worried about the tests - I don't think this PR as is will be merged, no point in fixing tests nobody will run.

Currently there's some needless changes to the CMakeList.txt file, this isn't needed any more, except to shim logging, which I'll do internally.

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.

1 participant