Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork56.4k
New odometry Pipeline#20755
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
New odometry Pipeline#20755
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| intestimateConstraint(int fromSubmapId,int toSubmapId,int& inliers, Affine3f& inlierPose); | ||
| boolupdateMap(int _frameId,Ptr<OdometryFrame> _frame); | ||
| boolupdateMap(int _frameId, OdometryFrame _frame); |
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 using of underscored parameters in public headers.
They are part of documentation and bindings.
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.
done
| virtualvoidintegrate(InputArray _depth,float depthFactor,const cv::Matx33f& intrinsics,constint currframeId); | ||
| virtualvoidraycast(const cv::Affine3f& cameraPose,const cv::Matx33f& intrinsics, cv::Size frameSize, | ||
| virtualvoidraycast(Odometry icp,const cv::Affine3f& cameraPose,const cv::Matx33f& intrinsics, cv::Size frameSize, |
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.
Still no reference.
Check all other methods and other types, including
bool compute(OdometryFrame srcFrame, OdometryFrame dstFrame, OutputArray Rt);| #include<iostream> | ||
| #include<opencv2/core.hpp> | ||
| #include<opencv2/core/ocl.hpp> | ||
| #include<opencv2/core/mat.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.
iostream
Avoid using of that header (I left detailed comment about that on this month - check project conversations in email).
Use logging.
core.hpp
mat.hpp
Not needed, included by default.
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.
done
| { | ||
| public: | ||
| Impl() {}; | ||
| virtual~Impl() {}; |
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.
redundant semicolon
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.
done
| virtualvoidsetImage(InputArray image) = 0; | ||
| virtualvoidgetImage(OutputArray image) = 0; | ||
| virtualvoidgetGrayImage(OutputArray image) = 0; | ||
| virtualvoidsetDepth(InputArray depth) = 0; | ||
| virtualvoidgetDepth(OutputArray depth) = 0; | ||
| virtualvoidsetMask(InputArray mask) = 0; | ||
| virtualvoidgetMask(OutputArray mask) = 0; | ||
| virtualvoidsetNormals(InputArray normals) = 0; | ||
| virtualvoidgetNormals(OutputArray normals) = 0; | ||
| virtualvoidsetPyramidLevel(size_t _nLevels, OdometryFramePyramidType oftype) = 0; | ||
| virtualvoidsetPyramidLevels(size_t _nLevels) = 0; | ||
| virtualsize_tgetPyramidLevels(OdometryFramePyramidType oftype) = 0; | ||
| virtualvoidsetPyramidAt(InputArray img, | ||
| OdometryFramePyramidType pyrType,size_t level) = 0; | ||
| virtualvoidgetPyramidAt(OutputArray img, | ||
| OdometryFramePyramidType pyrType,size_t level) = 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.
getters should be "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.
done
| const cv::Matx33f defaultCameraMatrix = | ||
| {/* fx, 0, cx*/0,0,0, | ||
| /* 0, fy, cy*/0,0,0, | ||
| /* 0, 0, 0*/0,0,0 }; | ||
| const std::vector<int> defaultIterCounts = {7,7,7,10 }; |
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 declaration of global objects - they slowdown library initialization.
Use class ctors / initialization on-demand instead.
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.
if I understood right, done
| constfloat defaultMinDepth =0.f; | ||
| constfloat defaultMaxDepth =4.f; | ||
| constfloat defaultMaxDepthDiff =0.07f; | ||
| constfloat defaultMaxPointsPart =0.07f; |
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.
local constants must be "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.
done
modules/3d/test/test_odometry.cpp Outdated
| #if SHOW_DEBUG_IMAGES | ||
| imshow("image", image); | ||
| imshow("warpedImage", warpedImage); | ||
| Mat resultImage, resultDepth; | ||
| warpFrame(image, depth, calcRvec, calcTvec, K, resultImage, resultDepth); | ||
| imshow("resultImage", resultImage); | ||
| waitKey(); | ||
| waitKey(100); | ||
| #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.
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.
done
| voidOdometrySettingsImplCommon::getIterCounts(OutputArray val)const | ||
| { | ||
| DefaultSets ds; | ||
| Mat(ds.defaultIterCounts).copyTo(val); |
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.
getter implementation conflicts with setter
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.
done
| template<typename TMat> | ||
| void OdometryFrameImplTMat<TMat>::setPyramidAt(InputArray _img, OdometryFramePyramidType pyrType,size_t level) | ||
| { | ||
| CV_Assert(pyrType < OdometryFramePyramidType::N_PYRAMIDS); |
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.
< pyramids.size() instead of some assumed value
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.
pyramids isvector<vector<TMat>> and in some cases, the levels could be different for different Pyramids.
this is the reason why I use assumed value
besides,pyramids.size() returns the number of pyramids but not levels
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.
Wrong logic.
It is simple.
Code below uses:
pyramids[pyrType]
So assumptions are:
pyrType >= 0pyrType < pyramids.size()
We need just to check them.
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 misunderstood, my bad.
I will fix now
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.
done
| void OdometryFrameImplTMat<TMat>::setPyramidAt(InputArray _img, OdometryFramePyramidType pyrType,size_t level) | ||
| { | ||
| CV_Assert(pyrType >=0); | ||
| CV_Assert(pyrType < pyramids.size()); |
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.
When our branch merged this part of the code, the build gave a WARNING and the logs are athttps://pullrequest.opencv.org/buildbot/builders/precommit_windows32/builds/13578/steps/compile%20release/logs/warnings%20%281%29.
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.
CV_Assert((size_t)pyrType < pyramids.size());
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'll fix it on this PR or create a new one?
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.
Nobody can fix already merged PR.
So create a new one.
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.
Uh oh!
There was an error while loading.Please reload this page.
Pull Request Readiness Checklist
See details athttps://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.