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

camera_calibration: Fix all-zero distortion coeffs returned for a rational_polynomial model #433

Merged

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jul 18, 2019

Using --k-coefficients greater than 3 with cameracalibrator.py currently returns a distortion coefficients array of all zeroes. I suppose this could be related to OpenCV version, which is 4.1.0 on my machine.

This is fixed by simply using the return values of cv2.calibrateCamera() instead of output variables to retrieve the distortion coefficients output.

Using an output variable to return values from cv2.calibrateCamera()
does not appear to work in OpenCV 4.1.
@valgur valgur changed the title camera_calibration: Fix empty distortion coeffs returned for a rational_polynomial model camera_calibration: Fix all-zero distortion coeffs returned for a rational_polynomial model Jul 18, 2019
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some clarity needed + a link to something documenting the bug this is resolving would be great

camera_calibration/src/camera_calibration/calibrator.py Outdated Show resolved Hide resolved
camera_calibration/src/camera_calibration/calibrator.py Outdated Show resolved Hide resolved
@valgur
Copy link
Contributor Author

valgur commented Jul 18, 2019

some clarity needed + a link to something documenting the bug this is resolving would be great

I have not created an issue regarding this. Should I create one?

@valgur
Copy link
Contributor Author

valgur commented Jul 18, 2019

Might want to merge #341 as well, if this one gets approved. That PR is buggy (tries to concatenate an int to str), so I included that fix here instead.

@SteveMacenski
Copy link
Member

I suppose this could be related to OpenCV version

I thought you had tracked down this is an OpenCV issue, if it is, you should definitely file a bug with them.

@valgur
Copy link
Contributor Author

valgur commented Jul 23, 2019

I took a closer look at the calibrateCamera() behavior regarding distCoeffs.
The distCoeffs returned from the function are always correct. Using distCoeffs as an input-output array only outputs the correct values when the shape and dtype of the input matches the dtype and shape of the expected output from the function, though. Looks like the array gets re-allocated in C++ and the reference to the input numpy array discarded.

I suppose this could be considered an OpenCV bug, but since the more typical and reliable way of retrieving the output via a return value works, I'm not sure it's worth creating a bug report for this.

@SteveMacenski
Copy link
Member

Fair enough. This looks good to me, can you give me a "yes I have tested this sufficiently to my liking to be comfortable merging this" statement? I don't have the hardware at this moment on travel to evalulate.

Then merging is OK by me with a second approval from probably @JWhitleyAStuff

@valgur
Copy link
Contributor Author

valgur commented Jul 25, 2019

Sure - yes, I would be comfortable merging this. The changes are quite straightforward and the unit tests pass on both my machine and the CI. I also checked the stereo calibration code for similar issues and did not find anything there.

@SteveMacenski
Copy link
Member

Ok, did you test this on an actual camera in real life? CI is good, but hardware is great.

@valgur
Copy link
Contributor Author

valgur commented Jul 26, 2019

Yep, I have tested and used it on real camera data. Both --k-coefficients 3 and >3 are working as expected.

@JWhitleyWork
Copy link
Collaborator

I'm honestly not familiar enough with what's going on in this PR to provide my stamp of approval so I'll leave this one up to @SteveMacenski. Merge at your leisure.

@SteveMacenski
Copy link
Member

@JWhitleyAStuff even without being familiar with the thing happening under the hood, I think its clear to show that instead of an implicit-output, its having a hard output and for whatever reason in the Python wrapper for OpenCV that solves this issue (which a little different data representation in numpy) I dont see anything here that changes an algorithm or usage so I'm OK merging it if its been tested to work on hardware such that all the places with the new dimensions are correct

@SteveMacenski SteveMacenski merged commit f1de5b0 into ros-perception:melodic Jul 29, 2019
wep21 pushed a commit to wep21/image_pipeline that referenced this pull request Oct 17, 2021
…ional_polynomial model (ros-perception#433)

* Fix empty distortion coeffs returned for a rational_polynomial model

Using an output variable to return values from cv2.calibrateCamera()
does not appear to work in OpenCV 4.1.

* Remove the redundant distCoeffs parameter from cv2.calibrateCamera()

* Set the shape of distortion_coefficients correctly in YAML output

Fixes ros-perception#278
JWhitleyWork pushed a commit that referenced this pull request Nov 11, 2021
…ional_polynomial model (#433)

* Fix empty distortion coeffs returned for a rational_polynomial model

Using an output variable to return values from cv2.calibrateCamera()
does not appear to work in OpenCV 4.1.

* Remove the redundant distCoeffs parameter from cv2.calibrateCamera()

* Set the shape of distortion_coefficients correctly in YAML output

Fixes #278
JWhitleyWork pushed a commit that referenced this pull request Apr 14, 2022
…ional_polynomial model (#433)

* Fix empty distortion coeffs returned for a rational_polynomial model

Using an output variable to return values from cv2.calibrateCamera()
does not appear to work in OpenCV 4.1.

* Remove the redundant distCoeffs parameter from cv2.calibrateCamera()

* Set the shape of distortion_coefficients correctly in YAML output

Fixes #278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants