kornia / kornia

Geometric Computer Vision Library for Spatial AI
https://kornia.readthedocs.io
Apache License 2.0
9.9k stars 966 forks source link

`unproject` function in PinholeCamera class does not have `normalize` argument as stated in the docs #2006

Open Hugstar opened 1 year ago

Hugstar commented 1 year ago

In the documentation for the unproject function in PinholeCamera class it states there's a 3rd argument, normalize. This argument does not appear in the function source code.

edgarriba commented 1 year ago

we need to add the two following lines: https://github.com/kornia/kornia/blob/5f7e076aab5aa7788ad057efe989f17026517557/kornia/geometry/camera/perspective.py#L76-L77

I'm seeing that we recently changed the implementation, probably we should target to standardize this a bit again. We have two separated implementation of point unprojection to the z=1 plane that could be coupled as follows in the new version of CameraModel (towards supporting other than pinhole). @Hugstar what i your use case for normalize ? I remember to add at some point because in some data coming from simulators depth was in a different representation.

Also, @ducha-aiki could you remind me why was this needed ? normalize_points_with_intrinsics https://github.com/kornia/kornia/blob/master/kornia/geometry/conversions.py#L1246 -- i believe a better naming for this process should be just undistort_points_z1 or undistort_points_affine as suggested here:

I would like to target something like @YanivHollander

def CameraModel::unproject_point(self, pixel_in_image: Tensor, depth_z: Tensor, normalize: bool = False) -> Tensor:
   xyz = stack((undistort_points_z1(pixel_in_image, *self.coefs), depth_z), -1)
   if normalize:
       xyz = F.normalize(xyz, ....)
   return xyz

def CameraModel::coefs(self) -> Tuple[Tensor, Tensor, Tensor, Tensor]:
    return self.fx, self.fy, self.cx, self.cy

def undistort_points_z1(pixel_in_image: Tensor, fx: Tensor, fy: Tensor, cx: Tensor, cy: Tensor) -> Tensor:
    # unpack coordinates
    # NOTE: at some point i want  to introduce `Vec2` to access point_2d.x / point_2d.y to avoid wrong random access
    u_coord: Tensor = point_2d[..., 0]
    v_coord: Tensor = point_2d[..., 1]

    x_coord: Tensor = (u_coord - cx) / fx
    y_coord: Tensor = (v_coord - cy) / fy

    return stack([x_coord, y_coord], -1)
ducha-aiki commented 1 year ago

@edgarriba while I might be technically wrong here, something inside me hugely protests about calling undistort a function , which does only affine transformation of a coordinates, such as X=fx * x + cx. I kind of expect at least radial distortion parameters in undistort function. That is why I called my function is that PR normalize_with_intrinsics

YanivHollander commented 1 year ago

@edgarriba, do you suggest I would add these two lines for normalize in the current PinholeCamera::unproject_points?, or this should go to a new and more general CameraModel class.

Also, there is something a bit confusing with the arguments depth and normalize. I think the first argument should be named distance, which can represent either the ray from the camera origin to the point in 3D; or the projection of this ray along the camera axis. The second argument would indicate which of the two are attributed to distance. We can use an enum for this second argument, and name it distance_type, or something similar

edgarriba commented 1 year ago

@ducha-aiki @YanivHollander moving this discussion to https://github.com/kornia/kornia/issues/2011

@Hugstar feel free also to join the conversation. As per the actual issue, for now we can quickly hack as I suggested before we re-design the api