opencv_contrib icon indicating copy to clipboard operation
opencv_contrib copied to clipboard

CharucoBoard_create regression / incompatible patterns (only even row count) with 4.6.0

Open stefan523 opened this issue 3 years ago • 12 comments

System information (version)

OpenCV = 4.6.0 / 4.5.5

Detailed description

cv2.aruco.CharucoBoard_create creates a different pattern in 4.6.0 than in 4.5.5 for even row counts. This leads to backwards incompatibility with previous versions, previously manufactured targets, etc.

OpenCV 4.5.5: charuco_OpenCV-4 5 5

OpenCV 4.6.0: charuco_OpenCV-4 6 0

Maybe this regression was introduced when resolving issue https://github.com/opencv/opencv_contrib/issues/2623 ?

Steps to reproduce
import cv2

aruco_dict = cv2.aruco.getPredefinedDictionary(cv2.aruco.DICT_5X5_1000)
charuco_board = cv2.aruco.CharucoBoard_create(
    squaresX = 5,
    squaresY = 4,
    squareLength = 4,
    markerLength = 3,
    dictionary = aruco_dict
)
img_board = charuco_board.draw((250,200), marginSize=0, borderBits=1)
cv2.imwrite('charuco_OpenCV-' + cv2.__version__ + '.png', img_board)

stefan523 avatar Jun 23 '22 08:06 stefan523

Behavior is aligned with chessboard: https://github.com/opencv/opencv/blob/4.x/doc/pattern.png

alalek avatar Jun 23 '22 09:06 alalek

Behavior is aligned with chessboard: https://github.com/opencv/opencv/blob/4.x/doc/pattern.png

That pattern in that link is 10x7 (although it claims to be 9x6 in the caption...or does it mean 9x6 corners? nvm.) and this looks the same in both versions. image

Anyhow the change breaks backwards compatibiltiy (SW&HW).

It would help a lot to e.g. have a switch to enable the legacy pattern of pre4.6.0.

stefan523 avatar Jun 23 '22 11:06 stefan523

About the chessboard pattern, this is indeed a 9x6 pattern since only corners from intersection are detected / used.

image

catree avatar Jun 23 '22 21:06 catree

Hmm, some more precision.

This was the code to generate the pattern (see https://github.com/opencv/opencv/pull/11046):

python gen_pattern.py -r 10 -c 7 -a A4 -T checkerboard -s 25

But when you are doing camera calibration (see https://github.com/opencv/opencv/blob/4.x/samples/cpp/calibration.cpp), you have to generate a 9x6 grid / corner points vector. That's why it is written 9x6.

catree avatar Jun 24 '22 08:06 catree

The below patch seems to work for me, adding a "legacy" switch when creating the board.

diff --git a/modules/aruco/include/opencv2/aruco/charuco.hpp b/modules/aruco/include/opencv2/aruco/charuco.hpp
index 2417b2fd..69b511a1 100644
--- a/modules/aruco/include/opencv2/aruco/charuco.hpp
+++ b/modules/aruco/include/opencv2/aruco/charuco.hpp
@@ -92,13 +92,14 @@ class CV_EXPORTS_W CharucoBoard : public Board {
      * @param markerLength marker side length (same unit than squareLength)
      * @param dictionary dictionary of markers indicating the type of markers.
      * The first markers in the dictionary are used to fill the white chessboard squares.
+     * @param legacy legacy chessboard pattern (pre 4.6.0)
      * @return the output CharucoBoard object
      *
      * This functions creates a CharucoBoard object given the number of squares in each direction
      * and the size of the markers and chessboard squares.
      */
     CV_WRAP static Ptr<CharucoBoard> create(int squaresX, int squaresY, float squareLength,
-                                            float markerLength, const Ptr<Dictionary> &dictionary);
+                                            float markerLength, const Ptr<Dictionary> &dictionary, bool legacy = false);
 
     /**
       *
@@ -126,6 +127,9 @@ class CV_EXPORTS_W CharucoBoard : public Board {
 
     // marker side length (normally in meters)
     float _markerLength;
+
+    // legacy chessboard pattern (pre 4.6.0)
+    bool _legacy;
 };
 
 
diff --git a/modules/aruco/src/charuco.cpp b/modules/aruco/src/charuco.cpp
index b179bc63..2676647e 100644
--- a/modules/aruco/src/charuco.cpp
+++ b/modules/aruco/src/charuco.cpp
@@ -101,7 +101,11 @@ void CharucoBoard::draw(Size outSize, OutputArray _img, int marginSize, int bord
     for(int y = 0; y < _squaresY; y++) {
         for(int x = 0; x < _squaresX; x++) {
 
-            if(y % 2 != x % 2) continue; // white corner, dont do anything
+            if(_legacy) {
+                if((y + ((_squaresY + 1) % 2)) % 2 != x % 2) continue; // white corner, dont do anything
+            } else {
+                if(y % 2 != x % 2) continue; // white corner, dont do anything
+            }
 
             double startX, startY;
             startX = squareSizePixels * double(x);
@@ -120,7 +124,7 @@ void CharucoBoard::draw(Size outSize, OutputArray _img, int marginSize, int bord
 /**
  */
 Ptr<CharucoBoard> CharucoBoard::create(int squaresX, int squaresY, float squareLength,
-                                  float markerLength, const Ptr<Dictionary> &dictionary) {
+                                  float markerLength, const Ptr<Dictionary> &dictionary, bool legacy) {
 
     CV_Assert(squaresX > 1 && squaresY > 1 && markerLength > 0 && squareLength > markerLength);
     Ptr<CharucoBoard> res = makePtr<CharucoBoard>();
@@ -129,6 +133,7 @@ Ptr<CharucoBoard> CharucoBoard::create(int squaresX, int squaresY, float squareL
     res->_squaresY = squaresY;
     res->_squareLength = squareLength;
     res->_markerLength = markerLength;
+    res->_legacy = legacy;
     res->dictionary = dictionary;
 
     float diffSquareMarkerLength = (squareLength - markerLength) / 2;
@@ -137,7 +142,11 @@ Ptr<CharucoBoard> CharucoBoard::create(int squaresX, int squaresY, float squareL
     for(int y = 0; y < squaresY; y++) {
         for(int x = 0; x < squaresX; x++) {
 
-            if(y % 2 == x % 2) continue; // black corner, no marker here
+            if(legacy) {
+                if((y + ((squaresY + 1) % 2)) % 2 == x % 2) continue; // black corner, no marker here
+            } else {
+                if(y % 2 == x % 2) continue; // black corner, no marker here
+            }
 
             vector<Point3f> corners(4);
             corners[0] = Point3f(x * squareLength + diffSquareMarkerLength,

stefan523 avatar Jun 28 '22 09:06 stefan523

@stefan523 there is behavior change only with even row count. There were several bugs: https://github.com/opencv/opencv_contrib/issues/2623, https://github.com/opencv/opencv_contrib/issues/2604

This path fixed this and now CharucoBoard behavior is aligned with chessboard: https://github.com/opencv/opencv/blob/4.x/doc/pattern.png

You can use the suggested patch, but it is recommended to use new patterns (with even row count) as the calibration might not work correctly on old patterns (with even row count). The legacy version not added to avoid using even templates, with increased re-projection error.

AleksandrPanov avatar Jul 06 '22 13:07 AleksandrPanov

The legacy version not added to avoid using even templates, with increased re-projection error.

Why do you say that? What is this based on?

it is recommended to use new patterns (with even row count) as the calibration might not work correctly on old patterns (with even row count)

Production code around the world relies on physical boards with even row counts, produced using pre-4.6.0 code, that cannot and will not be physically replaced to the new pattern. This renders opencv 4.6.0 unusable in many calibration/tracking systems, including ones that I developed and maintain. In #3301 the guy bought a board from calib.io, should he throw it out? Should calib.io discard their stock?

@AleksandrPanov I don't think you realize that this was a huge breaking change. A legacy flag set to true by default is the least that can be done to avoid breaking existing code when upgrading (speaking from experience here). @alalek what do you think?

gmedan avatar Jul 12 '22 20:07 gmedan

Now that 4.6.0 is out in the wild it should be worth to consider actively breaking API backward compatibility and not setting a default option, forcing developers to choose (legacy or new). Otherwise unsuspecting individuals might stumble over this issue for quite a while. Maybe someone is producing new targets with 4.6.0 now, which would break if a future version has the legacy flag set to true by default?

Either way it would be really appreciated to have the legacy functionality accessible without applying a patch manually and compiling opencv+contrib from source.

PS: The patch I suggested above does not change the order of IDs for "legacy", so maybe it would not re-open issues https://github.com/opencv/opencv_contrib/issues/2623 and https://github.com/opencv/opencv_contrib/issues/2604?

stefan523 avatar Jul 13 '22 22:07 stefan523

I understand the reasoning behind the change, but I believe that backward compatibility (huge) trumps consistency here. I strongly support the opinion that a legacy switch (on by default) is needed. For developers printing patterns ad-hoc the change might not seem dramatic, but ChAruCo patterns are used in mission critical production software pipelines by many customers worldwide. No-one expects an OpenCV version update to break compatibility with older patterns. Considerable investments have been made into stock. Keeping separate pre- and post- 4.6.0 ChAruCo boards is wasteful in terms of time and resources.

Jakob // Calib.io

jakobwilm avatar Jul 14 '22 00:07 jakobwilm

I went ahead and created pull request https://github.com/opencv/opencv_contrib/pull/3305 with this patch. Legacy is set to "true" by default. Some small adjustments were necessary to get the unit tests working properly with both board types. All is pass now, and there are no fails regarding bad reprojection errors or similar. Changing or removing the default value of legacy would be simple enough if that would be the final verdict.

stefan523 avatar Jul 20 '22 13:07 stefan523

I was calibrating with 4.6.0 and 8x11 board charuco board and always got huge reprojection error.

This bug is not easy to find.

I downgraded the opencv to 4.5.*, hope we can have a fix soon

ardiya avatar Jul 23 '22 05:07 ardiya

I updated pull request https://github.com/opencv/opencv_contrib/pull/3305 to resolve merge conflicts due to ongoing main branch development. What are the next steps to move forward? @AleksandrPanov, please comment.

stefan523 avatar Sep 09 '22 14:09 stefan523

Andrew Martchenko showed a workaround in this comment: https://github.com/opencv/opencv_contrib/issues/3367#issuecomment-1292874997

stefan523 avatar Oct 28 '22 06:10 stefan523

opencv 4.7.0 is now released, with aruco moved to the main repo (not contrib). This problem was not addressed, and the legacy flag was not introduced. @stefan523 I think you should reopen the issue under the main repo and port your PR

gmedan avatar Jan 01 '23 07:01 gmedan

@gmedan

opencv 4.7.0 is now released, with aruco moved to the main repo (not contrib). This problem was not addressed, and the legacy flag was not introduced. @stefan523 I think you should reopen the issue under the main repo and port your PR

Done:

  • https://github.com/opencv/opencv/pull/23152
  • https://github.com/opencv/opencv/pull/23153

stefan523 avatar Jan 19 '23 11:01 stefan523

Kudos @stefan523!

gmedan avatar Jan 19 '23 12:01 gmedan

Fixed in main branch by https://github.com/opencv/opencv/pull/23153 for next OpenCV release 4.8.0.

stefan523 avatar May 05 '23 19:05 stefan523