-
Notifications
You must be signed in to change notification settings - Fork 262
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
base: master
Are you sure you want to change the base?
Conversation
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.
Here's a minimal launch file to demo <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> |
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 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)? |
They are pretty similar, I agree. The main reasons why I preferred click:
|
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. |
Renamed 'i' -> 'intensity' http://docs.pointclouds.org/1.7.0/point__types_8hpp_source.html#l00380
Rviz is unable to find a transformation from base_link to other links otherwise.
This PR touches nearly all parts of the code more or less. The main changes are:
kitti2bag odom --color gray --sequence 1
. Click is also more convenient to write tests against.python setup.py test
orpytest
in the repo root.packages
parameter was not provoided). I think this fixes the occasional issue noted in the readme when kitti2bag is installed in non-editable mode.I also introduced some changes to the created bags:
base_link
toimu_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.