-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
VTK based implementation of extract trianglemesh #6648
Conversation
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
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.
Thanks for the PR! Please kindly check my comments. The PR also needs unit tests and benchmarking.
VTK version is more limited (does not allow weights and the perturbation of isosurface). Additional overhead is added through memory copy from TSDFVolume to VTK and tensor mesh to legacy mesh (they can be potentially optimized).
I think it would be fine to merge the PR if we can justify that the VTK version is faster. Otherwise, it might be good to rethink the necessity.
@@ -52,6 +52,8 @@ class UniformTSDFVolume : public TSDFVolume { | |||
const Eigen::Matrix4d &extrinsic) override; | |||
std::shared_ptr<geometry::PointCloud> ExtractPointCloud() override; | |||
std::shared_ptr<geometry::TriangleMesh> ExtractTriangleMesh() override; | |||
/// Temporary function will be deleted/"merged" with ExtractTriangleMesh | |||
std::shared_ptr<geometry::TriangleMesh> ExtractTriangleMesh_v2(); |
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.
It would be better to inject the implementation as an option in ExtractTriangleMesh(bool use_vtk = False)
.
@@ -136,6 +136,8 @@ In SIGGRAPH, 1996)"); | |||
"Debug function to inject the voxel TSDF data.", "tsdf"_a) | |||
.def("inject_volume_color", &UniformTSDFVolume::InjectVolumeColor, | |||
"Debug function to inject the voxel Color data.", "color"_a) | |||
// TODO remove after debugging | |||
.def("extract_triangle_mesh_v2", &UniformTSDFVolume::ExtractTriangleMesh_v2) |
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.
Same, do not duplicate API.
surface->SetValue(0, isoValue); | ||
|
||
//// To remain largest region. | ||
//// vtkNew<vtkPolyDataConnectivityFilter> confilter; |
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.
Either remove unused code or add a flag to run the code.
std::shared_ptr<geometry::TriangleMesh> | ||
UniformTSDFVolume::ExtractTriangleMesh_v2() { | ||
auto mesh = std::make_shared<geometry::TriangleMesh>(); | ||
//float *image_array = new float[resolution_ * resolution_ * resolution_]; |
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.
Remove unused comments.
auto mesh = std::make_shared<geometry::TriangleMesh>(); | ||
//float *image_array = new float[resolution_ * resolution_ * resolution_]; | ||
//const long r_sq = resolution_ * resolution_; | ||
int dims[] = {resolution_, resolution_, resolution_}; |
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.
Use std::array or modern containers, or explicitly declare array size.
//float *image_array = new float[resolution_ * resolution_ * resolution_]; | ||
//const long r_sq = resolution_ * resolution_; | ||
int dims[] = {resolution_, resolution_, resolution_}; | ||
float ***values = new float **[dims[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.
All pointers are allocated but not released. It's better to allocate a flattened std::vector and unroll the indices.
} | ||
} | ||
} | ||
vtkIdType totalSize = (vtkIdType)dims[0] * (vtkIdType) dims[1] * (vtkIdType) dims[2]; |
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.
Use static_cast.
for (int z = 0; z < resolution_ - 1; z++) { | ||
Eigen::Vector3i idx0(x, y, z); | ||
const float f = voxels_[IndexOf(idx0)].tsdf_; | ||
values[x][y][z] = f; |
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.
Only TSDF is used here, weights are not handled, see the original implementation. This is known to provide problematic mesh especially around boundaries.
vtkPolyData *sdata = surface->GetOutput(); | ||
auto tmesh = open3d::t::geometry::vtkutils::CreateTriangleMeshFromVtkPolyData(sdata); | ||
auto lmesh = tmesh.ToLegacy(); | ||
std::shared_ptr<geometry::TriangleMesh> pmesh(new geometry::TriangleMesh(lmesh.vertices_, lmesh.triangles_)); |
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.
Memory is copied twice in the conversion. It is also unsafe to use new to initialize a shared_ptr. Please use std::make_shared and reduce redundant memory copy.
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 implementation is not linked to the voxel grid / voxel block grid anymore. An example of how to use it with data in those formats would make it more accessible.
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.
Thanks! looks good.
Type
Motivation and Context
Checklist:
python util/check_style.py --apply
to apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description