NotificationsYou must be signed in to change notification settings
Fork56.4k
Star85.3k

fix type cast in drawDetectedMarkers, drawDetectedCornersCharuco, drawDetectedDiamonds#24247

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

asmorkalov merged 1 commit intoopencv:4.xfromAleksandrPanov:fix_drawDetectedCornersCharuco_type_error

Sep 12, 2023

Merged

fix type cast in drawDetectedMarkers, drawDetectedCornersCharuco, drawDetectedDiamonds#24247

asmorkalov merged 1 commit intoopencv:4.xfromAleksandrPanov:fix_drawDetectedCornersCharuco_type_error

Sep 12, 2023

Conversation

Copy link

Contributor

AleksandrPanov commentedSep 8, 2023•
edited
Loading

drawDetectedMarkers(), drawDetectedCornersCharuco(), drawDetectedDiamonds() uses onlyPoint2f (CV_32FC2) for detected corners. But this corners are casted toint later in the code.
drawDetectedMarkers(), drawDetectedDiamonds() has a requirement on CV_32FC2 for corners. This requirement is lost in drawDetectedCornersCharuco().

Strict input data requirements have been removed. Added only the requirement to have 2 channels. The input corners are then casted to CV_32SC2.

added tests

Pull Request Readiness Checklist

See details athttps://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 added bug test category: objdetect labels

Sep 8, 2023

asmorkalov reviewed

Sep 8, 2023

View reviewed changes

modules/objdetect/src/aruco/charuco_detector.cpp Outdated

		(_image.getMat().channels() ==1 \|\| _image.getMat().channels() ==3));
		CV_Assert((_charucoCorners.getMat().total() == _charucoIds.getMat().total()) \|\|
		_charucoIds.getMat().total() ==0);
		CV_Assert(_charucoCorners.getMat().type() == CV_32FC2);

Copy link

Contributor

asmorkalovSep 8, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I propose alternative:

CV_Assert(_charucoCorners.channels() == 2);..._charucoCorners.getMat().convertTo(..., ...);

Copy link

Contributor

asmorkalovSep 8, 2023

Choose a reason for hiding this comment

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

No need to limit type here. Just use proper conversions.

Copy link

ContributorAuthor

AleksandrPanovSep 8, 2023

Choose a reason for hiding this comment

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

then I will remove the limits from all functions (drawDetectedMarkers, drawDetectedCornersCharuco, drawDetectedDiamonds)

modules/objdetect/src/aruco/charuco_detector.cpp Outdated

		_charucoIds.getMat().total() ==0);
		CV_Assert(_charucoCorners.getMat().type() == CV_32FC2);

		size_t nCorners = _charucoCorners.getMat().total();

Copy link

Contributor

asmorkalovSep 8, 2023

Choose a reason for hiding this comment

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

InputArray hastotal() method. getMat() is redundant.

Copy link

ContributorAuthor

AleksandrPanovSep 8, 2023

Choose a reason for hiding this comment

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

removed getMat()

asmorkalov added this to the4.9.0 milestone

Sep 8, 2023

AleksandrPanov force-pushed thefix_drawDetectedCornersCharuco_type_error branch fromd65f3d3 to093878aCompare

September 8, 2023 14:12

AleksandrPanov changed the title~~fix drawDetectedCornersCharuco type error~~fix type cast in drawDetectedMarkers, drawDetectedCornersCharuco, drawDetectedDiamonds

Sep 8, 2023

AleksandrPanov marked this pull request as draft

September 8, 2023 15:53

AleksandrPanov force-pushed thefix_drawDetectedCornersCharuco_type_error branch 3 times, most recently from4dde75d tob7b8183Compare

September 8, 2023 17:47

AleksandrPanov marked this pull request as ready for review

September 8, 2023 17:58

AleksandrPanov force-pushed thefix_drawDetectedCornersCharuco_type_error branch 4 times, most recently from4bfb003 to587dc3fCompare

September 9, 2023 09:38

asmorkalov reviewed

Sep 11, 2023

View reviewed changes

modules/objdetect/src/aruco/aruco_detector.cpp Outdated

		Mat currentMarker = _corners.getMat(i);
		CV_Assert(currentMarker.total() ==4 && currentMarker.type() == CV_32FC2);
		CV_Assert(currentMarker.total() ==4 && currentMarker.channels() ==2);
		currentMarker.convertTo(currentMarker, CV_32SC2);

Copy link

Contributor

asmorkalovSep 11, 2023

Choose a reason for hiding this comment

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

It makes sense, to check if the input is already32SC2.convertTo does not check it.

Copy link

ContributorAuthor

AleksandrPanovSep 11, 2023

Choose a reason for hiding this comment

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

added check:

        if (currentMarker.type() != CV_32SC2)            currentMarker.convertTo(currentMarker, CV_32SC2);

modules/objdetect/src/aruco/charuco_detector.cpp Outdated

		Mat currentMarker = _corners.getMat(i);
		CV_Assert(currentMarker.total() ==4 && currentMarker.type() == CV_32FC2);
		CV_Assert(currentMarker.total() ==4 && currentMarker.channels() ==2);
		currentMarker.convertTo(currentMarker, CV_32SC2);

Copy link

Contributor

asmorkalovSep 11, 2023

Choose a reason for hiding this comment

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

The same for type check.

Copy link

ContributorAuthor

AleksandrPanovSep 11, 2023

Choose a reason for hiding this comment

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

added check:

        if (currentMarker.type() != CV_32SC2)            currentMarker.convertTo(currentMarker, CV_32SC2);

modules/objdetect/test/test_charucodetection.cpp Outdated

Comment on lines 694 to 695

		vector<vector<Point2d>> detected_double = {{Point2d(0.,0.),Point2d(10.,0.),Point2d(10.,10.),Point2d(0.,10.)}};
		vector<vector<Point2d>> detected_int = {{Point(0,0),Point(10,0),Point2d(10,10),Point2d(0,10)}};

Copy link

Contributor

asmorkalovSep 11, 2023

Choose a reason for hiding this comment

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

The same test typePoint2d andPoint2d. Should it be2i in the second case?

Copy link

ContributorAuthor

AleksandrPanovSep 11, 2023

Choose a reason for hiding this comment

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

fixed:
vector<vector<Point2i>> detected_int = {{Point(0, 0), Point(10, 0), Point(10, 10), Point(0, 10)}};

AleksandrPanov force-pushed thefix_drawDetectedCornersCharuco_type_error branch from587dc3f tofdec321Compare

September 11, 2023 07:56

AleksandrPanov marked this pull request as draft

September 11, 2023 13:01

AleksandrPanov force-pushed thefix_drawDetectedCornersCharuco_type_error branch fromfdec321 to8f24080Compare

September 11, 2023 16:43

asmorkalov reviewed

Sep 11, 2023

View reviewed changes

modules/objdetect/test/test_charucodetection.cpp Outdated

		for (size_t i =0ull; i <4ull; i++) {
		m =moments(contours[i]);
		detectedCenter =Point(cvRound(m.m10/m.m00),cvRound(m.m01/m.m00));
		ASSERT_TRUE(find(detected_int[0].begin(), detected_int[0].end(), detectedCenter) != detected_int[0].end());