-
Notifications
You must be signed in to change notification settings - Fork 734
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
radial point cloud nodes assume unrectified images #388
Comments
I started to look into this, and it turned into a bit of a rabbit hole: For starters - let's review which nodes use what (I'm only looking at the rolling branch for now, because any changes are likely to only go there):
Per the sensor_msgs/CameraInfo message:
There is quite a bit of good discussion on this answers.ros.org - but basically, it agrees with CameraInfo:
Looking back at the earlier history, xyz_radial and xyzi_radial were the first ones implemented - and they subscribe to image_raw - which would be appropriate when using K and D (as currently done in the code). It looks like when xyzrgb_radial was added it subscribes to an image_rect topics but still uses K and D matrix (that is inconsistent). To further complicate things, I just recently renamed image_raw->image_rect in those two nodes (#900 (comment)). So, I think I should revert the image_raw->image_rect in xyz_radial and xyzi_radial, and instead flip xyzrgb_radial to use image_raw - and add some docs that note these expect raw, rather than rectified images. This doesn't actually sort out the original issue mentioned here - which I presume to be a somewhat differently configured camera (one that is already rectified). |
PR to update topics: #906 |
Per findings in #388 (comment) - instead of renaming xyz_radial and xyzi_radial to image_rect, I should have made the xyzrgb_radial use image_raw (since these nodes use matrices K & D): * Revert the change in xyzi_radial - topic is depth/image_raw as it has always been * Revert the change in xyz_radial, although it is still changed slightly from the old "image_raw" -> "depth/image_raw" for consistency with the other nodes. * Update xyzrgb_radial: * depth_registered/image_rect -> depth/image_raw * rgb/image_rect_color -> rgb/image_raw * update launch files accordingly (and remove camera_info since it no longer needs to be renamed, happens automagically). Note: these launch files are probably epically bad since realsense doesn't output radial images... but we'll leave them as documentation for these nodes.
Per findings in ros-perception#388 (comment) - instead of renaming xyz_radial and xyzi_radial to image_rect, I should have made the xyzrgb_radial use image_raw (since these nodes use matrices K & D): * Revert the change in xyzi_radial - topic is depth/image_raw as it has always been * Revert the change in xyz_radial, although it is still changed slightly from the old "image_raw" -> "depth/image_raw" for consistency with the other nodes. * Update xyzrgb_radial: * depth_registered/image_rect -> depth/image_raw * rgb/image_rect_color -> rgb/image_raw * update launch files accordingly (and remove camera_info since it no longer needs to be renamed, happens automagically). Note: these launch files are probably epically bad since realsense doesn't output radial images... but we'll leave them as documentation for these nodes.
The depth_image_proc/point_cloud_xyz_radial and depth_image_proc/point_cloud_xyz_radial nodes apply a projective transformation to depth images to produce point clouds.
This projective transformation is calculated from the camera matrix K from the camera_info topic. In my opinion, this transformation should be calculated from the projection matrix P from the camera_info topic, if available, as well as from the rectification rotation matrix R. These can be provided to the cv::undistortPoints() function from within the initMatrix() method of the nodelet.
It would allow the camera_info topic to be fully utilized; I need a translation contained within the projection matrix to be applied to the point cloud before it is in the right frame.
Is the current behavior (only using K) intended or a bug?
The text was updated successfully, but these errors were encountered: