Skip to content

Add arguments for estimateRigidTransform#11594

Closed
take1014 wants to merge 1 commit intoopencv:masterfrom
take1014:estimateRigidTransform_4270
Closed

Add arguments for estimateRigidTransform#11594
take1014 wants to merge 1 commit intoopencv:masterfrom
take1014:estimateRigidTransform_4270

Conversation

@take1014
Copy link
Copy Markdown
Contributor

@take1014 take1014 commented May 26, 2018

resolves #4270

@hrnr
Copy link
Copy Markdown
Contributor

hrnr commented May 26, 2018

there is already estimateAffinePartial2D and friends https://docs.opencv.org/master/d9/d0c/group__calib3d.html#gad767faff73e9cbd8b9d92b955b50062d

I think we should really get rid of estimateRigidTransform in OpenCV 4, or reimplement using estimateAffinePartial2D to avoid duplicate code.

terfendail
terfendail reviewed May 28, 2018
View reviewed changes
modules/video/test/test_estimaterigid.cpp
Mat aff_est2 = estimateRigidTransform(img, rotated, true, 500, 3, 0.5f);
norm = cvtest::norm(aff_est1, aff_est2, NORM_INF);
EXPECT_DOUBLE_EQ(norm, 0.);
Mat aff_est3 = estimateRigidTransform(img, rotated, true, 100, 8, 0.1f);
Copy link
Copy Markdown
Contributor

@terfendail terfendail May 28, 2018

Choose a reason for hiding this comment

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

It looks like the test checks the result doesn't depend on RANSAC parameters rather than its correctness. Wouldn't it be better to check the result against aff matrix?

Copy link
Copy Markdown
Contributor Author

@take1014 take1014 May 28, 2018

Choose a reason for hiding this comment

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

Thanks for the follow.
Certainly, I need to fix this code as you pointed out.
However, as alalek and hrnr say, I do not modify the interface of estimateRigidTransform, because we need to unify the "posture estimation" algorithm to calib3d's function instead of video module's.

@alalek
Copy link
Copy Markdown
Member

alalek commented May 28, 2018

I agree with @hrnr to keep "pose estimation" algorithms in a single place - calib3d module.
"Video" module should reuse existed functionality instead of re-implementing similar algorithms.

@take1014
Copy link
Copy Markdown
Contributor Author

take1014 commented May 28, 2018 •
edited
Loading

@alalek, @hrnr, @terfendail
Thanks for the comments.
I agree with you.
I think "estimateAffinePartial2D" should be reused instead of "estimateRigidTransform".

@take1014 take1014 closed this May 28, 2018
@take1014 take1014 deleted the estimateRigidTransform_4270 branch May 28, 2018 13:42
@hrnr
Copy link
Copy Markdown
Contributor

hrnr commented May 28, 2018

maybe we should deprecate estimateRigidTransform in OpenCV 4. The RANSAC part could be reimplemented using calib3d right now.

@hrnr hrnr mentioned this pull request Aug 18, 2018
Merged
@catree catree mentioned this pull request Feb 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terfendail terfendail terfendail left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RANSAC parameters for estimateRigidTransform

4 participants

@take1014 @hrnr @alalek @terfendail