Skip to content

Recover pose from different cameras#17981

Closed
tompollok wants to merge 1 commit intoopencv:masterfrom
tompollok:recoverPoseFromDifferentCameras
Closed

Recover pose from different cameras#17981
tompollok wants to merge 1 commit intoopencv:masterfrom
tompollok:recoverPoseFromDifferentCameras

Conversation

@tompollok
Copy link
Copy Markdown
Contributor

@tompollok tompollok commented Jul 29, 2020

related to #17329 in order to get essential matrix and R and t for two different cameras

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under OpenCV (BSD) License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
force_builders_only=linux,docs

@tompollok
Copy link
Copy Markdown
Contributor Author

@vpisarev i added another interface for directly recovering R and t from different cameras.

@asmorkalov asmorkalov requested a review from vpisarev July 30, 2020 09:51
@tompollok tompollok changed the title Recover pose from different cameras [WIP] Recover pose from different cameras Jul 30, 2020
@tompollok
Copy link
Copy Markdown
Contributor Author

@vpisarev i think its ready for review.

The only thing that is missing are the tests. How should this be tested? I think i could compare the rodruigez vectors given a simple dataset, similar to what you did for #17816 as well as scaling the translation to a unit vector and then compare the directions of two different recoverPose functions?

return findEssentialMat(_pointsUntistorted1, _pointsUntistorted2, cm0, method, prob, threshold, _mask);
}

int cv::recoverPose( InputArray _points1, InputArray _points2,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what i dont like is the mixed use of sometimes using _ prefix for parameters and sometimes for variables. It seems to be mixed a lot in the whole code. Is there a guideline when to exactly use _ and when not to?

@tompollok tompollok changed the title [WIP] Recover pose from different cameras Recover pose from different cameras Jul 31, 2020
@asmorkalov asmorkalov added the pr: needs test New functionality requires minimal tests set label Jul 31, 2020
@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Friendly reminder about test.

@tompollok
Copy link
Copy Markdown
Contributor Author

Sorry i wont have time the next 2 weeks. I can do it in the last week of august

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok There is merge conflict still. Could you squash commits and rebase to current master?

@tompollok
Copy link
Copy Markdown
Contributor Author

yes ill fix it today

@tompollok tompollok force-pushed the recoverPoseFromDifferentCameras branch from 1845f0d to 9c5c137 Compare August 31, 2020 16:39
@tompollok tompollok force-pushed the recoverPoseFromDifferentCameras branch from 9c5c137 to aa16dde Compare August 31, 2020 16:43
@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok @vpisarev Do you plan to finish the PR?

@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Nov 30, 2020
length.
@param method Method for computing an essential matrix.
- **RANSAC** for the RANSAC algorithm.
- **LMEDS** for the LMedS algorithm.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use doxygen @ref instead of bold font for RANSAC and LMEDS.

@vpisarev
Copy link
Copy Markdown
Contributor

@tompollok, thank you for the contribution! 👍

points2.at<Point2f>(12) = Point2f(0.0f, 0.0f); // provoke an outlier detection
Inliers = recoverPose(E, points1, points2, cameraMatrix, R, t, mask);
EXPECT_EQ(0, (int)mask.at<unsigned char>(12)) << "Detecting outliers in function failed, testcase " << testcase;
Inliers2 = recoverPose(points1, points2, cameraMatrix, zeroDistCoeffs, cameraMatrix, zeroDistCoeffs, E3, R, t, RANSAC, 0.999, 1.0, mask);
Copy link
Copy Markdown
Member

@alalek alalek Dec 23, 2020

Choose a reason for hiding this comment

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

cameraMatrix, zeroDistCoeffs, cameraMatrix, zeroDistCoeffs

Test passes the same camera intrinsics for this call anywhere.
It doesn't validate the added functionality ("different cameras").

@vpisarev
Copy link
Copy Markdown
Contributor

@tompollok, could you, please, fix the builds and change the test so that the intrinsic cameras are different (so that the new functionality is actually tested)?

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Friendly reminder.

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Have you had a chance to finish the patch?

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Friendly reminder.

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Friendly reminder. Please tune the test to use really different cameras that is stated in the PR message.

@asmorkalov
Copy link
Copy Markdown
Contributor

@tompollok Do you have a chance to finish the patch?

@tompollok
Copy link
Copy Markdown
Contributor Author

@asmorkalov ill take a look. Sorry ive been very busy lately. but ill try to fix it in time for the next release.

@asenyaev
Copy link
Copy Markdown
Contributor

asenyaev commented Apr 8, 2021

jenkins cn please retry a build

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Aug 3, 2021

@tompollok Friendly reminder.

@thezane
Copy link
Copy Markdown
Contributor

thezane commented Aug 25, 2021

If @tompollok and opencv maintainers are ok with it, I would like to finish this PR as I'm interested in it. It has been open for a year and it would be good if users had access to this functionality.

It appears that only unit tests need to be fixed. What does everyone think about that?

@asmorkalov
Copy link
Copy Markdown
Contributor

@thezane please go ahead. Please presume commits history and original author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants