search

ros / geometry2

A set of ROS packages for keeping track of coordinate transforms.
189 stars 275 forks source link

New approach for the tf2::toMsg() hassle #491

Closed gleichdick closed 10 months ago

gleichdick commented 3 years ago

The second template parameter of tf2::toMsg() now has a default value, which is resolved to a ROS message type depending on the non-message datatype. This allows the deduction of the return value type.

Furthermore, toMsg(), fromMsg(), getTimestamp() and getFrameId() now have a default implementation, which forwards the calls into structs defined in the impl namespace. Now missing implementations of conversion methods result in compile-time errors, they do not occur during link-time anymore.

A lot of effort was put into the automatic conversion of dataypes with a stamp (tf2::Stamped<T> and the ...Stamped ROS messages) to avoid code duplication. The stamped types now use the conversion methods of the unstamped types and copy the timestamp/frame id informations.

This PR should remain API compatibility, but the ABI will break (as the non-templated functions like toMsg() are removed).

Further TODO's:

@seanyen may I ask you to test whether this approach works on Windows?

Related Issues:

430

ros-planning/moveit#1785

gleichdick commented 3 years ago

@tfoote friendly ping

tfoote commented 3 years ago

Thanks for this. It's going to take me a bit to review this fully.

As this will break ABI as a core module we generally won't want to roll it out into an active rosdistro for something quite this core without a significant problem. Would it make sense to target this for ROS 2 rolling instead of noetic. If we get it into rolling soon we can also shoot to get it into the upcoming Galactic release.

gleichdick commented 3 years ago

Okay, thanks. I'm currently porting this to ROS2, see ros2/geometry2#368.

peci1 commented 1 year ago

I guess this ROS 1 PR can be closed.

tfoote commented 10 months ago

This is something that we definitely can't change in ROS 1 distributions. I'm going to close this and point to the open PR on the ROS 2 repo: https://github.com/ros2/geometry2/pull/427 for further discussion.