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 (continued) #6636

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

shashigk
Copy link

@shashigk shashigk commented Feb 5, 2024

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

intelshashi and others added 9 commits October 5, 2023 22:57
Functionality to remove duplicate vertices and update
other vertex attributes and triangle indices is implemented.
The C++ and python tests are also written to test the code.

On branch sganjugu/remdup2
Changes to be committed:
modified:   cpp/open3d/t/geometry/TriangleMesh.cpp
modified:   cpp/open3d/t/geometry/TriangleMesh.h
modified:   cpp/pybind/t/geometry/trianglemesh.cpp
modified:   cpp/tests/t/geometry/TriangleMesh.cpp
modified:   python/test/t/geometry/test_trianglemesh.py
Previously, I was doing a manual copy and shrinking the vertices.
Instead in this checkin all vertex attributes, including the
coordinates are shrunk using IndexGet method using a vertex mask.

Further, the triangle index remapping is similar to what was done
earlier, with some simplications. One thing to note, is that
we cannot use utility::InclusivePrefixSum to remap vertex indices
because the duplicates can occur in any position and may be duplicates
of any earlier vertex.

For e.g., suppose there were 9 vertices to start with, and 8th (index 7, starting from 0),
was a duplicate of 2nd (index 1, starting from 0).

So, the vertex mask would look like this:
Vertex indices: [0, 1, 2, 3, 4, 5, 6, 7, 8]
Vertex mask:    [1, 1, 1, 1, 1, 1, 1, 0, 1]
Prefix sum:   [0, 1, 2, 3, 4, 5, 6, 7, 7, 8]
This gives an incorrect index map for 8th vertex, which is mapped to
index 7 instead of 1.

On branch sganjugu/remdup2
Your branch is up to date with 'origin/sganjugu/remdup2'.
Changes to be committed:
modified:   ../cpp/open3d/t/geometry/TriangleMesh.cpp
modified:   ../python/test/t/geometry/test_trianglemesh.py
- Made vertex mask type bool.
- Check if mesh has triangle indices before getting them.
- Modified some errors to warnings.

On branch sganjugu/remdup2
Your branch is up to date with 'origin/sganjugu/remdup2'.
Changes to be committed:
modified:   cpp/open3d/t/geometry/TriangleMesh.cpp
Instead of using if statements explicitly, used DISPATCH_FLOAT_INT_DTYPE_TO_TEMPLATE
to dispatch to RemoveDuplicateVerticesWorker function appropriately.

On branch sganjugu/remdup2
Your branch is up to date with 'origin/sganjugu/remdup2'.
Changes to be committed:
modified:   ../cpp/open3d/t/geometry/TriangleMesh.cpp
@shashigk
Copy link
Author

shashigk commented Feb 6, 2024

This is continuation of the PR:
#6414

@shashigk shashigk marked this pull request as ready for review February 6, 2024 10:01
@@ -30,6 +30,7 @@
- Fix geometry picker Error when LineSet objects are presented (PR #6499)
- Fix mis-configured application .desktop link for the Open3D viewer when installing to a custom path (PR #6599)
- Fix regression in printing cuda tensor from PR #6444 🐛
- Implemented functionality for removing duplicate vertices from TriangleMesh (PR #6414).
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 align this line with the rest of the list (e.g. add two more spaces after -)?

Comment on lines +1262 to +1268
/// (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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// (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
/// * point_to_old_index maps vertex coordinates to their indices.
// This is implemented using unordered_map.
/// * index_old_to_new maps vertex indices before duplicate removal to indices after
/// the removal. This is implemented using std::vector.
/// The first map allows the function to detect duplicates and the second allows it to update

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

auto vertices = mesh.GetVertexPositions().To(mesh.GetDevice()).Contiguous();
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, the vertex tensor is already on the device of the mesh, but we want to move the tensor to CPU, so I think we need to use core::Device() here.

using vmask_itype = bool;
core::Tensor vertex_mask =
core::Tensor::Zeros({orig_num_vertices}, vmask_type);
using Length_t = decltype(orig_num_vertices);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just use int64_t here? It looks like the GetLength type is always int64_t.

Comment on lines +1295 to +1296
const auto vmask_type = core::Bool;
using vmask_itype = bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a personal taste, but I find the original types, bool and core::Bool, more descriptive in this case.

point_to_old_index[coord] = i;
index_old_to_new[i] = k;
++k;
vertex_mask_ptr[i] = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 1 -> true, since it is a bool value here. (it is 1 in other functions above as we use int there for the prefix_sum approach).

def gen_box_mesh_dup_vertex(unique_v, unique_i, v_index, insert_pos):
"""
Generates a duplicate vertex list and an updated index list from
the uniques afte inserting a vertex at v_index in unique_v to position
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: afte - > after

Comment on lines +635 to +636
unique_i = [[4, 7, 5], [4, 6, 7], [0, 2, 4], [2, 6, 4], [0, 1,
2], [1, 3, 2],
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the line break here look as intended?

@ssheorey ssheorey added this to the v0.19 milestone Jul 16, 2024
@ssheorey ssheorey changed the title Sganjugu/remdup2 RemoveDuplicateVertices Implementation for TriangleMesh (continued) Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants