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

aruco refactoring and class interface #3240

Merged
merged 11 commits into from
Aug 5, 2022

Conversation

AleksandrPanov
Copy link
Contributor

@AleksandrPanov AleksandrPanov commented Apr 28, 2022

Preparing ArUco module to move to main opencv repository.

  • add ArucoDetector class (with detectMarkers() and refineDetectedMarkers() methods)
  • class ArucoDetector inherits from class cv::Algorithm and implemented read()/write()
  • add C wrapper for old API (in aruco.hpp/aruco.cpp)
  • add aruco_calib_pose.hpp/aruco_calib_pose.cpp and aruco_utils.cpp/aruco_utils.hpp
  • add Java and Python tests

possible add class BaseBoard to work with calibrateCamera/PoseEstimation/SolvePnP?

To move ArUco module to main opencv repository need to move all files except "aruco.cpp" and "aruco.hpp"

Before merging, it is necessary to make a git squash.

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 force-pushed the aruco_add_class_API branch 7 times, most recently from 5bd62f7 to 36e28be Compare May 4, 2022 17:47
modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco_detector.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco_detector.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/src/aruco_detector.cpp Outdated Show resolved Hide resolved
modules/aruco/src/aruco_detector.cpp Outdated Show resolved Hide resolved
modules/aruco/src/aruco_utils.cpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco/board.hpp Show resolved Hide resolved
@asmorkalov
Copy link
Contributor

@AleksandrPanov AleksandrPanov force-pushed the aruco_add_class_API branch 6 times, most recently from 3614e3c to 5775474 Compare May 12, 2022 18:31
@AleksandrPanov AleksandrPanov marked this pull request as ready for review May 13, 2022 07:49
@AleksandrPanov AleksandrPanov force-pushed the aruco_add_class_API branch 2 times, most recently from c5ade77 to dfd8e9b Compare May 18, 2022 00:41
@vpisarev vpisarev self-requested a review May 20, 2022 09:22
@asmorkalov
Copy link
Contributor

Thanks a lot for the patch! I tested the solution manually and it works for me. Couple of general comments on the code:

  • It make sense to duplicate some of detector tests, but with old API and stay it in contrib after migration. It allows to ensure that wrappers are in "calable" state and presumes original behaviour. They should be in separate test file for easy extraction.
  • There are still several "todo" in code like "// Todo: update other CORNER_REFINE methods". Do you plan to implement this after merge?

CV_EXPORTS_W void estimatePoseSingleMarkers(InputArrayOfArrays corners, float markerLength,
InputArray cameraMatrix, InputArray distCoeffs,
OutputArray rvecs, OutputArray tvecs, OutputArray _objPoints = noArray(),
Ptr<EstimateParameters> estimateParameters = EstimateParameters::create());
Copy link
Contributor

Choose a reason for hiding this comment

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

for now it's fine, but when we move these functions to the main opencv module, can we consider using named parameters concept, which will provide more convenient API, especially for Python and modern C++ users? (https://github.com/opencv/opencv/wiki/OE-34.-Named-Parameters)

Size imageSize, InputOutputArray cameraMatrix, InputOutputArray distCoeffs,
OutputArrayOfArrays rvecs, OutputArrayOfArrays tvecs, OutputArray stdDeviationsIntrinsics,
OutputArray stdDeviationsExtrinsics, OutputArray perViewErrors, int flags = 0,
TermCriteria criteria = TermCriteria(TermCriteria::COUNT + TermCriteria::EPS, 30, DBL_EPSILON));
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is new API, do we need both Extended and simple variants? Can we retain just extended? (and maybe later convert it to "named-parameters" style (see above))

@asmorkalov asmorkalov requested a review from alalek July 4, 2022 08:47
modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco/aruco_calib_pose.hpp Outdated Show resolved Hide resolved
Comment on lines 134 to 142
private:
// number of markers in X and Y directions
int _markersX, _markersY;

// marker side length (normally in meters)
float _markerLength;

// separation between markers in the grid
float _markerSeparation;
Copy link
Member

Choose a reason for hiding this comment

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

no changes and no objectives here

modules/aruco/include/opencv2/aruco/board.hpp Outdated Show resolved Hide resolved
modules/aruco/include/opencv2/aruco_detector.hpp Outdated Show resolved Hide resolved
@AleksandrPanov
Copy link
Contributor Author

@alalek, gridImpl and charucoImpl were added. Need help with getters and setters.

AleksandrPanov and others added 10 commits July 25, 2022 17:31
create aruco_utils.hpp

move Board, GridBoard, CharucoBoard to board.hpp/board.cpp

refactoring _getSingleMarkerObjectPoints()

refactoring _extractBits()

refactoring _findMarkerContours()

fix _copyVector2Output() in detectMarkers()

move testCharucoCornersCollinear() to board.hpp/board.cpp

move poseEstimate()/calibAruco() to aruco_calib_pose.hpp

reduce include files

move detectMarkers() to class ArucoDetector

move refineDetectedMarkers() to class ArucoDetector

add C API wrapper to detectMarkers(), refineDetectedMarkers()

update tests and samples to class API

add py tests: test_aruco_detector, test_aruco_detector_refine

refactoring, fix docs

add java tests: testArucoIssue3133, testArucoDetector

add readWriteParameter(), update readParameter()

implemented cv::Algorithm - read/write, added read/write to RefineParameters, added write to DetectorParameters

merge PatternPos/EstimateParameters after rebase

remove empty docstring for private function

fixes

fixes license
@AleksandrPanov AleksandrPanov force-pushed the aruco_add_class_API branch 4 times, most recently from 92a77df to a8be488 Compare August 2, 2022 21:29
Copy link
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.

👍 LGTM

@asmorkalov asmorkalov merged commit 8eaa8ac into opencv:4.x Aug 5, 2022
@alalek
Copy link
Member

alalek commented Aug 6, 2022

@asmorkalov merged commit 8eaa8ac into opencv:4.x

All 11 commits are merged. They should be squashed instead.

the use of this software, even if advised of the possibility of such damage.
*/

#include "precomp.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

precomp.hpp must be restored.

@@ -7,7 +7,6 @@
#include "opencv2/ts.hpp"
#include "opencv2/imgproc.hpp"
#include "opencv2/calib3d.hpp"
#include "opencv2/aruco.hpp"
#include <opencv2/aruco/charuco.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this change?

If you want to remove module's headers, then remove them all (charuco.hpp is still here).

Currently it looks like that something was broken and there is a hack to hide that (so user code may be broken too).

@alalek alalek mentioned this pull request Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants