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

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

valgur
Copy link
Collaborator

@valgur valgur commented May 10, 2019

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

  • Cleaner separation of raw and odom code. Moved all raw/odom specific code out of the message creation functions. pykitti helped a lot here by abstracting away many of the small differences between the datasets.
  • Replaced argparse with click. Click allows the raw and odom dataset handling to be moved to separate sub-commands with different parameters, e.g. kitti2bag odom --color gray --sequence 1. Click is also more convenient to write tests against.
  • Added unit tests. The tests download and cache a single synced raw drive and calibrations to run tests against. Odometry dataset is somewhat faked by using camera images from the raw drive. The unit tests verify that the program runs without errors and that the output is reasonable - that expected topics exist with the correct number of messages. The tests can be run via python setup.py test or pytest in the repo root.
  • Made the ROS distro configurable in Dockerfile. Changed the base image to 'perception' since it contains all the required packages and the resulting full image size is nearly the same (~1.2 GB) but the Dockerfile builds much faster.
  • Use the Dockerfile for running tests in Travis-CI. Configured Travis to test all of the recent ROS distros (with the exception of Indigo, where the Python version is too old for some dependencies and I don't see much point in trying to support it).
  • Fixed a setup.py installation issue (the required packages parameter was not provoided). I think this fixes the occasional issue noted in the readme when kitti2bag is installed in non-editable mode.
  • The odom dataset handling was quite broken with newer pykitti versions.

I also introduced some changes to the created bags:

  • Adjusted the information in CameraInfo to follow the ROS conventions for it:
    • Set the camera TF frames to the common rectified camera frame (that is, the frame of rectified camera 0). No pose information is lost since the pose of the cameras relative to the rectified camera frame is encoded in each camera's projection matrix P. The stereo_image_proc package expects this convention to be followed, in particular.
    • Renamed the RGB camera topics to 'image_rect_color'.
    • Set the rectifying rotation matrix R to identity since the cameras are already rectified.
    • Do not include the distortion parameters D, since the images are already undistorted.
  • Renamed 'camera_gray_left' -> 'camera_gray/left', etc. so the camera topics can be used more easily with stereo_image_proc.
  • Changed the dynamic TF frame id from base_link to imu_link which makes more sense to me since the data describes the IMU rather than the vehicle base (related to Problem with OXTS and transforms #24, Set OXTS poses to be in imu_link frame #25). Also corrected the dynamic TF frame id for odom to match the camera0 tf name used elsewhere.

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 and others added 18 commits April 30, 2019 16:57
The ImportError can provide some useful information when a transitive
dependency is missing, for example.
* The D and distortion_model parameters should not be added since the
images are already rectified.
* R is covered by P_rect and is redundant.
* K is included for convenience, even though it could also be directly extracted from P_rect.

* Simplified code to the point that any 'raw' and 'odom' branches outside the top-level functions are eliminated.
* Changed the odom IMU child TF frame name to match camera0's.
Test data is downloaded and cached in tests/data.
Also fixed a bug with odom timestamps.
Skip installing it explicitly since gets installed as a cv_bridge dependency anyway.
pykitti abstracts away quite a few differences between raw and odom, which is convenient.
Having separate 'raw' and 'odom' subcommands makes the CLI much cleaner.
Click is also easier to use in tests.

Also added 'both' as an odometry color option
perception includes all relevant ROS packages for kitti2bag.
The resulting image size is nearly the same but the kitti2bag layers are smaller and Dockerfile builds faster (good for Travis-CI).
Made adjustments to follow the conventions from
http://docs.ros.org/melodic/api/sensor_msgs/html/msg/CameraInfo.html

* Set the camera TF frames to the rectified camera frame (that is, the frame of rectified camera 0). The pose info of the cameras relative to the rectified camera frame is encoded in each camera's projection matrix P. The stereo_image_proc package expects this convention to be followed, in particular.
* Renamed the RGB camera topics to 'image_rect_color'.
* Set the rectifying rotation matrix R to identity since the cameras are already rectified.
* Do not include the distortion parameters D, since the images are already undistorted.
@valgur
Copy link
Collaborator Author

valgur commented May 14, 2019

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>

@valgur valgur requested a review from tomas789 May 14, 2019 07:05
@tomas789
Copy link
Owner

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
Copy link
Collaborator Author

valgur commented May 14, 2019

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:

  • The CLI can be used as a Python API as well since click defines the arguments as parameters to a function. My goal was to separate the raw and odom related code to separate functions anyway and this aligned well with click's subcommands.
  • Click provides a test runner, something that is lacking from argparse. This is a lot more convenient to use (the CLI output and exceptions are captured automatically, for example) and robust than the alternatives, like creating a subprocess to test the CLI.
  • I simply like click's API design and functionality better, personally.

@valgur
Copy link
Collaborator Author

valgur commented May 14, 2019

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.

Rviz is unable to find a transformation from base_link to other links otherwise.
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.

2 participants