Make finding corner neighbor symmetrical in ChessBoardDetector::findQuadNeighbors #25427
Conversation
modules/calib3d/src/calibinit.cpp
Outdated
| int& closest_corner_idx, | ||
| cv::Point2f& closest_corner_pt) | ||
| { | ||
| if (cur_quad.neighbors[i]) |
There was a problem hiding this comment.
The condition should be moved outside the function. i is not used inside the function.
modules/calib3d/src/calibinit.cpp
Outdated
| @@ -471,6 +471,110 @@ static void icvBinarizationHistogramBased(Mat & img) | |||
| } | |||
| } | |||
|
|
|||
| static bool findCornerNeighbor( | |||
| cv::AutoBuffer<ChessBoardQuad>& all_quads, | |||
There was a problem hiding this comment.
I suppose it should be just ChessBoardQuad* all_quads. AutoBuffer details are not needed in the function.
modules/calib3d/src/calibinit.cpp
Outdated
| const std::vector<Point2f>& all_quads_pts, | ||
| flann::GenericIndex<flann::L2_Simple<float>>& all_quads_pts_index, | ||
| const int idx, | ||
| const ChessBoardQuad& cur_quad, |
There was a problem hiding this comment.
cur_quad is all_quads[idx], I do not think that it's needed in the function parameters.
modules/calib3d/src/calibinit.cpp
Outdated
| const float radius, | ||
| std::vector<int>& neighbors_indices, | ||
| std::vector<float>& neighbors_dists, | ||
| const float thresh_scale, |
There was a problem hiding this comment.
I propose to set default value for thresh_scale or even make it local constant. It's constant equal to 1.0 on caller side.
There was a problem hiding this comment.
It is also used outside function
float radius = cur_quad.edge_len * thresh_scale + 1;
So, do you propose to set it twice with same value?
modules/calib3d/src/calibinit.cpp
Outdated
| std::vector<float>& neighbors_dists, | ||
| const float thresh_scale, | ||
| int& closest_quad_idx, | ||
| ChessBoardQuad*& closest_quad, |
There was a problem hiding this comment.
closest_quad= all_quads[closest_quad_idx]. No need to pass it with parameters.
modules/calib3d/src/calibinit.cpp
Outdated
| std::vector<int>& neighbors_indices, | ||
| std::vector<float>& neighbors_dists, |
There was a problem hiding this comment.
The values are not used outside the function. I propose to make it local.
There was a problem hiding this comment.
Their lengths are equal to all_corners_count which can be quite large. So, it's better to avoid such frequent allocations on every function call
|
@asmorkalov @AleksandrPanov gentle ping |
|
@asmorkalov @AleksandrPanov gentle ping |
|
@MaximSmolskiy, it looks like there are too many parameters in I added the Do you have any comments or suggestions? Otherwise, I like it. |
AleksandrPanov
force-pushed
the
make-finding-corner-neighbor-symmetrical-in-ChessBoardDetector-findQuadNeighbors
branch
from
31217ac
to
e705bae
Compare
AleksandrPanov
force-pushed
the
make-finding-corner-neighbor-symmetrical-in-ChessBoardDetector-findQuadNeighbors
branch
from
e705bae
to
e064683
Compare
|
@AleksandrPanov Thanks for refactoring! I don't have any suggestions |
Pull Request Readiness Checklist
The basic idea of finding pair of corners neighbors is to find best candidate for first corner and check if first corner quite good candidate for its best candidate. And we test first corner for its best candidate less than best candidate for first corner.
Idea of changes is to make finding corner neighbor symmetrical - find best candidate for first corner, find best candidate for second corner and match them as pair iff they are both best candidates for each other.
Additional advantage - it simplifies code and removes some code duplication.
I tested this PR with benchmark
There are minor changes in results
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.