-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add Double Sphere and Extended Unified Camera models to Kalibr #210
Conversation
Fix double-sphere copy issue and visualization; adds omni-none See merge request basalt/kalibr!1
Some more fixes for ds model. See merge request basalt/kalibr!2
Comments, cleanup and minor fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. I tested the models and they seem to work well.
For visualization and undistortion I also have updated the image_undistort repo to support the new models. ethz-asl/image_undistort#36
|
||
outKeypoint[0] = x * norm_inv; | ||
outKeypoint[1] = y * norm_inv; | ||
//std::cout << "normalize\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the commented out debug code?
@@ -888,7 +887,7 @@ bool OmniProjection<DISTORTION_T>::estimateTransformation( | |||
Eigen::Vector3d backProjection; | |||
|
|||
if (keypointToEuclidean(imagePoint, backProjection) | |||
&& backProjection[2] > 0.0) { | |||
&& backProjection.normalized()[2] > std::cos(80.0*M_PI/180.0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this to a window of 80 degrees?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because at line 898-899 we divide by z to get pseudo-pinhole points and use them with cv::solvePnP at line 931. This is numerically unstable for angles close to 90 degrees (z is close to zero). 80 degrees was chosen experimentally such that it works on all datasets we used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We found that for fisheye lenses with close to or larger than 180 deg FoV Kalibr frequently fails with PNP errors without this change (try some of the calibration sequences from https://vision.in.tum.de/research/vslam/double-sphere) for the omni model (and also the new models). What seems to happen is that is uses points close to 90deg (but still positive z). I guess any value sufficiently smaller than 90deg would work, 80 is just heuristic and probably not optimal (it's what we used for the new models). The proper fix would be to use a PNP implementation that allows to give bearing-vectors as input and not pinhole-undistored 2d corrdinates. That way all points could be used, even with z<0. For example OpenGV has such an implementation.
(PS: We've also added the check to the pinhole model, although that doesn't work for the wide-angle lenses for other reasons, even though e.g. Kannala Brandt (pinhole-equi) models the lenses quite well. We assume its a combination of the way KB is implemented in Kalibr (as pinhole distortion, limiting it to FOV < 180), and also how initialization is done (starting optimization with a single image and consecutively adding more). The result is always that the optimization diverges at some point.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm @VladyslavUsenko was quicker...
@@ -365,8 +471,8 @@ def getAccelerometerStatistics(self): | |||
|
|||
def setAccelerometerStatistics(self, noise_density, random_walk): | |||
self.checkAccelerometerStatistics(noise_density, random_walk) | |||
self.data["accelerometer_noise_density"] = accelerometer_noise_density | |||
self.data["accelerometer_random_walk"] = accelerometer_random_walk | |||
self.data["accelerometer_noise_density"] = noise_density |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a bug previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it, yes. But it seems it's not used by anyone :-).
@ZacharyTaylor Thanks for the review and the undistorter! If this is merged, the wiki documentation should probably be updated. I guess relevant sections include: |
That looks great. Thanks a lot for the contribution. |
We would like to contribute two models from our 3DV2018 paper "The Double Sphere Camera model" to Kalibr.
Main benefits of the models:
We provide the evaluation scripts and results for the proposed models here: https://vision.in.tum.de/research/vslam/double-sphere#kalibr_example
Short summary of results: Overall proposed models perform similar/better than omni-none and pinhole-equi (which fails for lenses with FoV close to 180 degrees). omni-radtan has slightly smaller reprojection error but it has 8 parameters and no closed form inverse.