-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Update ArUco tutorial #3126
Update ArUco tutorial #3126
Conversation
5fd0f91
to
deaa392
Compare
13ff2da
to
2c618f8
Compare
2c618f8
to
175c508
Compare
894f98d
to
e2a3014
Compare
5b6b44f
to
3d855aa
Compare
@alalek, @asmorkalov, @mshabunin, could you review this PR? |
03fc10f
to
9f2b371
Compare
9f2b371
to
50dc184
Compare
@alalek could you look and merge. |
@@ -0,0 +1,110 @@ | |||
// This file is part of OpenCV project. | |||
// It is subject to the license terms in the LICENSE file found in the top-level directory | |||
// of this distribution and at http://opencv.org/license.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Samples doesn't have license header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// of this distribution and at http://opencv.org/license.html. | ||
|
||
#ifndef _SAMPLES_UTILITY_HPP_ | ||
#define _SAMPLES_UTILITY_HPP_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need guard here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#include <opencv2/calib3d.hpp> | ||
#include <ctime> | ||
|
||
inline bool readCameraParameters(std::string filename, cv::Mat &camMatrix, cv::Mat &distCoeffs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline -> static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI throw warning to static functions:
warning: defined but not used [-Wunused-function]
what to do in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because not every sample uses all functions from samples_utility.hpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should use static inline
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These attached messages comes from tests compilation, not sample code.
Anonymous namespace may help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anonymous namespace didn't help ((( (warnings are still there due to tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
include of aruco_samples_utility.hpp
is removed from tests
@@ -44,6 +44,7 @@ the use of this software, even if advised of the possibility of such damage. | |||
#include <vector> | |||
#include <iostream> | |||
#include <ctime> | |||
#include "samples_utility.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"samples_utility.hpp" -> "aruco_samples_utility.hpp"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -36,7 +23,7 @@ void createBoard() | |||
} | |||
|
|||
//! [detwcp] | |||
void detectCharucoBoardWithCalibrationPose() | |||
inline void detectCharucoBoardWithCalibrationPose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add static inline
to avoid warning: defined but not used [-Wunused-function]
@@ -36,8 +36,9 @@ or tort (including negligence or otherwise) arising in any way out of | |||
the use of this software, even if advised of the possibility of such damage. | |||
*/ | |||
|
|||
|
|||
#include <opencv2/core/utils/logger.defines.hpp> | |||
#include "test_precomp.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
precomp must go first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#include "test_precomp.hpp" | ||
#include "../samples/samples_utility.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test should not rely on samples code.
Samples code should not be over-complicated by tests specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need functions from samples_utility.hpp
to advoid code duplication in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also some ArUco samples just don't work (in 3.4/4.x). Maybe using functions in a test will allow CI to check workability of samples.
I understand, that "test should not rely on samples code". But I don't know how check workability of samples in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek I can do this:
- add
readDetectorParameters()
to ArUco API - add
readDictionary()
to ArUco API - remove
aruco_samples_utility.hpp
- just reinsert
readCameraParameters()
andsaveCameraParams()
to samples/tests code.
Would it be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense (using cv::FileNode
instead of std::string
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek it should look something like this?
template<typename T>
inline void readParameter(FileNode node, T& parameter)
{
if (!node.empty())
node >> parameter;
}
/**
* @brief Read a new set of DetectorParameters from FileStorage.
*/
Ptr<DetectorParameters> DetectorParameters::readDetectorParameters(const Ptr<FileStorage>& pfs)
{
CV_Assert(!pfs.empty());
const FileStorage& fs = *pfs;
Ptr<DetectorParameters> params = DetectorParameters::create();
readParameter(fs["adaptiveThreshWinSizeMin"], params->adaptiveThreshWinSizeMin);
readParameter(fs["adaptiveThreshWinSizeMax"], params->adaptiveThreshWinSizeMax);
readParameter(fs["adaptiveThreshWinSizeStep"], params->adaptiveThreshWinSizeStep);
readParameter(fs["adaptiveThreshConstant"], params->adaptiveThreshConstant);
readParameter(fs["minMarkerPerimeterRate"], params->minMarkerPerimeterRate);
readParameter(fs["maxMarkerPerimeterRate"], params->maxMarkerPerimeterRate);
readParameter(fs["polygonalApproxAccuracyRate"], params->polygonalApproxAccuracyRate);
readParameter(fs["minCornerDistanceRate"], params->minCornerDistanceRate);
readParameter(fs["minDistanceToBorder"], params->minDistanceToBorder);
readParameter(fs["minMarkerDistanceRate"], params->minMarkerDistanceRate);
readParameter(fs["cornerRefinementMethod"], params->cornerRefinementMethod);
readParameter(fs["cornerRefinementWinSize"], params->cornerRefinementWinSize);
readParameter(fs["cornerRefinementMaxIterations"], params->cornerRefinementMaxIterations);
readParameter(fs["cornerRefinementMinAccuracy"], params->cornerRefinementMinAccuracy);
readParameter(fs["markerBorderBits"], params->markerBorderBits);
readParameter(fs["perspectiveRemovePixelPerCell"], params->perspectiveRemovePixelPerCell);
readParameter(fs["perspectiveRemoveIgnoredMarginPerCell"], params->perspectiveRemoveIgnoredMarginPerCell);
readParameter(fs["maxErroneousBitsInBorderRate"], params->maxErroneousBitsInBorderRate);
readParameter(fs["minOtsuStdDev"], params->minOtsuStdDev);
readParameter(fs["errorCorrectionRate"], params->errorCorrectionRate);
return params;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileNode node
const reference.
Ptr<FileStorage>&
FileNode instead of FileStorage as mentioned before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
TEST(CV_ArucoTutorial, can_find_singlemarkersoriginal) | ||
{ | ||
string img_path = samples::findFile("../opencv_contrib/modules/aruco/tutorials/aruco_detection/images" | ||
"/singlemarkersoriginal.jpg", true, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
samples::findFile
Test code should not use samples data.
../opencv_contrib/modules
Wrong way.
Check "text" module tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!)
check opencv_contrib/modules/aruco/CMakeLists.txt
48d852d
to
63ad95a
Compare
modules/aruco/src/aruco.cpp
Outdated
@@ -98,6 +98,44 @@ Ptr<DetectorParameters> DetectorParameters::create() { | |||
return params; | |||
} | |||
|
|||
template<typename T> | |||
inline void readParameter(FileNode node, T& parameter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static inline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#include <ctime> | ||
|
||
namespace { | ||
inline static bool readCameraParameters(std::string filename, cv::Mat &camMatrix, cv::Mat &distCoeffs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid indentation in namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
#include "test_precomp.hpp" | ||
#include "../samples/samples_utility.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileNode node
const reference.
Ptr<FileStorage>&
FileNode instead of FileStorage as mentioned before.
return 0; | ||
} | ||
FileStorage fs(parser.get<string>("dp"), FileStorage::READ); | ||
detectorParams = aruco::DetectorParameters::readDetectorParameters(fs["aruco_detector_params"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.root()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
c3e1180
to
5bbda1c
Compare
@alalek, thx for review ^_^ |
…tion photos, add tests, add aruco_samples_utility.hpp
5bbda1c
to
a965b9b
Compare
@alalek could you check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Some ArUco tutorial examples (Detection of ArUco Boards and Detection of ChArUco Corners) aren't reproduced:
This PR:
tutorial_dict.yml
) and custom detector parameters (the required flags have been added to the examples).New ChArUco board images
Also this PR:
readDetectorParameters()
andreadDictionary()
.samples_utility.hpp
withreadCameraParameters()
,saveCameraParams()
.can_find_singlemarkersoriginal
,can_find_gboriginal
,can_find_choriginal
,can_find_chocclusion
,can_find_diamondmarkers
.tutorial_dict.yml
andtutorial_camera_params.yml
.calibration samples
tutorial_camera_charuco.yml
(created bycalibrate_camera_charuco.cpp
and calibration samples).Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.