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

Usage of tf2 outside of ROS (evaluation for Basis) #726

Open
kyle-basis opened this issue Nov 11, 2024 · 13 comments
Open

Usage of tf2 outside of ROS (evaluation for Basis) #726

kyle-basis opened this issue Nov 11, 2024 · 13 comments

Comments

@kyle-basis
Copy link

kyle-basis commented Nov 11, 2024

Bug report

Required Info:

  • Operating System:
    • N/A
  • Installation type:
    • N/A
  • Version or commit hash:
  • DDS implementation:
    • N/A
  • Client library (if applicable):
    • N/A

Steps to reproduce issue

Unable to compile tf2 outside of ROS.

Expected behavior

Can compile tf2 outside of ROS, given ample legwork, given documentation.

Actual behavior

Cannot compile tf2 outside of ROS.

Additional information

Hi, I'm evaluating use of tf2 inside another robotics framework (https://github.com/basis-robotics/basis). Looking at https://github.com/ros2/geometry2/blob/rolling/tf2/mainpage.dox, there's a claim that Core is ROS agnostic, but looking at https://github.com/ros2/geometry2/blob/rolling/tf2/src/buffer_core.cpp#L181 there's still geometry_msgs sprinkled around (and in some other places, too). setTransformImpl looks nicely independent, but setTransform uses the ROS type. The same with the other *Impl types. The core seems to be pretty generic, it's not far off.

I'm happy to do the legwork to write my own CMake file, integration, etc - but I'd like to avoid forking the core library itself. Would a PR be accepted that changed getTransform to return a tf2::Transform, and then ran the conversion to rosmsg inside of tf2_ros or another library that sits in between the two? (and the same with the other interfaces that are tied to ROS?)

tf2 support is feedback that I've gotten from potential users, it would be great if I could provide another frontend to your library, rather than reimplementing it myself.

@clalancette
Copy link
Contributor

I'm happy to do the legwork to write my own CMake file, integration, etc - but I'd like to avoid forking the core library itself. Would a PR be accepted that changed getTransform to return a tf2::Transform, and then ran the conversion to rosmsg inside of tf2_ros or another library that sits in between the two? (and the same with the other interfaces that are tied to ROS?)

Yes and no. Certainly we could consider adding generic APIs that are ROS-independent; that is no problem.

However, removing/moving the ROS-specific APIs would have to be done over several releases, since we can only break API in a "tick-tock" manner; in one release we deprecate an API, and in the next we remove it.

My suggestion is to start with a PR for the "easy" part; adding in the generate API. We can review that one. And then we can talk about moving those ROS-specific APIs with deprecations.

@alsora
Copy link
Contributor

alsora commented Nov 11, 2024

+1 on doing this work, starting from adding the new APIs that work on tf2::Transform.

I was indeed looking to get this done myself, although for a different reason.
While doing some profiling of how tf2 was used in our codebase and how its APIs are implemented, I noticed that there are often redundant conversions between tf2 types and geometry_msgs types.
Having the core use only tf2 types and expose APIs to get this data type would alleviate the problem.

@kyle-basis
Copy link
Author

I'm happy to do the legwork to write my own CMake file, integration, etc - but I'd like to avoid forking the core library itself. Would a PR be accepted that changed getTransform to return a tf2::Transform, and then ran the conversion to rosmsg inside of tf2_ros or another library that sits in between the two? (and the same with the other interfaces that are tied to ROS?)

Yes and no. Certainly we could consider adding generic APIs that are ROS-independent; that is no problem.

Glad to hear it.

However, removing/moving the ROS-specific APIs would have to be done over several releases, since we can only break API in a "tick-tock" manner; in one release we deprecate an API, and in the next we remove it.

I have no desire to do a hard break of the API. How long would that take? I'm trying to figure out how to cross this against the work I need to do to get a basis-based build going. I think the easiest option would be to make the ROS bindings able to be #ifdef'd out, that way the API as ROS and friends see it stays the same, but generic frontends can do something else. The other option is to make another class that sits in between BufferCore and BufferCoreInterface but it feels wrong.

My suggestion is to start with a PR for the "easy" part; adding in the generate API. We can review that one. And then we can talk about moving those ROS-specific APIs with deprecations.

Do you have preferences for naming? We have setTransform and setTransformImpl, so...setTransformTf2? setTransformGeneric?

@alsora
Copy link
Contributor

alsora commented Nov 12, 2024

I think the easiest option would be to make the ROS bindings able to be #ifdef'd out, that way the API as ROS and friends see it stays the same, but generic frontends can do something else

We don't generally ifdef-out things.
This will introduce compile-time changes of the available APis and it would be confusing for the users (that wouldn't know which version they installed from rosdistro. We could discuss it, but I would vote against it unless there's a very strong reason (such as several other users that request it), which I don't see right now.

This would also require your users to manually build that repo.

The other option is to make another class that sits in between BufferCore and BufferCoreInterface but it feels wrong.

Yes, I agree that this feels wrong.

Normally, what we do is that we do a PR that adds the new APIs and marks as deprecated the old ones.
This will be pushed to the rolling branch.
The new APIs can be backported to other ros distributions, but not the deprecation.

After the next ROS release is out (in May next year) we would remove the deprecated functions in the rolling branch and as such from the following releases.
So it's a long process.
I don't think we can remove a public dependency from a ROS package really faster than that.

I think that for you, once the new APIs are in place, the easiest thing is to fork and ifdef-out the old functions (you can do it also now, but at least that way you can first make sure that the new APIs are what will eventually be used by the "official ROS distro").
Eventually.
Next year, when the new ros release will be out you'll be able to get back to use the official repo.

@alsora
Copy link
Contributor

alsora commented Nov 12, 2024

I think that the closest thing we did is what happened with rosbag2.
In that case there were some major performance updates to the rosbag package, and a lot of users interest ros2/rosbag2#657

Note how in that case we created a new branch foxy-future that included the API-breaking changes.
This branch was not released via rosdistro and had to be build by the users manually.

Note that at the time, rosbag2 was almost unusable without those changes.

@kyle-basis
Copy link
Author

I don't mind forking as long as eventually my fork and upstream converge, sounds good.

@kyle-basis
Copy link
Author

Did some more work on this - found two more areas that depends on ROS - logging and the CMake file itself. Both of those are by necessity compile time choices. The CMake file can be split into two so that vanilla (non-CMake) builds can operate on just the library itself, or controlled via an option. Logging is trickier - it would be easier if you'd reconsider not allowing compile time choices for tf2 - anyone wanting to have a ROS free version of tf2 won't be pulling it from rosdistro anyhow. Basis's workflow is to use CMake FetchContent and compile most dependencies that way.

The only other option I see is to have a runtime configurable logging handler. It looks like the only place there's logging is in buffer_core.cpp so this might work.

@clalancette
Copy link
Contributor

The CMake file can be split into two so that vanilla (non-CMake) builds can operate on just the library itself, or controlled via an option.

I'm not sure I understand you here. Can you explain more about what you are trying to accomplish?

Logging is trickier - it would be easier if you'd reconsider not allowing compile time choices for tf2

What would you propose as a "compile time choice"? How would it work?

Overall, I'm happy to make changes here to make tf2 more standalone, but I don't want to introduce a lot of complexity either in tf2 itself or in the rest of the ecosystem to get there.

@kyle-basis
Copy link
Author

I don't have a desire to introduce complexity to TF either.

CMake - I want to do something like this:

FetchContent_Declare(
  tf2
  GIT_REPOSITORY https://github.com/ros2/geometry2.git
  SOURCE_SUBDIR tf2
)
FetchContent_MakeAvailable(tf2)

and have it compile as a standalone library. This might require setting some sort of option for CMake, but that's okay. I've given an example here - https://github.com/basis-robotics/geometry2/blob/generic_tf2_core_2/tf2/CMakeLists.txt#L14, but it would be even cleaner to separate out just the lines associated with add_library and move them to a separate .cmake file. In the end it's not horrible if I can't do this (I can instead duplicate the contents of the CMake file or maintain a fork), but in general standalone users won't want to use ament, as it comes with some baggage.

"compile time choice": - swapping TF to use a different logging macro would be one. Have RCUTILS_LOG_ERROR be instead used as TF_LOG_ERROR with a way of configuring the logging provider via CMake or some other method. Also though, this could be accomplished with just making logging part of the tf interface, and allowing derived classes to choose their own logger.

I can give more concrete examples of these in the coming days, if it would be easier.

@clalancette
Copy link
Contributor

and have it compile as a standalone library. This might require setting some sort of option for CMake, but that's okay. I've given an example here - https://github.com/basis-robotics/geometry2/blob/generic_tf2_core_2/tf2/CMakeLists.txt#L14, but it would be even cleaner to separate out just the lines associated with add_library and move them to a separate .cmake file. In the end it's not horrible if I can't do this (I can instead duplicate the contents of the CMake file or maintain a fork), but in general standalone users won't want to use ament, as it comes with some baggage.

So one thing we could consider, which has been done elsewhere in the ecosystem, is to try and find ament_cmake, and fall-back to using "standard" CMake if it is not available. You can see an example of that in action in https://github.com/facontidavide/PlotJuggler/blob/main/CMakeLists.txt , but it does add some complexity; there is a reason we use ament_cmake, after all. But it would be worthwhile exploring how much complexity it would add to the tf2 CMake file. I can't guarantee we'll take it (one of the great strengths of ROS is how consistent our CMakeFiles.txt are), but we can certainly evaluate it.

@kyle-basis
Copy link
Author

kyle-basis commented Nov 14, 2024 via email

@tfoote
Copy link
Contributor

tfoote commented Nov 15, 2024

This was a compromise we made early on. The library is designed to be independent but we didn't want to force the use of abstracting the datatypes completely and then forcing additional copies/conversions so the datatype is reused. Breaking tf2 out into a fully separate library is something that could be done, but so far we haven't had the demand for doing along with someone who's interested in taking on the additional effort and maintenance burden.


For the technical side of forking it and just getting it done. I've done this several times, and what you need to do is copy the few generated message datatypes from a ROS workspace and install them through another mechanism. And you do need the couple of utility dependencies. In this sort of situation just rewriting the build into your other projects approach is usually best. Which will eliminate ament (even if you stick to CMake, you won't have the ament linters, or necessarily the gtest facilities that we use in our standard ROS approach that could be disabled instead of trying to make them all work there. And for things like the logging macros. You can also just copy them into the same place that you put the copied headers for the messages.

@kyle-basis
Copy link
Author

kyle-basis commented Nov 15, 2024

Copying the ros types out is something I'm not excited to do - it means that now I always go through an additional copy, from the internal to the ROS type to my type. Additionally anything storing tf2 types or doing math on them without serializing them will use an additional copy. I'm happy to maintain my own CMake file internally, it's not a big deal - but putting aside my usecases, architecturally the ROS types really don't belong. There's code here https://github.com/basis-robotics/geometry2/blob/rolling/tf2/src/buffer_core.cpp#L704 that would be better served just calling tf2::toMsg but cannot due to the conversions being in a library that depends on tf2 - there would be a circular dependency. I'm going to make a PR next week showing a clean fix, it would be a win-win.

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

No branches or pull requests

4 participants