Reenable sensor_msgs test #422
Conversation
|
Thanks for splitting this out. CI:
|
Signed-off-by: Chris Lalancette <[email protected]>
|
@gleichdick FYI, please take a look at 7eb81ba and let me know what you think. I did a few additional cleanups in there. |
|
Thanks, your cleanup commit looks good! |
Signed-off-by: Chris Lalancette <[email protected]>
|
OK, Windows was complaining about stuffing doubles into floats. I don't have a great solution for that, so in 283cc4b, I ended up just doing a cast. That can lead to subtle problems in some situations, I guess, but I don't know what else to do there. Also, this is pre-existing behavior; we are just seeing it now because we added tests. Here's another round of CI:
|
|
@ahcorde This all looks good to me, but since I've made some non-trivial contributions here, I'd appreciate a second review from you. |
tf2_sensor_msgs/package.xml
Outdated
| @@ -25,10 +25,10 @@ | |||
| <depend>tf2_ros</depend> | |||
| <!-- <depend>python_orocos_kdl</depend> --> | |||
| <!-- <test_depend>rostest</test_depend> --> | |||
|
|
|||
| <exec_depend>tf2_ros_py</exec_depend> | |||
| <!-- <exec_depend>tf2_ros_py</exec_depend> --> | |||
tf2_sensor_msgs/package.xml
Outdated
| <!-- <depend>python_orocos_kdl</depend> --> | ||
| <!-- <test_depend>rostest</test_depend> --> |
| cloud, "B", tf2::durationFromSec( | ||
| 2.0)); |
There was a problem hiding this comment.
this looks wierd.
| cloud, "B", tf2::durationFromSec( | |
| 2.0)); | |
| cloud, "B", tf2::durationFromSec(2.0)); |
| sensor_msgs::msg::PointCloud2 cloud_advanced = tf_buffer->transform( | ||
| cloud, "B", tf2::timeFromSec(2.0), | ||
| "A", tf2::durationFromSec(3.0)); |
There was a problem hiding this comment.
| sensor_msgs::msg::PointCloud2 cloud_advanced = tf_buffer->transform( | |
| cloud, "B", tf2::timeFromSec(2.0), | |
| "A", tf2::durationFromSec(3.0)); | |
| sensor_msgs::msg::PointCloud2 cloud_advanced = tf_buffer->transform( | |
| cloud, "B", tf2::timeFromSec(2.0), "A", tf2::durationFromSec(3.0)); |
| t.header.stamp.sec = 2; | ||
| t.header.stamp.nanosec = 0; |
There was a problem hiding this comment.
Did you try this?
| t.header.stamp.sec = 2; | |
| t.header.stamp.nanosec = 0; | |
| t.header.stamp.sec = rclcpp::Time(2, 0); |
| cloud.header.stamp.sec = 2; | ||
| cloud.header.stamp.nanosec = 0; |
python scripts don't seem to be ported to ROS2 tho
|
Btw, closes #365 |
|
| @@ -23,12 +23,11 @@ | |||
| <depend>sensor_msgs</depend> | |||
| <depend>tf2</depend> | |||
| <depend>tf2_ros</depend> | |||
| <!-- <depend>python_orocos_kdl</depend> --> | |||
| <!-- <test_depend>rostest</test_depend> --> | |||
There was a problem hiding this comment.
I still think we should remove this. It's a ros1 dependency
There was a problem hiding this comment.
@gleichdick I will relaunch CI when this line will be removed
There was a problem hiding this comment.
I removed them, furhtermore some more flake8 issues are resolved which the CI found. Maybe we should add the flake8 modules mentioned here to the CI of this project (not the big one)?
There was a problem hiding this comment.
While I agree we should eventually do that, I don't think we should do that here. The next step for this package in my opinion is to split it up into separate tf2_sensor_msgs and tf2_sensor_msgs_py packages, and actually get the Python stuff working (which it currently isn't). At that point, it would make sense to enable the automated flake8 tests.
|
|
All right, we have two approvals and green CI. I'm going to merge this one, thanks for splitting it out @gleichdick . |
This was separated from #368