tomas789 / kitti2bag

Convert KITTI dataset to ROS bag file the easy way!
MIT License
720 stars 261 forks source link

Refactor and simplify, replace argparse with click, add unit tests, change some topics & TFs #38

Open valgur opened 5 years ago

valgur commented 5 years ago

This PR touches nearly all parts of the code more or less. The main changes are:

I also introduced some changes to the created bags:

I hope I'm not causing too much of a headache by modifying such a large chunk of code at once. Feel free to let me know if you don't agree with any changes or would like to see something implemented differently.

valgur commented 5 years ago

Here's a minimal launch file to demo stereo_image_proc working after the camera topic adjustments:

<launch>
  <arg name="manager" default="disp_manager"/>

  <group ns="/kitti/camera_gray">
    <node pkg="nodelet" type="nodelet" name="$(arg manager)" args="manager" output="screen"/>
    <node pkg="nodelet" type="nodelet" name="disparity" output="screen"
          args="load stereo_image_proc/disparity $(arg manager)">
      <param name="approximate_sync" value="true"/>
    </node>
    <node pkg="nodelet" type="nodelet" name="point_cloud2" output="screen"
          args="load stereo_image_proc/point_cloud2 $(arg manager)">
      <remap from="left/image_rect_color" to="/kitti/camera_color/left/image_rect_color"/>
      <param name="approximate_sync"      value="true"/>
    </node>
  </group>

  <node pkg="image_view" type="stereo_view" name="stereo_view" output="screen">
    <remap from="stereo" to="/kitti/camera_gray"/>
    <remap from="image" to="image_rect"/>
    <param name="approximate_sync" value="true"/>
  </node>
</launch>
tomas789 commented 5 years ago

This PR looks like a great improvement. I went through the code quickly and it looks good to me. I especially like the dockerization of it as it adds a path to avoid the installation-related issues. I'm struggling a bit with why did you decide to go with the click instead of the built-in argparse as they both can do basically the same stuff but why not.

I will not be able to test the code but I'm happy to approve if somebody else says they tried and it worked. Or if you know somebody that could verify, I will add them as collaborators and they can approve that too.

Also, could you update the README.md file with examples on how to run it (preferably add an option without having to use the Docker as some guys might not be that familiar with that)?

valgur commented 5 years ago

why did you decide to go with the click instead of the built-in argparse

They are pretty similar, I agree. The main reasons why I preferred click:

valgur commented 5 years ago

I don't have any third parties to invite to review the code right now besides some colleagues, but that would not help much, I think.

I think most of the changes are fairly straightforward. The only change I'm not 100% sure about is the dynamic TF frame change from 'base_link' to 'imu_link'. I would like to reproduce the issue #11 and see if this change fixes it.

I'll update the readme later today. I think adding a changelog file would be a very good idea as well.