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

Add Tf2 versions of lookupTransform, lookupVelocity, setTransform #728

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

Conversation

kyle-basis
Copy link

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

This PR implements #726 - see there for initial discussion.

Some notes:

  • No response was given for naming, so I went with *Tf2.
  • I'm not totally pleased with lookupVelocity - tf2 doesn't have a dual vector type that I can find, and I didn't really want to implement a new Velocity for this PR. I'm happy to back that part of the change out - the function can be moved wholesale into tf2_ros when the time comes, instead, as it's not part of BufferCoreInterface.
  • I combined the two manual reimplementations of toMsg into one reimplementation - it really shows that the interface is wrong here in the first place - the helper methods that should be used would cause a depedency loop.
  • tf2::Transform doesn't have a child_frame_id - I think I did the correct thing, but it might be worth introducing somehow.
  • I'm unable to actually run the tests - I get an error on colcon build
  • My version of colcon doesn't appear to have filter support, testing is a little hard. I think the solution here is to use an underlay, will experiment

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.

@tfoote do you mind to take a look ?

TF2_PUBLIC
geometry_msgs::msg::VelocityStamped lookupVelocity(
const std::string & tracking_frame, const std::string & observation_frame,
const TimePoint & time, const tf2::Duration & averaging_interval) const;

/** \brief Lookup the velocity of the moving_frame in the reference_frame
* \param reference_frame The frame in which to track
Copy link
Contributor

Choose a reason for hiding this comment

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

update docs

Copy link
Author

Choose a reason for hiding this comment

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

I think I've fixed this

@kyle-basis
Copy link
Author

Only test fails are due to linting - do y'all have a script to fix this automatically?

@clalancette
Copy link
Contributor

Only test fails are due to linting - do y'all have a script to fix this automatically?

I think you can run ament_uncrustify --reformat to fix it.

@kyle-basis
Copy link
Author

kyle-basis commented Nov 13, 2024

Linting fixed - not sure why tf2_rospy is failing on jenkins, though

@MichaelOrlov
Copy link

@tfoote do you mind to take a look ?

@tfoote Friendly ping for review.

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.

4 participants