-
-
Notifications
You must be signed in to change notification settings - Fork 55.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
PnP issue 15647 #15650
base: 4.x
Are you sure you want to change the base?
PnP issue 15647 #15650
Conversation
Please don't reopen PRs about the same/similar things. |
Sorry about the double PR. |
minor fixes fix whitespace errors reverted to previous solvePnPGeneric interface removed whitespace errors fixed doc issues fixed doc issues 2 fixed doc issues 3 fixed minor warnings fixed minor warnings 2
1d75d81
to
41f3cdf
Compare
|
||
/** @brief create creates a pointer to a new instance of this class | ||
|
||
@param withGeometricTests set this true if you want to execute geometryWarn before solving pnp. |
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.
this refers to the non existing geometryWarn
(probably validProblem
) function. Use @ref to get an buildbot error for this.
Also I would recommend using @copydoc PnPSolver::PnPSolver
for the create functions to avoid copy & paste
I think documenting the corner cases of the different algorithms alone is an invaluable contribution. I just have some minor code-style suggestions. |
@param _opoints | ||
@return true if correct, false otherwise | ||
*/ | ||
CV_WRAP bool isPlanarTag(InputArray _opoints) const; |
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.
this does not use any members and thus can be static
|
||
@returns true if required, false otherwise | ||
*/ | ||
CV_WRAP virtual bool requires3DObject() const = 0; |
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.
how about using a single int getCaveats() const
function returning a mask like PNP_CAVEAT_PLANAR | PNP_CAVEAT_TAG
instead of the three functions below?
The same mask could be also returned from validProblem
to signal which check failed.
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.
Thank you for working on this improvement!
At first we need to keep library changes compatible. We should not break existing User applications. Or existing tests.
Try unchanged tests (reverted code changes) and run them - there are several failed tests:
[ FAILED ] 5 tests, listed below:
[ FAILED ] Calib3d_SolveP3P.accuracy
[ FAILED ] Calib3d_SolvePnPRansac.double_support
[ FAILED ] Calib3d_SolvePnP.generic
[ FAILED ] Calib3d_SolvePnP.refine3pts
[ FAILED ] Calib3d_SolvePnP.refine
In general, tests should be added only - not changed.
Suggestion from @paroj with flags for geometric restrictions sounds good.
Checks are applied for "object points" only. Sometimes it make sense to handle them once (move to ctor/create to pass them once?).
For example, factory with "object points" can analyze them and selects proper solver (additional to algorithm-specific factories).
//!< - point 1: [ squareLength / 2, squareLength / 2, 0] | ||
//!< - point 2: [ squareLength / 2, -squareLength / 2, 0] | ||
//!< - point 3: [-squareLength / 2, -squareLength / 2, 0] | ||
//!< This is a special case suitable for marker pose estimation.\n |
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.
Please revert these unnecessary whitespace changes (and many other below).
class CV_EXPORTS_W PnPSolver : public Algorithm | ||
#else | ||
class CV_EXPORTS_W PnPSolver : public virtual Algorithm | ||
#endif |
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.
Why virtual
is needed?
|
||
@returns Identity intrinsic matrix (3x3 single channel double) | ||
*/ | ||
CV_WRAP cv::Mat makeIdentityIntrinsic() const; |
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.
protected:
CV_WRAP
This should be removed from public API, so this can't be wrapped.
opoints are defined in object coordinates. | ||
@return | ||
*/ | ||
CV_WRAP size_t getNumberOfPoints(InputArray opoints) const; |
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.
See Mat::checkVector()
the model coordinate system to the camera coordinate system. | ||
@param tvecs Vector of output translation vectors (double) | ||
*/ | ||
CV_WRAP virtual void solve(InputArray opoints, InputArray ipoints, CV_OUT std::vector<Mat> & rvecs, CV_OUT std::vector<Mat> & tvecs) const = 0; |
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.
Why is this method protected? (I believe solveProblem()
should be virtual instead)
|
||
@param withGeometricTests flag | ||
*/ | ||
CV_WRAP PnPSolverP3PComplete(bool withGeometricTests); |
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.
bool withGeometricTests = false
to be consistent with create()
interface
@tobycollins Friendly reminder. Do you have a chance to fix notes? |
jenkins cn please retry a build |
This pullrequest changes
This pull request is designed to fix the problems discussed in #15647
PnPSolver
base class has been created according to the OpenCV algorithm-based design guidelines. This is derived by classes that implement a PnP solver algorithm, and also for specifying important properties of the PnP solver, implemented as virtual member functions. These include:minPointNumber
: specifies the minimal number of points that can be handled by the solver (e.g. EPnP requires 4 and DLT requires 6)maxPointNumber
: specifies the maximal number of points that can be handled by the solver (e.g. AP3P handles at most 4 points, while others can handle an arbitrary number of points)requires3DObject
: specifies if the PnP solver requires non co-planar model pointsrequiresPlanarObject
: specifies if the PnP solver requires co-planar model pointsrequiresPlanarTagObject
: specifies if the PnP solver requires co-planar model points arranged in a square tag formationartificalDegeneracy
: warns if the object points & image points will cause an artificial degeneracyThe
PnPSolver
class provides the functiongeometryWarn
that tests the input object points and image points for whether they will will cause the solver to fail. This involves running basic tests (checking number of points, checking for co-linear points etc.) and also running specific checks for each algorithm, as implemented inartificalDegeneracy
.All the currently used PnP solvers in OpenCV have been implemented as derived classes of
PnPSolver
: EPnP, DLT, DLS, P3P, AP3P, Zhang, IPPE and IPPESquare.A
PnPRefiner
base class has bee created according to the OpenCV algorithm-based design guidelines. This is derived by classes that implements refinement algorithms. All the currently used refinement algorithms in OpenCV have been implemented as derived classes: C API Levenberg Marquardt, C++ API Levenberg Marquardt and VVS.The public interfaces:
solvePnP
andsolvePnPRansac
are unchanged. When these are called,PnPSolver
andPnPRefiner
objects are created according to the flags passed toSolvePnP
andSolvePnPRansac
respectively.The public function:
pnp
has been created. This is used to solve a PnP problem usingPnPSolver
andPnPRefiner
objects. This unifies all PnP functionality in OpenCV, including P3P, in a single interface.A new test set has been created (test_pnp_precision.cpp) that does the following:
Tests
pnp
with all combinations ofPnPSolver
andPnPRefiner
objects for solver precision, using all combinations of precision and formats for object points, image points, intrinsics and distortion coefficients. This exhaustive test makes testing a new PnPSolver or PnPRefiner classes easy.Tests all
PnPRefiner
classes using an imprecise initial pose estimate.Documents corner cases that cause one or more
PnPSolver
to fail (also called artificial degeneracies). This set can grow if more corner cases get included by the community.Credit should also go to @catree and @alalek for the recent work on improving solvePnP #14431 #14362 #14181