-
Notifications
You must be signed in to change notification settings - Fork 117
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
Migrate tf to tf2 #208
Migrate tf to tf2 #208
Conversation
[#534] PASSED on indigoAll tests passed
[#534] PASSED on kineticAll tests passed
[#534] PASSED on melodicAll tests passed
|
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
==========================================
+ Coverage 89.54% 90.03% +0.48%
==========================================
Files 17 17
Lines 1473 1465 -8
==========================================
Hits 1319 1319
+ Misses 154 146 -8
Continue to review full report at Codecov.
|
[#535] FAILED on indigo
[#535] FAILED on kinetic
[#535] FAILED on melodic
|
[#536] PASSED on indigoAll tests passed
[#536] PASSED on kineticAll tests passed
[#536] PASSED on melodicAll tests passed
|
@at-wat Please take a look when you can. |
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.
Some test code uses tf1.
It's nice to migrate them to tf2 and remove package dependency to tf1.
src/mcl_3dl.cpp
Outdated
@@ -614,10 +612,10 @@ class MCL3dlNode | |||
map_pos.x_ = f_pos_[0]->in(map_pos.x_); | |||
map_pos.y_ = f_pos_[1]->in(map_pos.y_); | |||
map_pos.z_ = f_pos_[2]->in(map_pos.z_); | |||
trans.setOrigin(tf::Vector3(map_pos.x_, map_pos.y_, map_pos.z_)); | |||
trans.setRotation(tf::Quaternion(map_rot.x_, map_rot.y_, map_rot.z_, map_rot.w_)); | |||
tf::vector3TFToMsg(tf::Vector3(map_pos.x_, map_pos.y_, map_pos.z_), trans.transform.translation); |
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.
would be better not to use tf::
functions.
#include <tf/transform_datatypes.h>
seems be removable if they are replaced.
[#537] PASSED on melodicAll tests passed
[#537] PASSED on indigoAll tests passed
[#537] PASSED on kineticAll tests passed
|
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.
@jojo43 If all tf1 functions are unused, please remove dependency to tf1 from package.xml and CMakeLists.txt.
@@ -37,6 +37,7 @@ | |||
#include <sensor_msgs/point_cloud2_iterator.h> | |||
#include <std_srvs/Trigger.h> | |||
#include <tf/transform_datatypes.h> |
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.
isn't it removable?
@@ -37,6 +37,7 @@ | |||
#include <sensor_msgs/point_cloud2_iterator.h> | |||
#include <std_srvs/Trigger.h> | |||
#include <tf/transform_datatypes.h> |
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.
same here
[#538] PASSED on melodicAll tests passed
[#538] PASSED on indigoAll tests passed
[#538] PASSED on kineticAll tests passed
|
[#539] PASSED on kineticAll tests passed
[#539] PASSED on melodicAll tests passed
[#539] PASSED on indigoAll tests passed
|
@at-wat |
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.
LGTM, thanks
sensor_msgs::PointCloud2 pc2_tmp; | ||
pcl::toROSMsg(*pc_local_accum_, pc2_tmp); | ||
geometry_msgs::TransformStamped trans = | ||
tfbuf_.lookupTransform(frame_ids_["base_link"], pc_local_accum_->header.frame_id, ros::Time(0)); |
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.
stamp should not be 0. there are several same bugs.
return; | ||
} | ||
geometry_msgs::TransformStamped trans = | ||
tfbuf_.lookupTransform(frame_ids_["odom"], msg->header.frame_id, ros::Time(0)); |
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.
in.y = acc_measure.y_; | ||
in.z = acc_measure.z_; | ||
geometry_msgs::TransformStamped trans = | ||
tfbuf_.lookupTransform(frame_ids_["base_link"], msg->header.frame_id, ros::Time(0)); |
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.
// Static transform | ||
tfl_.lookupTransform(frame_ids_["base_link"], msg->header.frame_id, ros::Time(0), trans); | ||
trans = tfbuf_.lookupTransform(frame_ids_["base_link"], msg->header.frame_id, ros::Time(0)); |
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.
(this and https://github.com/at-wat/mcl_3dl/pull/208/files#r238511052 should be always static transform (base_link->imu_link) in most case. timestamp is not mandatory to be treated here.)
return; | ||
} | ||
sensor_msgs::PointCloud2 pc2_tmp; | ||
pcl::toROSMsg(*pc_local_accum_, pc2_tmp); |
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.
toROSMsg -> transform -> fromROSMsg is not good at the performance
Fix #191