Skip to content

Extend USAC coverage. #22884

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 45 commits into
base: 4.x
Choose a base branch
from
Open

Extend USAC coverage. #22884

wants to merge 45 commits into from

Conversation

tfunatomi
Copy link
Contributor

@tfunatomi tfunatomi commented Nov 29, 2022

Add estimateSE2(...), estimateSE3(...), estimateSIM2(...), estimateSIM3(...) for estimating an geometric transformation with rotation and translation (with scaling for SIM) using USAC: as alternative for estimateAffinePartial2D and estimateAffine3D.

Problem

Nice feature based on USAC: RANSAC-based universal framework has been introduced for the following functions.

However, the following functions are not supported and only support legacy methods (RANSAC and LMEDS).

Meanwhile, these functions provided to estimate a constrained geometric transformations consists of rotation, translation, and scaling with appropriate options.

Proposal

I really needed to estimate for the transformation in the class of Rigid/Similarity transform in 2D and 3D, denoted as $\mathit{SE}(d), \mathit{SIM}(d)$, where $d=2,3$.
In order to clarify the class of the transformation of the output, I propose to add the functions of estimateSE2(...), estimateSE3(...), estimateSIM2(...), estimateSIM3(...) as alternative for estimateAffinePartial2D and estimateAffine3D. These functions takes arguments compatible with the existing functions such as estimateAffine2D.

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 Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

Add `estimateSE2(...)`, `estimateSE3(...)`, `estimateSIM2(...)`, `estimateSIM3(...)` for estimating an geometric transformation with rotation and translation (with scaling for SIM) using USAC: as alternative for `estimateAffinePartial2D` and `estimateAffine3D`.
@alalek alalek mentioned this pull request Nov 29, 2022
6 tasks
@asmorkalov asmorkalov requested a review from vpisarev November 29, 2022 16:02
@tfunatomi
Copy link
Contributor Author

@alalek Thank you for your reaction.
I did not understand where the test modules are, but my colleague helped to identify it.
I added some modules to test my code and I've gotten the results as follows.

[----------] 1 test from usac_SE2
[ RUN ] usac_SE2.accuracy
[ OK ] usac_SE2.accuracy (579 ms)
[----------] 1 test from usac_SE2 (579 ms total)

[----------] 1 test from usac_SIM2
[ RUN ] usac_SIM2.accuracy
[ OK ] usac_SIM2.accuracy (575 ms)
[----------] 1 test from usac_SIM2 (575 ms total)

[----------] 1 test from usac_SE3
[ RUN ] usac_SE3.accuracy
[ OK ] usac_SE3.accuracy (1362 ms)
[----------] 1 test from usac_SE3 (1362 ms total)

[----------] 1 test from usac_SIM3
[ RUN ] usac_SIM3.accuracy
[ OK ] usac_SIM3.accuracy (1569 ms)
[----------] 1 test from usac_SIM3 (1569 ms total)

@tfunatomi tfunatomi force-pushed the usac-se branch 2 times, most recently from eaba327 to 30514d4 Compare November 30, 2022 07:11
@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Nov 30, 2022
@asmorkalov asmorkalov requested a review from savuor December 6, 2022 09:17
Copy link
Contributor

@savuor savuor left a comment

Choose a reason for hiding this comment

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

It's fully correct, it works and it should be merged. But:

  • Why Partial solver is disabled? I've removed it until it's not in use.
  • Weights are supported by the algorithm but never actually used. I think we may extend the interface by supporting weights (maybe in next PR?). We can also extend tests by making some ground truth tests.
  • I've optimized the function solve_weighted_umeyama a bit: just 1 loop, almost no data copying, plain data types (can be easily vectorized in the future). It's 20% faster on my machine.
  • I've pushed my changes into your branch, please check

@tfunatomi
Copy link
Contributor Author

@savuor
I appreciate your time and efforts for reviewing the request.

  • Why Partial solver is disabled? I've removed it until it's not in use.

I tried to cover many functions for SE2/SIM2/SE3/SIM3 by the Partial solver, but I gave up.
I forgot to remove the one. Thank you.

  • Weights are supported by the algorithm but never actually used. I think we may extend the interface by supporting weights (maybe in next PR?). We can also extend tests by making some ground truth tests.

I do not understand the design concept of usac module, but I have kept the compatibility with AffineNonMinimalSolverImpl.
Such extension of supporting weights sounds nice :)

  • I've optimized the function solve_weighted_umeyama a bit: just 1 loop, almost no data copying, plain data types (can be easily vectorized in the future). It's 20% faster on my machine.

I appreciate your labors for the optimization.

Comment on lines +3281 to +3287
CV_EXPORTS_W cv::Mat estimateSE2(InputArray from, InputArray to, OutputArray inliers = noArray(),
int method = USAC_DEFAULT, double ransacReprojThreshold = 3,
size_t maxIters = 2000, double confidence = 0.99,
size_t refineIters = 10);

CV_EXPORTS_W cv::Mat estimateSE2(InputArray pts1, InputArray pts2, OutputArray inliers,
const UsacParams &params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the function is analog of estimateAffine2D and estimateAffine3D https://docs.opencv.org/4.x/d9/d0c/group__calib3d.html#ga27865b1d26bac9ce91efaee83e94d4dd. They already have overloads with USAC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asmorkalov Thank you for your comment.

My estimateSE2() outputs a rigid transform with 3-DOF (rotation+translation) that is different from the output of estimateAffine2D, which has 6-DOF. Such a solution with some constraints is useful for applications like in-plane registration.

estimateSIM2() outputs a similarity transform with 4-DOF (scaling+rotation+translation), which estimateAffinePartial2D estimates, but there is no implementation with USAC as far as I understood.

I think estimateAffine3D does not have an overload with USAC as I stated:

However, the following functions are not supported and only support legacy methods (RANSAC and LMEDS).

Meanwhile, these functions provided to estimate a constrained geometric transformations consists of rotation, translation, and scaling with appropriate options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to extend existing API with USAC then, but not introduce the new one. @vpisarev What is your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asmorkalov

I propose to extend existing API with USAC then, but not introduce the new one.

I agree.
I was supposed to do that, but I was concerned about backward compatibility.
estimateAffinePartial2D and estimateAffine3D are similar but different in

  • estimateAffinePartial2D only supports similarity transform (SIM2) but estimateAffine3D supports either rigid or similarity with its argument: scale
  • estimateAffinePartial2D supports RANSAC but estimateAffine3D does NOT.

The main reason for making the new ones is that the function name sounds inappropriate: the estimates are NOT affine transform.
But, I would welcome your suggestions about the interface, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asmorkalov
What shall I do for this?
Extending the existing API might lose compatibility, so I think the current form is the second best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose to extend existing API with USAC then, but not introduce the new one. @vpisarev What is your opinion?

@asmorkalov @vpisarev
1.5 years have past already... Can I have your opinion/decision for the next step?

*/
const Mat * points_mat;
const float * const points;
float m11, m12, m13, m14, m21, m22, m23, m24, m31, m32, m33, m34;
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, we have Matx and Vec structs for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I prefer to use them, indeed.
But, I just follow the existing 2D version in favor.

class ReprojectionDistanceAffineImpl : public ReprojectionErrorAffine {
private:
const Mat * points_mat;
const float * const points;
float m11, m12, m13, m21, m22, m23;

@tfunatomi
Copy link
Contributor Author

@asmorkalov Are there any actions I can take to get approved?

@tfunatomi tfunatomi requested a review from asmorkalov August 20, 2023 22:38
@tfunatomi
Copy link
Contributor Author

Hi @asmorkalov,

It has been 1.5 years since I submitted a pull request, and it seems to have been forgotten.
I still believe that there is no alternative in this project, and this improvement is valuable for the community.

Could you please review the pull request when you have a moment?
If there are any changes needed or concerns, I'd be happy to address them.

Thank you for your time and consideration.

@tfunatomi
Copy link
Contributor Author

@alalek @asmorkalov Gentle ping – would appreciate your thoughts on whether the current integration is sufficient or if any changes are still needed. Thank you.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants