Skip to content

Fix bug in ChessBoardDetector::findQuadNeighbors#24779

Merged
asmorkalov merged 2 commits intoopencv:4.xfrom
MaximSmolskiy:fix-bug-in-ChessBoardDetector-findQuadNeighbors
Feb 6, 2024
Merged

Fix bug in ChessBoardDetector::findQuadNeighbors#24779
asmorkalov merged 2 commits intoopencv:4.xfrom
MaximSmolskiy:fix-bug-in-ChessBoardDetector-findQuadNeighbors

Conversation

@MaximSmolskiy
Copy link
Copy Markdown
Contributor

@MaximSmolskiy MaximSmolskiy commented Dec 27, 2023 •
edited
Loading

Pull Request Readiness Checklist

corners and neighbors indices means not filling order, but relative position. So, for example if quad->count = 2, it doesn't mean that quad->neighbors[0] and quad->neighbors[1] are filled. And we should should iterate over all four neighbors.

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

@asmorkalov asmorkalov added the pr: needs test New functionality requires minimal tests set label Dec 27, 2023
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 27, 2023

@MaximSmolskiy Could you add some test to highlight the problem. I need time to understand the code and issue root.

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Dec 27, 2023

@MaximSmolskiy Could you add some test to highlight the problem. I need time to understand the code and issue root.

@asmorkalov I checked this pr on hundreds of images and there were no changes in quality/accuracy.
But we checked some part of corners when comment and intuition say that we should check all corners.
You can add CV_Assert(closest_quad->neighbors[j]); and it should fail and then it mean that we miss some corners during this check

@AleksandrPanov AleksandrPanov self-assigned this Jan 10, 2024
@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Jan 15, 2024

@asmorkalov @AleksandrPanov gentle ping

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Jan 22, 2024

@asmorkalov @AleksandrPanov gentle ping

@AleksandrPanov
Copy link
Copy Markdown
Contributor

AleksandrPanov commented Jan 23, 2024

@MaximSmolskiy, I will check this PR today, and test the benchmark

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Jan 26, 2024

@AleksandrPanov Any updates?

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

MaximSmolskiy commented Feb 2, 2024

@asmorkalov @AleksandrPanov ping

AleksandrPanov
AleksandrPanov approved these changes Feb 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@AleksandrPanov AleksandrPanov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested with benchmark:
--configuration=generate_run --board_x=7 --path=res_chessboard --synthetic_object=chessboard

Before PR fix:
image

After PR fix:
image

There are no regressions.

@AleksandrPanov AleksandrPanov removed the pr: needs test New functionality requires minimal tests set label Feb 5, 2024
@AleksandrPanov
Copy link
Copy Markdown
Contributor

AleksandrPanov commented Feb 5, 2024

@asmorkalov, I propose to merge this PR.

asmorkalov
asmorkalov approved these changes Feb 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov asmorkalov added this to the 4.10.0 milestone Feb 6, 2024
@asmorkalov asmorkalov merged commit 28d22d7 into opencv:4.x Feb 6, 2024
@MaximSmolskiy MaximSmolskiy deleted the fix-bug-in-ChessBoardDetector-findQuadNeighbors branch February 6, 2024 13:17
@asmorkalov asmorkalov mentioned this pull request Feb 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

@AleksandrPanov AleksandrPanov AleksandrPanov approved these changes

Assignees

@AleksandrPanov AleksandrPanov

Labels

bug category: objdetect

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MaximSmolskiy @asmorkalov @AleksandrPanov