search

ros / common_msgs

Commonly used messages in ROS. Includes messages for actions (actionlib_msgs), diagnostics (diagnostic_msgs), geometric primitives (geometry_msgs), robot navigation (nav_msgs), and common sensors (sensor_msgs), such as laser range finders, cameras, point clouds.
http://wiki.ros.org/common_msgs
177 stars 191 forks source link

Add FISHEYE model definition #151

Closed DavidTorresOcana closed 3 years ago

DavidTorresOcana commented 4 years ago

Add FISHEYE model definition to be used in image_geometry model definition.

tfoote commented 4 years ago

This seems reasonable. Can you please add documentation for exactly what model this is implementing? We need to make sure that the model is specifically clarified.

DavidTorresOcana commented 4 years ago

This seems reasonable. Can you please add documentation for exactly what model this is implementing? We need to make sure that the model is specifically clarified.

The model is described in this OpenCV library.

It corresponds to a specific case of the radial distortion Kannala-Brandt model

DavidTorresOcana commented 4 years ago

This seems reasonable. Can you please add documentation for exactly what model this is implementing? We need to make sure that the model is specifically clarified.

@tfoote Is there any more doubts? There are several PRs awaiting for this to be passed.

As off today Melodic has a fisheye calibration tool but not a way to rectify with that calibration.

Please let us move this.

dirk-thomas commented 4 years ago

@DavidTorresOcana Please do not request reviews from as many people as you can think off (same for ros-perception/image_common#146 and ros-perception/vision_opencv#306). A simple comment on the ticket asking for attention is sufficient and usually makes sure the maintainer of the repository get a notification.

tfoote commented 4 years ago

As I mentioned in my review please add documentation to the code with a comment explaining it and a link. A comment here in this PR will be hard for people to find in the future.

DavidTorresOcana commented 4 years ago

As I mentioned in my review please add documentation to the code with a comment explaining it and a link. A comment here in this PR will be hard for people to find in the future.

Agree: See 06c96c3 Would that be ok?

mintar commented 4 years ago

Please do not merge this PR. The "fisheye" distortion model proposed in this PR is the same as the "equidistant" distortion model that we already have! The model is based on the following publication:

J. Kannala and S. Brandt (2006). A Generic Camera Model and Calibration Method for Conventional, Wide-Angle, and Fish-Eye Lenses, IEEE Transactions on Pattern Analysis and Machine Intelligence, vol. 28, no. 8, pp. 1335-1340

The "equidistant" model in sensor_msgs is exactly that: https://github.com/ros/common_msgs/pull/109#issuecomment-327615470

It's unfortunate that there are so many different names for the same distortion model:

But all of these are just different names for the same model. Therefore, I suggest we keep the existing name "equidistant". Maybe add some comment to the source code that mentions all the different names for it.

DavidTorresOcana commented 4 years ago

Please do not merge this PR. The "fisheye" distortion model proposed in this PR is the same as the "equidistant" distortion model that we already have! The model is based on the following publication:

J. Kannala and S. Brandt (2006). A Generic Camera Model and Calibration Method for Conventional, Wide-Angle, and Fish-Eye Lenses, IEEE Transactions on Pattern Analysis and Machine Intelligence, vol. 28, no. 8, pp. 1335-1340

The "equidistant" model in sensor_msgs is exactly that: #109 (comment)

It's unfortunate that there are so many different names for the same distortion model:

But all of these are just different names for the same model. Therefore, I suggest we keep the existing name "equidistant". Maybe add some comment to the source code that mentions all the different names for it.

Thank you for feedback. I agree camera model naming in CV field is chaotic. At the end the mathematical description is the best help and, as you pointed out, equidistant can do the job.

I agree to use EQUIDISTANT instead, so will update the pipeline accordingly.

The problem I see is that I developed the entire pipeline including the calibration tool ros-perception/image_pipeline#440 using FISHEYE which is being merged ros-perception/image_pipeline#542 We will have to update the calibration tool in those too

DavidTorresOcana commented 3 years ago

Closing this as the intended functionality was already available.