Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix objPoints order in GridBoard and CharucoBoard #3174

Merged
merged 6 commits into from
Mar 16, 2022

Conversation

AleksandrPanov
Copy link
Contributor

@AleksandrPanov AleksandrPanov commented Feb 11, 2022

Fixes #2623 and #2604

This PR:

Fixed objPoints order in GridBoard and CharucoBoard.
In the _drawPlanarBoardImpl() function the y coordiantes of the charuco markers get horizontally mirrored by pf.y = (1.0f - pf.y / sizeY) * float(out.rows); Now pf.y = pf.y / sizeY * float(out.rows);

Drawing correctness was checked by example.cpp.txt

Before fix:

  • Arrays objPoints and ids for 3x4 aruco::GridBoard are in the wrong order
    board_detected_grid
  • Arrays objPoints and ids for 3x4 aruco::CharucoBoard are in the wrong order. Also objPoints coordinates are incorrect.
    charuco_detected_grid

After fix:
image
Everything is drawn correctly.

Fixed generation of rotated images (in ArUco tests). Now rotated images are generated using the "aircraft principal axes".

image
rotate_pitch
rotate_yaw

Also fixed objPoints[markerIndex] order (from CCW to CW) in GridBoard and CharucoBoard. Because tutorial and all ArUco algs use CW order.

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

@AleksandrPanov AleksandrPanov self-assigned this Feb 11, 2022
@AleksandrPanov AleksandrPanov force-pushed the fix_gridboard_objPoints branch 3 times, most recently from 3bd672c to f824b1b Compare February 14, 2022 00:57
modules/aruco/include/opencv2/aruco.hpp Outdated Show resolved Hide resolved

// rotate camera in pitch axis
Mat rotPitch(3, 1, CV_64FC1);
rotPitch.ptr< double >(0)[0] = pitch;
Copy link
Contributor

Choose a reason for hiding this comment

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

rotPitch.at< double >(i, j) is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@AleksandrPanov AleksandrPanov force-pushed the fix_gridboard_objPoints branch 3 times, most recently from 9e0a992 to 03dd8c5 Compare February 15, 2022 19:33
@AleksandrPanov AleksandrPanov marked this pull request as ready for review February 15, 2022 20:50
@AleksandrPanov AleksandrPanov removed their assignment Feb 16, 2022
@@ -294,7 +294,7 @@ class CV_EXPORTS_W Board {
CV_WRAP void setIds(InputArray ids);

/// array of object points of all the marker corners in the board
/// each marker include its 4 corners in CCW order. For M markers, the size is Mx4.
/// each marker include its 4 corners in CW order. For M markers, the size is Mx4.
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to mention CW or CCW without specifying used coordinate system.

Please take a look on calib3d module documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this definition:

    /// each marker include its 4 corners in this order:
    ///-   objPoints[i][0] - left-top point of i-th marker
    ///-   objPoints[i][1] - right-top point of i-th marker
    ///-   objPoints[i][2] - right-bottom point of i-th marker
    ///-   objPoints[i][3] - left-bottom point of i-th marker

modules/aruco/test/test_aruco_utils.hpp Show resolved Hide resolved
corners[2] = corners[0] + Point3f(markerLength, -markerLength, 0);
corners[3] = corners[0] + Point3f(0, -markerLength, 0);
corners[2] = corners[0] + Point3f(markerLength, markerLength, 0);
corners[3] = corners[0] + Point3f(0, markerLength, 0);
Copy link
Member

Choose a reason for hiding this comment

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

You just changed the coordinate system by flipping Y.

Copy link
Contributor Author

@AleksandrPanov AleksandrPanov Feb 16, 2022

Choose a reason for hiding this comment

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

This changes was added to make coordinate systems consistent.
Now Board::create() uses this coordinate system:
image
But Board::draw() uses another coordinate system:
image

Also, Board::create() sets corners for marker in objPoints with "CCW" order:
image
But detectMarkers() finds markers and reorders corners to "CW" order:
image

This makes coordinate systems not consistent and makes the code hard to read

namespace opencv_test {
namespace {

static inline double deg2rad(double deg) { return deg * CV_PI / 180.; }
Copy link
Member

Choose a reason for hiding this comment

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

avoid indentation in namespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@AleksandrPanov
Copy link
Contributor Author

@alalek thanks for the review)
do you have any more questions?

@AleksandrPanov AleksandrPanov marked this pull request as draft February 24, 2022 14:29
axis.push_back(Point3f(0.f, 0.f, 0.f));
axis.push_back(Point3f(length, 0.f, 0.f));
axis.push_back(Point3f(0.f, length, 0.f));
axis.push_back(Point3f(0.f, 0.f, -length));
Copy link
Contributor

Choose a reason for hiding this comment

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

why -length? Your function differs from standard drawFrameAxes with opposite direction of the third axis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added to improve appearance. The drawAxis() function doesn't make sense, as it is just calls drawFrameAxes().

axis.push_back(Point3f(offset, offset, 0.f));
axis.push_back(Point3f(length+offset, offset, 0.f));
axis.push_back(Point3f(offset, length+offset, 0.f));
axis.push_back(Point3f(offset, offset, -length));
Copy link
Contributor

Choose a reason for hiding this comment

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

The same, the result axis direction is opposite to what standard drawFrameAxes does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@AleksandrPanov AleksandrPanov force-pushed the fix_gridboard_objPoints branch 2 times, most recently from 3b22360 to de40e05 Compare March 3, 2022 22:33
@AleksandrPanov
Copy link
Contributor Author

AleksandrPanov commented Mar 14, 2022

I am not fan of the drawCustomAxis() because it feels unnecessary for me. I would prefer some good API for handling SE3/SO3 transformations with some good tutorials. You can also shoot yourself in the foot if you use axisCoefficient=(-1,0,0) or any other non valid values.

Moreover, the extra functionality of drawCustomAxis() is not used in your PR (only for debugging purposes), no?

What about composeRT(), can it do the same job?

@catree, thx)
I removed drawCustomAxis() and added cv::drawFrameAxes()'s link to docs and replaced old images with new ones. The tutorial now shows the correct direction and position of the axes.

@AleksandrPanov
Copy link
Contributor Author

@catree, I think that good API for handling SE3/SO3 transformations should add into another PR. This PR fixes only ArUco problems.

@AleksandrPanov
Copy link
Contributor Author

AleksandrPanov commented Mar 16, 2022

@alalek could you recheck PR? I fixed catree's issue.

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@alalek alalek merged commit 56d492c into opencv:3.4 Mar 16, 2022
andy-held pushed a commit to andy-held/opencv_contrib that referenced this pull request Mar 23, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial
andy-held added a commit to andy-held/opencv_contrib that referenced this pull request Mar 23, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
AleksandrPanov pushed a commit to AleksandrPanov/opencv_contrib that referenced this pull request Mar 25, 2022
…oints

Fix objPoints order in GridBoard and CharucoBoard

* fix gridBoard

* fix charucoBoard

* add rightBottomBorder

* add test_aruco_utils and code refactoring/fix tests

* fix axes and add charuco dict

* add axes test, remove drawAxis(), update tutorial

(cherry picked from commit 56d492c)
@alalek alalek mentioned this pull request Mar 26, 2022
@opencv-pushbot opencv-pushbot mentioned this pull request Apr 23, 2022
@Licoze
Copy link

Licoze commented May 5, 2022

Hello, @AleksandrPanov
FYI:
Removing drawAxis breaks backward compatibility. (for example for some wrappers https://github.com/shimat/opencvsharp)
Wouldn't it be better to move it to the next major release?

@mabl
Copy link

mabl commented Jun 23, 2022

It looks like this change makes interpolateCornersCharuco return a different number for board corners. See image here. Before this, this corner id mapped to the index of board->chessboardCorners.

Is this change intentional? If so, how can I recover the corresponding index for board->chessboardCorners from interpolateCornersCharuco markerIds, i.e. the list of identifiers for each marker in corners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants