-
Notifications
You must be signed in to change notification settings - Fork 529
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
switch to tf2 #31
base: hydro-devel
Are you sure you want to change the base?
switch to tf2 #31
Conversation
Looks fine to me, but @tfoote should really review it, he's more familiar with the tf specific changes than me. |
Reading through it it looks reasonable. I opened an enhancement ticket to duplicate the getYaw method from tf. But you shouldn't wait for that. |
thx ! With all the tf2 conversions I've done, the only thing that could not be done simply is indeed the getYaw function (navigation uses it a lot). Will follow up on ros/geometry2#110 |
33dc6de
to
c6cea78
Compare
88c76b2
to
3c5cdc7
Compare
@vrabaud Is there any reason that this didn't ever get merged? |
@tfoote, can you please review ? Also, tests now run slower, is that to be expected from tf2 ? Thx |
gmapping/src/tftest.cpp
Outdated
tf_ = new tf::TransformListener(*node_, true, | ||
ros::Duration(10)); | ||
tf_.reset(new tf2::Buffer(ros::Duration(10))); | ||
tfl_ = new tf2_ros::TransformListener(*tf_, *node_, true); | ||
tf_->setExtrapolationLimit( ros::Duration().fromSec(0.2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setExtrapolationLimit was chosen not to be implemented in tf2 as there wasn't yet found a good use case for it in tf. I'm not sure that this unit will compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, that file has been useless forever actually.
@@ -4,5 +4,5 @@ | |||
<!--test time-limit="30" test-name="basic_localization_stage" pkg="gmapping" | |||
type="test_map.py" args="90.0"/--> | |||
<!-- TODO: Fix the rtest to work with replay, for now it's not working --> | |||
<test time-limit="250" test-name="map_data_test_replay" pkg="gmapping" type="gmapping-rtest" args="200.0 0.05 4000 4000 0.005 0.010"/> | |||
<test time-limit="300" test-name="map_data_test_replay" pkg="gmapping" type="gmapping-rtest" args="200.0 0.05 4000 4000 0.005 0.010"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to tf -> tf2 should not change the behavior. tf is using tf2 under the hood these days so if anything it should be slightly faster due to fewer function calls due to no indirection. This suggests that there's some timeouts or delays coming in somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
performance was actually very close to that limit (I have it to fail sometimes at that limit before this patch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you don't want to be right on the cusp anyway, that a forumla for flaky tests.
@tfoote , @wjwwood , if you have a few minutes to check that. Tests pass. Thx