search

DOI-USGS / usgscsm

This repository stores USGS Community Sensor Model (CSM) camera models
Other
26 stars 33 forks source link

Fix for radial-tangential distortion #484

Closed oleg-alexandrov closed 5 months ago

oleg-alexandrov commented 6 months ago

The OpenCV radial-tangential distortion model I added a few months ago is useful on the ASP side and helps with combining data from multiple vendors (and sensor types) under the CSM umbrella.

I found there is a bug, however, in my implementation, that is due to insufficiently clear OpenCV documentation regarding lens distortion at: https://docs.opencv.org/4.x/dc/dbb/tutorial_py_calibration.html

That does not say that distorted and undistorted pixels must be normalized by focal length. I found this bit by looking at another doc page, at https://docs.opencv.org/4.x/d9/d0c/group__calib3d.html

I put a fix and validated that this agrees with OpenCV's implementation. Before, I had tested only the distortion/undistortion formula, rather than the full chain of world-to-pixel transform, that's why this bug went undetected.

The existing CSM lens distortion formulas do not normalize by focal length. So this fix required breaking the interface of the distort/undistort functions, as now they need to know about the focal length, to apply/undo this normalization.

I fixed the tests that broke as result of this and updated the API as needed.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

oleg-alexandrov commented 5 months ago

Any thoughts? I use the radial-tangential distortion model quite a bit, and is likely the default go-to model for a generic sensor.

acpaquette commented 5 months ago

@oleg-alexandrov appreciate the second ping, this got buried in my inbox. My immediate thought is could the focal length be put into the coefficients list for use within the distortion algorithm? I know it wouldn't be an actual coefficient but it would prevent the API from being broken. I am also not sure if this would constitute a breaking change, considering it is an internal function that is not exposed to users

jlaura commented 5 months ago

In my experience pushing heterogeneous things into a vector significantly reduces the readability and accessibility of the code base. Isn't this a bug fix?

oleg-alexandrov commented 5 months ago

The focal length could be made an optional last argument if need be, but I don't see a good reason for that.

This is indeed an internal function, so changing the API should not break anything.

There is a very good reason as to why a distortion function must know the focal length. This assures that the distortion is applied correctly independently of image resolution. If you put a new sensor of higher resolution inside of your camera, but keep the same lens, the distortion should function as before.

I understand your reluctance to break the API, but I don't think this is a concern for this particular function.

acpaquette commented 5 months ago

All fair points, I guess it just seems weird that none of the other distortion algorithms for both applying and removing distortion care about the focal length.

It makes a little more sense that a generic distortion model would.

oleg-alexandrov commented 5 months ago

I think an orbital sensor is usually a very custom thing, with fixed resolution, to be calibrated as the chief scientist wishes. People running bots on the ground want the flexibility of swapping the "eyes" of their machines with predictable consequences.