ChArUco pre460 pattern support by stefan523 · Pull Request #23153 · opencv/opencv

stefan523 · 2023-01-19T11:28:30Z

Add support for certain ChArUco board patterns as they had been generated with OpenCV contrib version prior 4.6.0.

The pull request adds a setLegacyParameter(bool) method to the Charuco class to allow switching to the old board design as described in #23152.

Default setting for this parameter is `false´ to remain compatible with OpenCV contrib 4.6.0 and OpenCV 4.7.0.

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

asmorkalov · 2023-01-20T09:00:39Z

modules/objdetect/include/opencv2/objdetect/aruco_board.hpp

+     * @param legacy legacy chessboard pattern (pre 4.6.0). Pattern generation was changed for even row patterns (see https://github.com/opencv/opencv_contrib/issues/3291).
     * The first markers in the dictionary are used to fill the white chessboard squares.
     */
+    CV_WRAP CharucoBoard(const Size& size, float squareLength, float markerLength,


I propose to not touch the constructor, but add setLegacyFlag() method. It's easier to remove legacy method in future than break compatibility for all code that uses Charuco.

I agree that we should not have an overload ctor, but rather a legacy flag (set to true by default) as the last parameter in the existing ctor (with an explanation in the ctor documentation), and go without a setLegacy method.

The reasons are below:

Migrating from pre-4.6.0 to 4.6.0 breaks the code silently (when using a physical target manufactured with pre 4.6.0) and it is difficult for a novice developer to figure out why.

Migrating from pre-4.5.5 to 4.7.0 breaks the code explicitly because the ctor is different (Size instead of two ints), as in @stefan523's example code in ChArUco pattern has broken backward compatibility #23152. When the developer fixes the compilation error, they trigger the silent failures of the item above. If we introduce a legacy flag set to true by default, this is avoided and backward compatibility is preserved even if the developer is not aware of the legacy flag.

Migrating from 4.6.0 to 4.7.0 requires the developer to explicitly update the ctor both in the Size parameters and in the legacy flag set to false (that is, assuming they are analyzing a target fabricated using 4.6.0 code). This is the only case that actually leads to backward compatibility issues per my suggestion, when the developer is not aware of the role of the legacy flag.

I think that it is more important to maintain backward compatibility with the dozens of released versions that have been employed to produce physical charuco targets that are still is use in production lines. Deciding otherwise will inevitably lead to systems version upgrades silently failing without an evident explanation.

I propose to not touch the constructor, but add setLegacyFlag() method. It's easier to remove legacy method in future than break compatibility for all code that uses Charuco.

I'm not sure if I understand the ask correctly.
I did keep the existing ctor as-is and added an overload with legacy flag in order to pass the ABI compliance check:
https://pullrequest.opencv.org/buildbot/builders/precommit_linux64/builds/102489/steps/Compare%20ABI%20dumps/logs/report-html
Adding a setLegacyFlag() method would add a new sympol as well. And this would break code, trigger an ABI compliance failure if removed in the future, would it not? I don't see the difference in that respect.

The existing ctor already creates a board. Adding a setLegacyFlag() method that is called after the constructor has run would mean to clear the board and recreate it differently. Is that really the ask? I don't see the advantage, only things that could go bad in the workflow. Or do I get it wrong?

Either way I'm fine with anything that enables usage of existing targets and data. @asmorkalov, please advise and I'll try to adapt.

I believe we should minimize or do not bring code with legacy/buggy behavior from opencv_contrib into the main repository (as we don't really want to support/maintain that).

If setLegacyFlag() doesn't work, then I propose to create CharucoBoardLegacy or cv::aruco::legacy::CharucoBoard in opencv_contrib/modules/aruco for this purpose.
There is inheritance with Board base class and Pimpl design, but some trick is required to expose Board::Impl to opencv_contrib (we don't need that in public OpenCV headers).

Deciding otherwise will inevitably lead to systems version upgrades silently failing without an evident explanation.

one way or another, mentioning this change in the release notes would have been nice..

probably it makes sense to dub the BBF order (i.e. 4.6, 4.7 behavior) "legacy" and hence set the flag to false by default.

I doubt that with that many WBF patterns in the wild and WBF effectively being the default from 4.8 on BBF will be anything but legacy.

Resolved: constructor is not being touched. Different board layouts are supported by setBBF(bool) method.

Sorry, there is useful information for discussion here. Please don't mark as resolved for now.

@AleksandrPanov, what are the required steps to merge this pull request?

I'm interest as well to understand when this pul request will be close?

modules/objdetect/src/aruco/aruco_board.cpp

paroj · 2023-01-30T12:24:44Z

👍 nice work! lets hope the maintainers agree.

stefan523 · 2023-02-02T11:34:28Z

@alalek, @asmorkalov, I updated the PR according to your suggestions: Leaving the constructor as-is, adding a method to optionally select different pattern designs. Can you please review?

gmedan

Great work

asmorkalov

👍 Thanks a lot for the contribution!

alalek

@asmorkalov There is something wrong again with GitHub Actions - no builds are run.

modules/objdetect/src/aruco/aruco_board.cpp

modules/objdetect/test/test_charucodetection.cpp

stefan523 · 2023-02-03T14:49:44Z

Hi @alalek,
all review comments should be fixed now. Please review/approve. Thanks!

AleksandrPanov

@stefan523, thank you for work, LGTM

The board design had to change to get compatibility with checkerboard pattern for camera calibration.

alalek · 2023-02-06T19:43:48Z

modules/objdetect/include/opencv2/objdetect/aruco_board.hpp

+     * Default behavior: WBF for even row, BBF for odd row patterns.
+     */
+    CV_WRAP void setBBF(bool BBF);
+    CV_WRAP bool getBBF() const;


@AleksandrPanov Did you really verify that?

Default pattern must be like this: https://github.com/opencv/opencv/blob/4.7.0/doc/pattern.png

We start discussion of this patch with setLegacyPattern() flag. No aggrement to use BBF or other unclear abbreviations (first, even, odd in one description is a true mess).
Description should mention changes after 4.5.5/4.6.0 releases.

Default behavior must be preserved (4.7.0).

@alalek, @AleksandrPanov, all raised concerns should've been resolved now. Please approve.

@alalek, I liked @stefan523's idea to give a logical name to the board design parameter. I think that @stefan523 need choose a different name (instead of BBF) and improve the description in the documentation.

Pattern design was changed to implement for calibration compatibility with charuco chessboards (interactive calibration tool + kalibr). For a better understanding, I suggest using this board design parameter as a new parameter, and not just a flag for compatibility with old solutions. I could suggest the name of the parameter as chessboardPatternCompatibility.

I was wrong, the default behavior should be like in 4.7.0.

Documentation/comments should be more clear now.
I had the parameter change to legacyParamter per directions in the other comment, but I am happy to change it again to anything else like chessboardPatternCompatibility if this would be the accepted name?

modules/objdetect/src/aruco/aruco_board.cpp

modules/objdetect/test/test_charucodetection.cpp

stefan523 · 2023-01-29T15:28:20Z

modules/objdetect/include/opencv2/objdetect/aruco_board.hpp

+     * @param legacy legacy chessboard pattern (pre 4.6.0). Pattern generation was changed for even row patterns (see https://github.com/opencv/opencv_contrib/issues/3291).
     * The first markers in the dictionary are used to fill the white chessboard squares.
     */
+    CV_WRAP CharucoBoard(const Size& size, float squareLength, float markerLength,


Resolved: constructor is not being touched. Different board layouts are supported by setBBF(bool) method.

modules/objdetect/src/aruco/aruco_board.cpp

modules/objdetect/test/test_charucodetection.cpp

stefan523 · 2023-03-15T14:24:16Z

Can someone please restart the build checks?
The build checks failed in "test_features2d" which is unrelated to this PR.

Seems resolved.. PR is ready to be merged. @AleksandrPanov, @asmorkalov, can you support? Or are we waiting for @alalek?

wittenator · 2023-04-15T21:22:08Z

Is there already an (approximate) date for the OpenCV 4.8 release by any chance?

pietrocolombo · 2023-04-24T13:28:18Z

When will it be merge?

stefan523 · 2023-04-26T11:44:52Z

When will it be merge?
I don't know the process.
@AleksandrPanov, can you answer this question?

asmorkalov · 2023-04-26T12:15:40Z

@stefan523 @pietrocolombo I apologize for the delay. The ball is on my side.

asmorkalov · 2023-05-04T14:14:08Z

Rebased and squashed.

apurvazaveri · 2025-09-11T03:26:27Z

Thank you Stefan!

This was referenced Jan 19, 2023

ChArUco pattern has broken backward compatibility #23152

Closed

CharucoBoard_create regression / incompatible patterns (only even row count) with 4.6.0 opencv/opencv_contrib#3291

Closed

asmorkalov requested a review from AleksandrPanov January 19, 2023 11:35

asmorkalov added category: objdetect feature RFC labels Jan 19, 2023

stefan523 marked this pull request as draft January 19, 2023 17:57

stefan523 marked this pull request as ready for review January 19, 2023 17:59

stefan523 marked this pull request as draft January 19, 2023 18:31

stefan523 marked this pull request as ready for review January 19, 2023 20:06

stefan523 force-pushed the ChArUco_pre460_pattern_support branch 2 times, most recently from 4ed7580 to 9989103 Compare January 19, 2023 21:13

asmorkalov reviewed Jan 20, 2023

View reviewed changes

asmorkalov requested a review from alalek January 23, 2023 07:24

paroj reviewed Jan 25, 2023

View reviewed changes

modules/objdetect/src/aruco/aruco_board.cpp Outdated Show resolved Hide resolved

stefan523 requested review from asmorkalov and removed request for AleksandrPanov and alalek January 30, 2023 07:19

gmedan approved these changes Feb 2, 2023

View reviewed changes

paroj approved these changes Feb 2, 2023

View reviewed changes

asmorkalov approved these changes Feb 3, 2023

View reviewed changes

alalek reviewed Feb 3, 2023

View reviewed changes

modules/objdetect/src/aruco/aruco_board.cpp Outdated Show resolved Hide resolved

modules/objdetect/src/aruco/aruco_board.cpp Outdated Show resolved Hide resolved

modules/objdetect/test/test_charucodetection.cpp Outdated Show resolved Hide resolved

stefan523 marked this pull request as draft February 3, 2023 11:07

stefan523 force-pushed the ChArUco_pre460_pattern_support branch 3 times, most recently from d259d2d to 8bd30af Compare February 3, 2023 13:44

stefan523 marked this pull request as ready for review February 3, 2023 14:48

AleksandrPanov approved these changes Feb 6, 2023

View reviewed changes

alalek reviewed Feb 6, 2023

View reviewed changes

AleksandrPanov self-requested a review February 8, 2023 13:09

stefan523 commented Feb 9, 2023

View reviewed changes

stefan523 force-pushed the ChArUco_pre460_pattern_support branch 2 times, most recently from dbc2a2f to e80ff08 Compare February 17, 2023 15:35

stefan523 mentioned this pull request Feb 17, 2023

Aruco/Charuco test case fixes for floating point for loops #23271

Merged

6 tasks

stefan523 removed request for AleksandrPanov and asmorkalov March 15, 2023 14:00

stefan523 marked this pull request as draft March 15, 2023 14:20

stefan523 marked this pull request as ready for review March 15, 2023 14:21

ChArUco pre460 pattern support

e55784a

asmorkalov force-pushed the ChArUco_pre460_pattern_support branch from e80ff08 to e55784a Compare May 4, 2023 14:13

asmorkalov merged commit 6160104 into opencv:4.x May 4, 2023

stefan523 deleted the ChArUco_pre460_pattern_support branch May 5, 2023 18:59

stefan523 mentioned this pull request May 5, 2023

ChArUco pre460 pattern support via switch opencv/opencv_contrib#3305

Closed

6 tasks

asmorkalov mentioned this pull request May 31, 2023

(5.x) Merge 4.x #23718

Merged

Uh oh!

Conversation

stefan523 commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paroj commented Jan 30, 2023

Uh oh!

stefan523 commented Feb 2, 2023

Uh oh!

gmedan left a comment

Choose a reason for hiding this comment

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefan523 commented Feb 3, 2023

Uh oh!

AleksandrPanov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AleksandrPanov Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefan523 commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Can someone please restart the build checks? The build checks failed in "test_features2d" which is unrelated to this PR.

Uh oh!

wittenator commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefan523 commented Jan 19, 2023 •

edited

Loading

AleksandrPanov left a comment •

edited

Loading

AleksandrPanov Feb 8, 2023 •

edited

Loading

stefan523 commented Mar 15, 2023 •

edited

Loading

Can someone please restart the build checks?
The build checks failed in "test_features2d" which is unrelated to this PR.

wittenator commented Apr 15, 2023 •

edited

Loading