Skip to content

fix axes and docs #3256

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

Merged
merged 3 commits into from
Jun 2, 2022
Merged

fix axes and docs #3256

merged 3 commits into from
Jun 2, 2022

Conversation

AleksandrPanov
Copy link
Contributor

@AleksandrPanov AleksandrPanov commented May 18, 2022

fixes #3250
update docs and tutorials for Aruco estimatePoseSingleMarkers()

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 added category: aruco category: documentation Documentation fix or update, does not affect code labels May 18, 2022
@AleksandrPanov AleksandrPanov force-pushed the fix_aruco_axes_docs branch 2 times, most recently from b4ba56c to 3c58d9d Compare May 19, 2022 19:06
@@ -245,7 +266,7 @@ CV_EXPORTS_W void detectMarkers(InputArray image, const Ptr<Dictionary> &diction
* the camera individually. So for each marker, one rotation and translation vector is returned.
* The returned transformation is the one that transforms points from each marker coordinate system
* to the camera coordinate system.
* The marker corrdinate system is centered on the middle of the marker, with the Z axis
* The marker coordinate system is centered on the top-left corner of the marker, with the Z axis
Copy link
Contributor

Choose a reason for hiding this comment

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

"top-left corner" is against default behaviour:

    EstimateParameters() {
        pattern = PatternPos::CCW_center;
        useExtrinsicGuess = false;
        solvePnPMethod = SOLVEPNP_ITERATIVE;
    }

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

@@ -286,7 +286,7 @@ translation vectors of the estimated poses will be in the same unit
- The output parameters `rvecs` and `tvecs` are the rotation and translation vectors respectively, for each of the markers
in `markerCorners`.

The marker coordinate system that is assumed by this function is placed at the center of the marker
The marker coordinate system that is assumed by this function is placed in the top left corner of the marker
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 for documentation here.

Copy link
Contributor Author

@AleksandrPanov AleksandrPanov Jun 1, 2022

Choose a reason for hiding this comment

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

*refixed

@asmorkalov
Copy link
Contributor

Please add documentation for the new parameters structure and add reference to it. The default behaviour should be presumed.

@asmorkalov
Copy link
Contributor

In file included from /build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/src/aruco.cpp:40:0:
/build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/include/opencv2/aruco.hpp: In constructor 'cv::aruco::EstimateParameters::EstimateParameters()':
/build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/include/opencv2/aruco.hpp:235:19: error: 'PatternPos' is not a class or namespace
         pattern = PatternPos::CCW_center;
                   ^
/build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/src/aruco.cpp: In function 'void cv::aruco::_getSingleMarkerObjectPoints(float, cv::OutputArray, cv::aruco::EstimateParameters)':
/build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/src/aruco.cpp:823:59: error: 'cv::aruco::EstimateParameters::PatternPos' is not a class or namespace
     if (estimateParameters.pattern == EstimateParameters::PatternPos::CW_top_left_corner) {
                                                           ^
/build/precommit-contrib_linux64/3.4/opencv_contrib/modules/aruco/src/aruco.cpp:829:64: error: 'cv::aruco::EstimateParameters::PatternPos' is not a class or namespace
     else if (estimateParameters.pattern == EstimateParameters::PatternPos::CCW_center) {
                                                                ^
make[2]: *** [modules/aruco/CMakeFiles/opencv_aruco.dir/src/aruco.cpp.o] Error 1

@AleksandrPanov AleksandrPanov changed the title fix axes docs fix axes and docs May 25, 2022
@AleksandrPanov AleksandrPanov marked this pull request as ready for review May 30, 2022 07:51
* The coordinates of the four corners of the marker in its own coordinate system are:
* (0, 0, 0), (markerLength, 0, 0),
* (markerLength, markerLength, 0), (0, markerLength, 0)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that CCW and CW meaning should be covered too.

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

@@ -810,19 +810,30 @@ static void _identifyCandidates(InputArray _image, vector< vector< vector< Point


/**
* @brief Return object points for the system centered in a single marker, given the marker length
* @brief Return object points for the system centered in a top left corner of single marker, given the marker length
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to cover all function parameters. Center coordinate system is default behaviour, but not the only one.

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_aruco_axes_docs branch 2 times, most recently from 899a218 to 5f780e2 Compare May 31, 2022 16:09
@asmorkalov
Copy link
Contributor

@alalek The patch ready for merge.

@alalek alalek merged commit 0eda296 into opencv:3.4 Jun 2, 2022
*/
static void _getSingleMarkerObjectPoints(float markerLength, OutputArray _objPoints) {
static void _getSingleMarkerObjectPoints(float markerLength, OutputArray _objPoints,
EstimateParameters estimateParameters) {
Copy link
Member

Choose a reason for hiding this comment

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

const reference.

@@ -1221,17 +1233,17 @@ class SinglePoseEstimationParallel : public ParallelLoopBody {
public:
SinglePoseEstimationParallel(Mat& _markerObjPoints, InputArrayOfArrays _corners,
InputArray _cameraMatrix, InputArray _distCoeffs,
Mat& _rvecs, Mat& _tvecs)
Mat& _rvecs, Mat& _tvecs, EstimateParameters _estimateParameters)
Copy link
Member

Choose a reason for hiding this comment

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

const reference

@@ -1242,21 +1254,20 @@ class SinglePoseEstimationParallel : public ParallelLoopBody {
InputArrayOfArrays corners;
InputArray cameraMatrix, distCoeffs;
Mat& rvecs, tvecs;
EstimateParameters estimateParameters;
Copy link
Member

Choose a reason for hiding this comment

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

const reference


CV_Assert(markerLength > 0);

Mat markerObjPoints;
_getSingleMarkerObjectPoints(markerLength, markerObjPoints);
_getSingleMarkerObjectPoints(markerLength, markerObjPoints, *estimateParameters);
Copy link
Member

Choose a reason for hiding this comment

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

unchecked pointer unreference.

* These pattern points define this coordinate system:
* ![Image with axes drawn](images/singlemarkersaxes.jpg)
*/
CCW_center,
Copy link
Member

@alalek alalek Jun 4, 2022

Choose a reason for hiding this comment

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

Missing prefix for enum and its values violates OpenCV coding style guide.

Constants MUST be UPPERCASE (defined by the same coding style guide)

@alalek alalek mentioned this pull request Jun 4, 2022
hakaboom pushed a commit to hakaboom/opencv_contrib that referenced this pull request Jul 1, 2022
fix axes and docs

* fix axes docs, tutorial and add estimateParameters, change estimateParameters in test

* update docs, add singlemarkersaxes2.jpg

* fix docs
@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
category: aruco category: documentation Documentation fix or update, does not affect code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants