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

Add more callibration parameters to the pinhole camera model #31

Closed
wants to merge 26 commits into from

Conversation

recmo
Copy link

@recmo recmo commented Oct 25, 2020

No description provided.

for &camera_r in &test_values {
let corrected = distortion.calibrate(camera_r);
let reprojected = distortion.uncalibrate(corrected);
assert_eq!(camera_r, reprojected);
Copy link
Author

Choose a reason for hiding this comment

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

The inverse being exact happens to work on my machine, but may not be stable across architectures. I should replace this with a fuzzy comparison.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at this crate: https://docs.rs/approx/0.4.0/approx/.

If you don't mind, I actually would like this to go into the integration tests folder (tests folder) for better visibility since this doesn't test anything internal to the crate. I like to do that so people can see examples of usage there.

@@ -0,0 +1,161 @@
//! Radial distortion model

use num_traits::Float;
Copy link
Member

Choose a reason for hiding this comment

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

To get around the check failure on this line, just use Float::method(f). Basically, use the Float trait method using the static method syntax instead of the f.method() syntax, which will prevent any unused import errors. This happens because when std is enabled it uses those primitive methods rather than the ones on Float.

@vadixidav
Copy link
Member

@mpizenberg is currently in the process of updating everything to the latest nalgebra with const generics. Once that is merged, I will work on getting this PR ready. If you have any tips on what still needs to be done, just let me know.

@vadixidav
Copy link
Member

I did attempt to rebase this on current master, but I ran into an issue with rug. It seems that rug depends on a library called gmp-mpfr-sys, which seems to not support MSVC currently. Additionally, it utilizes a C library for arbitrary-precision floating point arithmetic. One of our goals is to be able to develop everything with minimal C dependencies, and also it is a goal to have cross-platform support. It is totally reasonable to create a separate crate which supports fisheye that is not part of the core cv mono-repo. At some point we should see if we can replace rug. Currently, there doesn't seem to be a pure-Rust alternative for arbitrary precision floating point arithmetic, but perhaps that is something we can work on, or perhaps we can work within the bounds of f64. For the time being, this code is valuable, but some of it might need to live outside of this repository until we resolve that issue.

@recmo
Copy link
Author

recmo commented Jul 12, 2021

At some point we should see if we can replace rug.

Note that it is a dev-dependency only used for testing, so it should not cause trouble outside of development of the crate itself, i.e. cross-platform support should not be an issue.

rug is used to compute reference values that are accurate to the full f64 precision so the tests can verify numerical accuracy of the library methods. I've found subtle losses of precision this way in the past, so I recommend testing accuracy.

Computing reference results is only necessary because test-cases are automatically generated using proptest. The alternative is to manually write a number of testcases with hard-coded reference results computed externally. This is the approach opencv uses. In my experience the test coverage you get this way is not as good as with proptest, but it is a viable way to get rid of the rug dev dependency. Another option is to simply feature-gate the dependency and relevant tests.

@vadixidav
Copy link
Member

At some point we should see if we can replace rug.

Note that it is only used for testing, so it should not cause trouble outside of development, i.e. cross-platform support should not be an issue.

rug is used to compute reference values that are accurate to the full f64 precision so the tests can verify numerical accuracy of the library methods. I've found subtle losses of precision this way in the past, so I recommend testing accuracy.

Computing reference results is only necessary because test-cases are automatically generated using proptest. The alternative is to manually write a number of testcases with hard-coded reference results computed externally. This is the approach opencv uses. In my experience the test coverage you get this way is not as good as with proptest, but it is a viable way to get rid of the rug dev dependency. Another option is to simply feature-gate the dependency and relevant tests.

Ahh, my bad, I assumed that rug was being used for the camera models themselves. In that case the use of rug is totally fine. I will pick this back up.

@vadixidav vadixidav deleted the branch rust-cv:master July 30, 2021 17:52
@vadixidav vadixidav closed this Jul 30, 2021
@vadixidav vadixidav mentioned this pull request Sep 5, 2021
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.

2 participants