MRPT / mrpt

:zap: The Mobile Robot Programming Toolkit (MRPT)
https://docs.mrpt.org/reference/latest/
BSD 3-Clause "New" or "Revised" License
1.94k stars 628 forks source link

Camera calibration bug regarding number of distortion coefficients #169

Closed CBaiz closed 8 years ago

CBaiz commented 8 years ago

Hello, there is a bug in the camera calibration tool. OpenCV returns either 5 or 8 distortion coefficients (dependent on the flag CV_CALIB_RATIONAL_MODEL), regardless of the size of the passed cv::Mat object. However, in line 310 and 323 in the file checkerboard_cam_calib.cpp, only 4 values are regarded:

... cv::Mat cameraMatrix, distCoeffs(1,4,CV_64F,cv::Scalar::all(0)); ... for (int i=0;i<4;i++) out_camera_params.dist[i] = distCoeffs.ptr()[i];

I've actually checked back with OpenCV to confirm this (see http://answers.opencv.org/question/74243/confusion-about-number-of-distortion-parameters/). In situations where the 5th parameter is important to the distortion, the calibration suffers from a significant loss of quality (see appended image)

1

jlblancoc commented 8 years ago

Thank you very much for the feedback and for checking everything before posting!

Now that you have those images at hand, would you mind modifying the code yourself? You can do a fork and pull request in GitHub. Let me know if this is fine for you...

CBaiz commented 8 years ago

I'm trying to build the project with CMake. If I'm successful, I can send a pull request.

CBaiz commented 8 years ago

I've successfully fixed the bug. I'll send a pull-request shortly, never done one before. Also, I wasn't able to debug the application as it always crashed in the function "tokenize" in string_utils.cpp when calling "free(dupStr)", claiming dupStr not to be a pointer to heap-allocated space (which is a lie). However, I was able to run a release build and test the bugfix and it's looking good. Comparison (new vs old): versionwithoutbug versionwithbug

jlblancoc commented 8 years ago

Brilliant!! Thanks a lot for the contribution :+1:

If you want to put your real name & affiliation in the changelog, go and edit it. See last bug fix in https://github.com/MRPT/mrpt/blob/master/doc/doxygen-pages/changeLog_doc.h

CBaiz commented 8 years ago

That's quite alright, my name in the commit suffices :)