-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[GSoC] Add Submaps and PoseGraph optimization for Large Scale Depth Fusion #2619
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
Conversation
Merge opencv master
- Implement Integrate function (untested)
- Clean up comments and few fixes
- Format code - Delete kinfu_back.cpp
- Address Review comments
- Fix formatting according to comments - Move volume abstract class to include/opencv2/rgbd/ - Write factory method for creating TSDFVolumes - Small bug fixes - Minor fixes according to comments
- Fix raycasting bug causing to loop forever - Suppress warnings by explicit conversion - Disable hashTsdf test until we figure out memory leak - style changes - Add missing license in a few files, correct precomp.hpp usage
Implement overlap_ratio check to create new submaps
(Huber threshold based inliers pending)
- Simplify calculation of visibleBlocks
|
||
That's why you need to set the OPENCV_ENABLE_NONFREE option in CMake to use KinectFusion. | ||
*/ | ||
class CV_EXPORTS_W LargeKinfu |
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.
Docs should be rewritten later
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.
Have modified the docs a bit. Do let me know if there are any concerns or issues.
voxelSizeInv(1.0f / voxelSize), | ||
pose(_pose), | ||
raycastStepFactor(_raycastStepFactor) | ||
: voxelSize(_voxelSize), voxelSizeInv(1.0f / voxelSize), pose(_pose), raycastStepFactor(_raycastStepFactor) |
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.
The code in this file is under active refactoring in another PR, a rebase will be required later.
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.
int unitResolution = 0; | ||
|
||
/** @brief Initial pose of the volume in meters */ | ||
cv::Affine3f pose; |
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 use Matx44f
instead, Affine3f
has problems with Python 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.
As far as I know Affine3f coerces the matrix into SE3, i.e., the R matrix has determinant always +1, so changing to Matx44f is not advisable I think.
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.
In fact nothing stops a user from spoiling Affine3f
since:
- it can be constructed from arbitrary 4x4 matrix (or 3x3 R + 1x3 t matrices)
- it keeps the same
Matx44f
inside as a public member
So in both cases a user should be careful about the 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.
Done. Changed most interfaces into Matx44f. Personally though I think we should consider adding a proper SE3 pose class
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 are plans to add quaternions and possibly dual quaternions into OpenCV 5.0. If they are done properly, there will be consistent data structures to keep both SO(3) and SE(3) transforms.
|
||
CV_WRAP virtual void reset() = 0; | ||
|
||
virtual const Affine3f getPose() 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.
Please use Matx44f instead, Affine3f has problems with Python 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.
#include <opencv2/viz.hpp> | ||
#endif | ||
|
||
static vector<string> readDepth(std::string fileList); |
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 think common pieces from all demos like DepthSource
etc. can be moved to a single file later.
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 this, and moved code for kinfu_demo and large_kinfu_demo into a single io_utils.hpp file. Have not modified dynafu_demo code. Let me know if that is required.
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.
- Great! Please move this header file from public headers to samples directory, I think the functions it contains make a little sense without these demos.
- You may also modify dynafu_demo.
|
||
PoseConstraint() : weight(0){}; | ||
|
||
void accumulatePose(const Affine3f& _pose, int _weight = 1) |
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.
Is this pose averaging method good enough for constraint estimation? I mean, rotation matrix average is almost never a rotation matrix again. If you need a good method for pose averaging, I can provide you with DQB src which I'm going to integrate in one of my next PRs.
Or is this used just to estimate mean/stddev for matrix coeffs? If so, why not collecting axis-angle vectors 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.
I think this pose averaging method is good enough. Here, the resulting matrix always represents the same relative pose and is coerced back into SE3 using Affine3f I think. Correct me If I'm wrong. Let's discuss this offline.
inlierConstraint += fromSubmapData.constraints[i].matrix; | ||
} | ||
} | ||
inlierConstraint /= float(max(localInliers, 1)); |
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.
The same about average pose
modules/rgbd/src/volume.cpp
Outdated
params.depthTruncThreshold = 0.f; // depthTruncThreshold not required for TSDF | ||
break; | ||
case VolumeType::HASHTSDF: | ||
params.unitResolution = 16; |
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.
Unused fields like resolution
and unitResolution
should be marked as such, like it's done with depthTruncThreshold
//! Compose current pose and add to camera trajectory | ||
currTrackingSubmap->composeCameraPose(affine); | ||
std::cout << "Number of inliers: " << numInliers << "\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.
Should we continue if tracking 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.
Since we are tracking each submap independently, we should go forward. I am yet to add some code to remove the submap from active list, if the tracking fails consistently for a few frames, etc.
modules/rgbd/src/pose_graph.cpp
Outdated
Vec6f delta = deltaVec.rowRange(6 * currentNodeId, 6 * (currentNodeId + 1)); | ||
Affine3f pose = nodes.at(currentNodeId).getPose(); | ||
Affine3f currDelta = Affine3f(Vec3f(delta.val), Vec3f(delta.val + 3)); | ||
Affine3f updatedPose = currDelta * pose; |
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.
Be careful with Affine3f
multiplication (here and everywhere else), looks like they are multiplied in reverse order compared to pose matrices.
- Minor changes to Gauss newton and Sparse matrix. Residual still increases slightly over iterations
p.volumeParams.tsdfTruncDist = 7 * p.volumeParams.voxelSize; // about 0.04f in meters | ||
p.volumeParams.maxWeight = 64; // frames | ||
p.volumeParams.raycastStepFactor = 0.25f; // in voxel sizes | ||
p.volumeParams.depthTruncThreshold = p.truncateThreshold; |
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.
Does this thing duplicate the params in volume.cpp
?
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.
Yes, pretty much. Since there is no copy constructor for volumeParams, this is done.
modules/rgbd/src/large_kinfu.cpp
Outdated
// TODO: Convert constraints to posegraph | ||
PoseGraph poseGraph = submapMgr->MapToPoseGraph(); | ||
std::cout << "Created posegraph\n"; | ||
Optimizer::optimizeCeres(poseGraph); |
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 change the name to optimize()
, i.e. w/o mentioning a backend's name. This can be changed later.
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.
Recent PR merge #2629 is possibly causing a problem with tracking in the demo (kinfu_demo and large_kinfu_demo). Will need to debug more. |
The fix for the PR #2629 is going to be delivered soon. As soon as it's done, this PR should be build-fixed and merged too. |
@akashsharma02 will you fix this or should I do that? |
@akashsharma02 when you're ready, please rename this PR: remove |
👍 |
The build will break with the new version of Ceres, 2.0.0, which just got released. Main issue probably is that it now requires C++14. Same issue as with the sfm module (see #2578). Maybe someone could check if it's possible to use C++14 mode for at least the files that depend on ceres, if ceres version >= 2.0.0 was found? |
The true solution should be to get rid of Ceres in this module. |
Thanks for the response. Makes sense. In the meantime, looks like the immediate issue regarding compilation with ceres 2.0 got fixed in #2732. |
This work is part of GSoC and is for the 2nd and 3rd part of the evaluation phase of the program.
My mentor for the GSoC time period is: @savuor
My proposal is available here: https://summerofcode.withgoogle.com/dashboard/project/6190777371197440/details/
Checklist before review:
Pull Request Readiness Checklist