Skip to content

Conversation

@DumDereDum
Copy link
Member

@DumDereDum DumDereDum commented Jan 27, 2022

Main PR: opencv/opencv#21189

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

//! @addtogroup kinect_fusion
//! @{

struct CV_EXPORTS_W VolumeParams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this class is in this file? It's not used in kinfu.cpp or anywhere else.

Copy link
Member Author

@DumDereDum DumDereDum Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number of voxels in each dimension for volumeUnit
Applicable only for hashTSDF.
*/
CV_PROP_RW int unitResolution = { 0 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just = 0;


other parameters should be initialized too (by default ctor, see below existing code).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/** @brief Default set of parameters that provide higher quality reconstruction
at the cost of slow performance.
*/
CV_WRAP static Ptr<VolumeParams> defaultParams(int _volumeType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, underscore prefix in public header.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/** @brief Default set of parameters that provide higher quality reconstruction
at the cost of slow performance.
*/
CV_WRAP static Ptr<VolumeParams> defaultParams(int _volumeType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int

enum

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/** @brief Kind of Volume
Values can be TSDF (single volume) or HASHTSDF (hashtable of volume units)
*/
CV_PROP_RW int kind;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int

Why int? You have already defined enum type above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

vs.setTsdfTruncateDistance(params.volumeParams.tsdfTruncDist);
vs.setMaxWeight(params.volumeParams.maxWeight);
vs.setVolumeResolution(Vec3i(params.volumeParams.unitResolution,
params.volumeParams.unitResolution, params.volumeParams.unitResolution));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broken indentation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@alalek
Copy link
Member

alalek commented Feb 8, 2022

Link on the main "opencv" PR must be added in the description.

CV_PROP_RW float voxelSize = volumSize / 512.f;

/** @brief TSDF truncation distance
Distances greater than value from surface will be truncated to 1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line is necessary after @brief here and below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CV_PROP_RW VolumeType kind = VolumeType::TSDF;

/** @brief Resolution of voxel space
Number of voxels in each dimension.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line is necessary after @brief line here and below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

struct CV_EXPORTS_W VolumeParams
{
/** @brief Kind of Volume
Values can be TSDF (single volume) or HASHTSDF (hashtable of volume units)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broken indentation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

params.resolutionZ = 512;
params.voxelSize = volumeSize / 512.f;
params.depthTruncThreshold = 0.f; // depthTruncThreshold not required for TSDF
params.tsdfTruncDist = 7 * params.voxelSize; //! About 0.04f in meters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//!

There is not doxygen in the source code.
Comment is incorrect due to missing <.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +31 to +34
*/
CV_PROP_RW int resolutionX = 128;
CV_PROP_RW int resolutionY = 128;
CV_PROP_RW int resolutionZ = 128;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each member should be documented.

Why we can't use Vec3i?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was in the previous version of the code and this structure will be removed in future

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The true reason to use 3 ints instead of Vec3i is that Vec3i does not work well with bindings generator

Comment on lines 81 to 82
};
struct CV_EXPORTS_W Params
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty space between declarations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@alalek alalek mentioned this pull request Feb 18, 2022
6 tasks
@alalek alalek merged commit 119fb7e into opencv:5.x Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants