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

radial point cloud nodes assume unrectified images #388

Open
madsherlock opened this issue Mar 4, 2019 · 2 comments
Open

radial point cloud nodes assume unrectified images #388

madsherlock opened this issue Mar 4, 2019 · 2 comments

Comments

@madsherlock
Copy link

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?

@mikeferguson
Copy link
Member

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):

  • xyz, xyzi, xyzrgb use image_geometry::PinholeCameraModel - which means they actually use the P matrix
  • xyz_radial, xyzi_radial, xyzrgb_radial use a transform matrix derived from K and D.

Per the sensor_msgs/CameraInfo message:

  • K is the "Intrinsic camera matrix for the raw (distorted) images"
  • P "specifies the intrinsic (camera) matrix of the processed (rectified) image"
  • R is the "Rectification matrix (stereo cameras only)" and is "A rotation matrix aligning the camera coordinate system to the ideal stereo image plane so that epipolar lines in both stereo images are parallel."

There is quite a bit of good discussion on this answers.ros.org - but basically, it agrees with CameraInfo:

  • If you are subscribing to image_raw, use K and D
  • If you are subscribing to image_rect, use P and set D to the identity matrix.

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).

@mikeferguson
Copy link
Member

PR to update topics: #906

mikeferguson added a commit that referenced this issue Jan 22, 2024
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.
@mikeferguson mikeferguson changed the title depth_image_proc/point_cloud_xyz_radial Projection matrix not used radial point cloud nodes assume unrectified images Feb 9, 2024
Kotochleb pushed a commit to Kotochleb/image_pipeline that referenced this issue May 27, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants