Skip to content
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

RemoveDuplicateVertices Implementation for TriangleMesh. #6414

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
* Check for support of CUDA Memory Pools at runtime (#4679)
* Fix `toString`, `CreateFromPoints` methods and improve docs in `AxisAlignedBoundingBox`. 🐛📝
* Migrate Open3d documentation to furo theme ✨ (#6470)
* Implemented functionality for removing duplicate vertices from TriangleMesh (PR #6414).

## 0.13

Expand Down
133 changes: 133 additions & 0 deletions cpp/open3d/t/geometry/TriangleMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1215,6 +1215,139 @@ TriangleMesh TriangleMesh::SelectByIndex(const core::Tensor &indices) const {
return result;
}

namespace {

/// This is a templatized local version of RemoveDuplicates.
/// Use of templates allows us to use common code for different
/// datatype possibilities for vertex.positions, namely, float32, float64,
/// and triangle.indices, namely, int32, int64.
/// This function first computes two maps
/// (MapA.) From vertex coordinates to their indices.
// This is implemented using unordered_map.
/// (MapB.) From the vertex indices before duplicate removal to indices after removal.
/// This is implemented using std::vector.
///
/// MapA allows the function to detect duplicates and MapB allows it update triangle indices.
/// During the computation of the above maps unique vertex attributes including positions
/// are moved to beginning of the corresponding attribute tensor.
///
/// Caveats:
/// The unordered_map approach is fast in general but if the inputs are close, the
/// hashing could cause problems and it might be good to consider a sorting based algorithms,
/// which can do a better job of comparing the double values.
/// \param mesh The mesh from which duplicate vertices are to be removed.
template<typename Coord_t, typename Tindex_t>
TriangleMesh& RemoveDuplicateVerticesWorker(TriangleMesh &mesh)
intelshashi marked this conversation as resolved.
Show resolved Hide resolved
{
//Algorithm based on Eigen implementation
if(!mesh.HasVertexPositions()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please run python util/check_style.py --apply on your changes? There are a few accidental space misses.

utility::LogWarning("TriangeMesh::RemoveDuplicateVertices: No vertices present, ignoring.");
return mesh;
}

typedef std::tuple<Coord_t, Coord_t, Coord_t> Point3d;
std::unordered_map<Point3d, size_t, utility::hash_tuple<Point3d>>
point_to_old_index;

auto vertices = mesh.GetVertexPositions();
auto orig_num_vertices = vertices.GetLength();
const auto vmask_type = core::Int32;
using vmask_itype = int32_t;
core::Tensor vertex_mask = core::Tensor::Zeros({orig_num_vertices}, vmask_type);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the mask is not used to store the remapped indices, I think we can have it as Bool right from the initialization.

using Length_t = decltype(orig_num_vertices);
std::vector<Length_t> index_old_to_new(orig_num_vertices);

//REM_DUP_VERT_STEP 1:
// Compute map from points to old indices, and use a counter
// to compute a map from old indices to new unique indices.
const Coord_t *vertices_ptr = vertices.GetDataPtr<Coord_t>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to move vertices tensor to CPU device, too.

vmask_itype * vertex_mask_ptr = vertex_mask.GetDataPtr<vmask_itype>();
Length_t k = 0; // new index
for (Length_t i = 0; i < orig_num_vertices; ++i) { // old index
Point3d coord = std::make_tuple(vertices_ptr[i * 3], vertices_ptr[i * 3 + 1],
vertices_ptr[i * 3 + 2]);
if (point_to_old_index.find(coord) == point_to_old_index.end()) {
point_to_old_index[coord] = i;
index_old_to_new[i] = k;
++k;
vertex_mask_ptr[i] = 1;
} else {
index_old_to_new[i] = index_old_to_new[point_to_old_index[coord]];
}
}
vertex_mask = vertex_mask.To(mesh.GetDevice(), core::Bool);

//REM_DUP_VERT_STEP 2:
// Update vertex attributes, by shrinking them appropriately.
for (auto item : mesh.GetVertexAttr()) {
mesh.SetVertexAttr(item.first, item.second.IndexGet({vertex_mask}));
}

//REM_DUP_VERT_STEP 3:
// Remap triangle indices to new unique vertex indices, if required.
if (k < orig_num_vertices) {
core::Tensor tris = mesh.GetTriangleIndices();
core::Tensor tris_cpu = tris.To(core::Device()).Contiguous();
const int64_t num_tris = tris_cpu.GetLength();
//Update triangle indices.
Tindex_t *indices_ptr = tris_cpu.GetDataPtr<Tindex_t>();
for(int64_t i = 0; i < num_tris * 3; ++i) {
int64_t new_idx = index_old_to_new[indices_ptr[i]];
indices_ptr[i] = Tindex_t(new_idx);
}
//Update triangle indices, no need to update other attributes.
tris = tris_cpu.To(mesh.GetDevice());
mesh.SetTriangleIndices(tris);
}

utility::LogDebug(
"[RemoveDuplicatedVertices] {:d} vertices have been removed.",
(int)(orig_num_vertices - k));

return mesh;

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this empty line is not needed.

}

} //Anonymous namespace

///
/// Remove duplicate vertices from a mesh and return the resulting mesh.
///
TriangleMesh& TriangleMesh::RemoveDuplicateVertices()
{
/// This function mainly does some checks and then calls the RemoveDuplicateVerticesWorker
/// with appropriate data type.
if(!HasVertexPositions()) {
utility::LogWarning("TriangeMesh::RemoveDuplicateVertices: No vertices present, ignoring.");
return *this;
}
auto vertices = GetVertexPositions();
auto triangles = GetTriangleIndices();
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be meshes w/o triangles tensor. In this case, the call to GetTrianglesIndices throws.
Could you please add safety checks for this case?

if (core::Int32 != triangles.GetDtype() && core::Int64 != triangles.GetDtype()) {
utility::LogError("Only Int32 or Int64 are supported for triangle indices");
return *this;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to return after utility::LogError. (here and below)

}
if (core::Float32 != vertices.GetDtype() && core::Float64 != vertices.GetDtype()) {
utility::LogError("Only Float32 or Float64 is supported for vertex coordinates");
return *this;
}

if(core::Float32 == vertices.GetDtype()) {
if (core::Int32 == triangles.GetDtype()) {
return RemoveDuplicateVerticesWorker<float, int>(*this);
} else {
return RemoveDuplicateVerticesWorker<float, int64_t>(*this);
}
} else {
if (core::Int32 == triangles.GetDtype()) {
return RemoveDuplicateVerticesWorker<double, int>(*this);
} else {
return RemoveDuplicateVerticesWorker<double, int64_t>(*this);
}
}
Comment on lines +1335 to +1347
Copy link
Contributor

Choose a reason for hiding this comment

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

While working on my SelectByIndex method, I discovered a set of macros to do this in more compact way:
https://github.com/isl-org/Open3D/blob/main/cpp/open3d/core/Dispatch.h#L90
I think we could rewrite it as smt like:

Suggested change
if(core::Float32 == vertices.GetDtype()) {
if (core::Int32 == triangles.GetDtype()) {
return RemoveDuplicateVerticesWorker<float, int>(*this);
} else {
return RemoveDuplicateVerticesWorker<float, int64_t>(*this);
}
} else {
if (core::Int32 == triangles.GetDtype()) {
return RemoveDuplicateVerticesWorker<double, int>(*this);
} else {
return RemoveDuplicateVerticesWorker<double, int64_t>(*this);
}
}
DISPATCH_FLOAT_INT_DTYPE_TO_TEMPLATE(vertices.GetDtype(), triangles.GetDtype()), [&]() {
RemoveDuplicateVerticesWorker<scalar_t, int_t>(*this);
});

}


} // namespace geometry
} // namespace t
} // namespace open3d
6 changes: 6 additions & 0 deletions cpp/open3d/t/geometry/TriangleMesh.h
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,12 @@ class TriangleMesh : public Geometry, public DrawableGeometry {
/// empty, return an empty mesh.
TriangleMesh SelectFacesByMask(const core::Tensor &mask) const;


/// Removes duplicate vertices and their associated attributes.
/// It also updates the triangle indices to refer to the reduced array of vertices.
/// \return Mesh with the duplicate vertices removed.
TriangleMesh& RemoveDuplicateVertices();

/// Returns a new mesh with the vertices selected by a vector of indices.
/// If an item from the indices list exceeds the max vertex number of
/// the mesh or has a negative value, it is ignored.
Expand Down
4 changes: 4 additions & 0 deletions cpp/pybind/t/geometry/trianglemesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,10 @@ the partition id for each face.
print(np.unique(mesh.triangle.partition_ids.numpy(), return_counts=True))

)");
triangle_mesh.def(
"remove_duplicate_vertices", &TriangleMesh::RemoveDuplicateVertices,
"Removes duplicate vertices from vertex attribute"
"'positions' and updates other vertex attributes and triangle indices.");

triangle_mesh.def(
"select_faces_by_mask", &TriangleMesh::SelectFacesByMask, "mask"_a,
Expand Down
51 changes: 51 additions & 0 deletions cpp/tests/t/geometry/TriangleMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,57 @@ TEST_P(TriangleMeshPermuteDevices, Transform) {
core::Tensor::Init<float>({{2, 2, 1}, {2, 2, 1}}, device)));
}

TEST_P(TriangleMeshPermuteDevices, RemoveDuplicateVertices) {
core::Device device = GetParam();
t::geometry::TriangleMesh mesh(device);
mesh.SetVertexPositions(
core::Tensor::Init<float>({{0.0, 0.0, 0.0},
{1.0, 0.0, 0.0},
{0.0, 0.0, 1.0},
{1.0, 0.0, 1.0},
{0.0, 1.0, 0.0},
{1.0, 1.0, 0.0},
{0.0, 1.0, 1.0},
{1.0, 0.0, 0.0},
{1.0, 1.0, 1.0}}, device));
mesh.SetTriangleIndices(
core::Tensor::Init<int>({{4, 8, 5},
{4, 6, 8},
{0, 2, 4},
{2, 6, 4},
{0, 1, 2},
{1, 3, 2},
{7, 5, 8},
{7, 8, 3},
{2, 3, 8},
{2, 8, 6},
{0, 4, 1},
{1, 4, 5}}, device));
mesh.RemoveDuplicateVertices();
EXPECT_TRUE(mesh.GetVertexPositions().AllClose(
core::Tensor::Init<float>({{0.0, 0.0, 0.0},
{1.0, 0.0, 0.0},
{0.0, 0.0, 1.0},
{1.0, 0.0, 1.0},
{0.0, 1.0, 0.0},
{1.0, 1.0, 0.0},
{0.0, 1.0, 1.0},
{1.0, 1.0, 1.0}})));
EXPECT_TRUE(mesh.GetTriangleIndices().AllClose(
core::Tensor::Init<int>({{4, 7, 5},
{4, 6, 7},
{0, 2, 4},
{2, 6, 4},
{0, 1, 2},
{1, 3, 2},
{1, 5, 7},
{1, 7, 3},
{2, 3, 7},
{2, 7, 6},
{0, 4, 1},
{1, 4, 5}})));
}

TEST_P(TriangleMeshPermuteDevices, Translate) {
core::Device device = GetParam();

Expand Down