[GSoC] Add new Hashtable based TSDF volume to RGBD module #2566

akashsharma02 · 2020-06-10T18:12:26Z

This work is part of GSoC and is potentially for the first 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/

Please note that this is still Work in Progress, as the system is not yet reliable and does not work well.
Making an initial PR as requested for early code review.

Pull Request Readiness Checklist

I agree to contribute to the project under OpenCV (BSD) License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch

Merge opencv master

- Implement Integrate function (untested)

- Clean up comments and few fixes

- Format code - Delete kinfu_back.cpp

savuor

Will review algorithmic part again if there are some glitches.
Will repeat a review for class interface details and so on.

savuor · 2020-06-11T14:08:14Z

modules/rgbd/samples/kinfu_demo.cpp

@@ -26,39 +27,39 @@ static vector<string> readDepth(std::string fileList)
    vector<string> v;

    fstream file(fileList);
-    if(!file.is_open())
+    if (!file.is_open())


Please restore formatting of kinfu_demo: it's hard to see what changes are significant and which are not. Of course, this can be left where formatting is really awful.

not done (please point on commit with non-style changes in this file)

Reverted all the style changes in the file. Only added useHashTSDF parameter to the demo

savuor · 2020-06-11T14:08:46Z

modules/rgbd/src/kinfu.cpp

@@ -60,7 +61,8 @@ Ptr<Params> Params::defaultParams()

    // default pose of volume cube
    p.volumePose = Affine3f().translate(Vec3f(-volSize/2.f, -volSize/2.f, 0.5f));
-    p.tsdf_trunc_dist = 0.04f; //meters;
+    /* p.volumePose  = Affine3f(); */
+    p.tsdf_trunc_dist = 5 * p.voxelSize; //meters;


An experimental thing?

The pose change is an experimental thing. However, in general, it is better to have the truncation distance as a multiple to voxel size, as opposed to a constant, because with different resolutions (coarse or fine) the integration might become problematic when truncation distance goes below voxel size.

So please adjust tsdf_trunc_dist so that it's close to previous value of 0.04f to maintain compatibility.

Done. Changed to 7 * voxel_size for 512 resolution and 2* voxel_size for 128 resolution

modules/rgbd/src/kinfu.cpp

modules/rgbd/src/tsdf.cpp

modules/rgbd/src/hash_tsdf.cpp

savuor · 2020-06-12T23:52:59Z

modules/rgbd/src/hash_tsdf.cpp

+                        stepSize   = tstep;
+                    }
+                    //! Surface crossing
+                    if (prevTsdf > 0.f && currTsdf <= 0.f && currWeight > 0)


The loop should stop at surface crossing, even if NaN and Inf conditions are false

This is not resolved yet and there is a problem:
If surface crossing happened and tInterp is not a good number (which is possible if prevTsdf is close to currTsdf: this leads to Inf or even NaN if stepSize is also small so that tcurr is close to tprev)
OR
If the voxels are OK but their neighbours are not good so that it's impossible to get normal
THEN
We skip this step and search for the next point on a ray.
BUT we shouldn't.
A loop should be finished if we faced a surface, even if we cannot get feasible points/normals from it (in that case we should indicate it with NaNs).

What should be done is to run loop until surface crossing, mark it with a bool, after the loop check that bool and if it's true then perform the interpolation, normal calculation and so on, if it's false then write NaNs to output pixels.

I had fixed this by moving the break outside the (is tInterp invalid check), but it seems it was not pushed correctly for some reason. I think just breaking the loop regardless of the tInterp value at a surface crossing solves the issue.

modules/rgbd/src/hash_tsdf.cpp

- Address Review comments

savuor

Please take a look at my comments (including old, not answered ones)

modules/rgbd/include/opencv2/rgbd/kinfu.hpp

modules/rgbd/samples/kinfu_demo.cpp

savuor · 2020-06-16T13:06:54Z

modules/rgbd/include/opencv2/rgbd/kinfu.hpp

@@ -210,13 +222,13 @@ class CV_EXPORTS_W KinFu

        @param points vector of points which are 4-float vectors
     */
-    CV_WRAP virtual void getPoints(OutputArray points) const = 0;
+    /* CV_WRAP virtual void getPoints(OutputArray points) const = 0; */


TODO: getPoints and getNormals for HashTSDF

Done, but not tested. Will write as part of tests.

savuor · 2020-06-16T14:05:54Z

modules/rgbd/src/kinfu.cpp

@@ -60,7 +61,8 @@ Ptr<Params> Params::defaultParams()

    // default pose of volume cube
    p.volumePose = Affine3f().translate(Vec3f(-volSize/2.f, -volSize/2.f, 0.5f));
-    p.tsdf_trunc_dist = 0.04f; //meters;
+    /* p.volumePose  = Affine3f(); */
+    p.tsdf_trunc_dist = 5 * p.voxelSize; //meters;


So please adjust tsdf_trunc_dist so that it's close to previous value of 0.04f to maintain compatibility.

modules/rgbd/src/kinfu.cpp

modules/rgbd/src/tsdf.cpp

savuor

For now this pull request looks almost ready. According to my comments there is a list of things TODO:

fix raycast loop, surface crossing
minors in hash_tsdf.cpp: extra print, float16
Expose volumes to public interface:
- move volume.hpp to include/opencv2/rgbd directory
- add a factory to this header: a function with a name for example makeVolume which takes an enum and a set of parameters like pose, voxelSize, truncDist, raymarchStep and returns TSDFVolume or HashTSDFVolume as cv::Ptr<Volume>. This should look like what's going on in KinFuImpl constructor you wrote.
Make sure that the demo still works
Fix tests

savuor · 2020-06-20T22:17:11Z

modules/rgbd/src/hash_tsdf.cpp

+                        stepSize   = tstep;
+                    }
+                    //! Surface crossing
+                    if (prevTsdf > 0.f && currTsdf <= 0.f && currWeight > 0)


This is not resolved yet and there is a problem:
If surface crossing happened and tInterp is not a good number (which is possible if prevTsdf is close to currTsdf: this leads to Inf or even NaN if stepSize is also small so that tcurr is close to tprev)
OR
If the voxels are OK but their neighbours are not good so that it's impossible to get normal
THEN
We skip this step and search for the next point on a ray.
BUT we shouldn't.
A loop should be finished if we faced a surface, even if we cannot get feasible points/normals from it (in that case we should indicate it with NaNs).

savuor · 2020-06-20T22:19:22Z

modules/rgbd/src/hash_tsdf.cpp

+                        stepSize   = tstep;
+                    }
+                    //! Surface crossing
+                    if (prevTsdf > 0.f && currTsdf <= 0.f && currWeight > 0)


What should be done is to run loop until surface crossing, mark it with a bool, after the loop check that bool and if it's true then perform the interpolation, normal calculation and so on, if it's false then write NaNs to output pixels.

modules/rgbd/src/hash_tsdf.cpp

- 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

savuor · 2020-06-25T11:38:59Z

👍

alalek

Thank you for contribution!

Several coding style notes are below.

modules/rgbd/include/opencv2/rgbd/intrinsics.hpp

alalek · 2020-06-27T08:22:44Z

modules/rgbd/include/opencv2/rgbd/intrinsics.hpp

+namespace cv
+{
+    namespace kinfu
+    {


Avoid indentation in namespaces.

modules/rgbd/include/opencv2/rgbd/intrinsics.hpp

alalek · 2020-06-27T08:25:48Z

modules/rgbd/include/opencv2/rgbd/volume.hpp

+// module's directory
+
+#ifndef __OPENCV_VOLUME_H__
+#define __OPENCV_VOLUME_H__


OPENCV_VOLUME_H

Missing module name: __OPENCV_RGBD_VOLUME_H__

alalek · 2020-06-27T08:26:44Z

modules/rgbd/include/opencv2/rgbd/volume.hpp

+class Volume
+{
+   public:
+    explicit Volume(float _voxelSize, cv::Affine3f _pose, float _raycastStepFactor)


explicit

Makes sense for cases with single parameter only.

alalek · 2020-06-27T08:36:01Z

modules/rgbd/samples/kinfu_demo.cpp

@@ -26,39 +27,39 @@ static vector<string> readDepth(std::string fileList)
    vector<string> v;

    fstream file(fileList);
-    if(!file.is_open())
+    if (!file.is_open())


not done (please point on commit with non-style changes in this file)

alalek · 2020-06-27T08:37:02Z

modules/rgbd/src/hash_tsdf.cpp

+// This code is also subject to the license terms in the LICENSE_KinectFusion.md file found in this
+// module's directory


perhaps not related to this functionality

modules/rgbd/src/hash_tsdf.cpp

alalek · 2020-06-27T08:40:44Z

modules/rgbd/src/hash_tsdf.cpp

+    mutable Mutex mutex;
+};
+
+struct VolumeUnitInFrustumInvoker : ParallelLoopBody


consider using of lambda functions instead for all ParallelLoopBody stuff

Do we prefer lambdas over functor objects just because of coding style or ParallelLoopBody misses some important features?

I have changed a few small parallel_for_ to lambdas.

modules/rgbd/src/hash_tsdf.hpp

modules/rgbd/src/tsdf.hpp

modules/rgbd/src/utils.hpp

modules/rgbd/src/volume.cpp

- 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

akashsharma02 · 2020-07-03T21:16:18Z

Hi @alalek, I have fixed all the review comments, can the PR be merged? Or is there something missing from my side?

akashsharma02 and others added 10 commits March 21, 2020 20:32

Merge pull request #1 from opencv/master

4dbdee4

Merge opencv master

Merge remote-tracking branch 'upstream/master'

dcbe5a8

Merge remote-tracking branch 'upstream/master'

a7c3ab0

- Add HashTSDF class

3fa8a3c

- Implement Integrate function (untested)

Integration seems to be working, raycasting does not

98ce3d0

Update integration code

01a172c

Integration and Raycasting fixes, (both work now)

5a3f537

- Format code

9b5b0ce

- Clean up comments and few fixes

Add Kinect Fusion backup file

bdbafd0

- Add interpolation for vertices and normals (slow and unreliable!)

70b1dfb

- Format code - Delete kinfu_back.cpp

savuor self-requested a review June 11, 2020 13:39

Bug fix for integration and noisy odometry

fcfd6dd

savuor reviewed Jun 13, 2020

View reviewed changes

- Create volume abstract class

ddf5c18

- Address Review comments

savuor reviewed Jun 16, 2020

View reviewed changes

savuor reviewed Jun 20, 2020

View reviewed changes

akashsharma02 force-pushed the master branch from 24e3147 to 6facabe Compare June 21, 2020 17:53

- Add getPoints and getNormals function

a00d7eb

- 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

akashsharma02 force-pushed the master branch 6 times, most recently from 2b0c153 to d15ae3b Compare June 24, 2020 21:21

savuor self-assigned this Jun 24, 2020

alalek reviewed Jun 27, 2020

View reviewed changes

vpisarev changed the title ~~Add new Hashtable based TSDF volume to RGBD module~~ [GSoC] Add new Hashtable based TSDF volume to RGBD module Jun 28, 2020

akashsharma02 force-pushed the master branch from d15ae3b to 6198a3e Compare June 29, 2020 19:26

alalek reviewed Jun 30, 2020

View reviewed changes

modules/rgbd/src/hash_tsdf.hpp Outdated Show resolved Hide resolved

modules/rgbd/src/tsdf.hpp Outdated Show resolved Hide resolved

modules/rgbd/src/utils.hpp Outdated Show resolved Hide resolved

modules/rgbd/src/volume.cpp Show resolved Hide resolved

- Add tests for hashTSDF

75656bd

- 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

akashsharma02 force-pushed the master branch from 6198a3e to 75656bd Compare June 30, 2020 18:11

alalek merged commit 4683455 into opencv:master Jul 3, 2020

		// This code is also subject to the license terms in the LICENSE_KinectFusion.md file found in this
		// module's directory

[GSoC] Add new Hashtable based TSDF volume to RGBD module #2566

[GSoC] Add new Hashtable based TSDF volume to RGBD module #2566

Uh oh!

Conversation

akashsharma02 commented Jun 10, 2020

Pull Request Readiness Checklist

Uh oh!

savuor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savuor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

savuor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

savuor commented Jun 25, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!