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

[Mesh] TriangleMesh's "+=" operator appends UVs regardless of the presence of existing features #6728

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

Conversation

cdbharath
Copy link

@cdbharath cdbharath commented Mar 27, 2024

Allows appending of UV coordinates, textures and material IDs with += operator overload in TriangleMesh class irrespective of the presence of these attributes in the existing mesh (mesh on LHS to += operator)

Type

Motivation and Context

The += operator overload of TriangleMesh class does not append UV coordinates, textures and material IDs when the existing mesh (mesh on LHS to += operator) already contains any of these attributes. There is no reason to limit += for UVs to TriangleMesh with the attributes already assigned as it currently happens here.

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

The current change doesn't check if the presence of UV coordinates, textures and material IDs in the existing mesh and appends only based on their availability in the incoming mesh.

I am attaching a screenshot that shows the addition of UV coordinates based on += operator. The test code is as follows

    auto texture_image = io::CreateImageFromFile("/home/bharath/Downloads/uv1.png");

    auto box = geometry::TriangleMesh::CreateBox(1.0, 1.0, 1.0, true, true);    
    box->textures_.push_back(*texture_image);
    box->triangle_material_ids_.resize(box->triangles_.size(), 0);

    auto sphere = geometry::TriangleMesh::CreateSphere(1.0, 6, true);   
    sphere->textures_.push_back(*texture_image);
    sphere->triangle_material_ids_.resize(sphere->triangles_.size(), 0);

    auto scene = std::make_shared<geometry::TriangleMesh>();
        
    // Append new meshes using +=
    *scene += *box;
    *scene += *sphere;
            
    open3d::visualization::DrawGeometries({scene}, "Scene");

Case 1: Both box and sphere has UVs, textures and material IDs
Note: First 2 images are inputs and the 3rd image is the output
Alt TextAlt TextAlt Text

Case 2: Box has UVs, textures and material IDs. But sphere has only UVs
Note: First 2 images are inputs and the 3rd image is the output
Alt TextAlt TextAlt Text

Case 3: Sphere has UVs, textures and material IDs. But box has only textures and material IDs
Note: First 2 images are inputs and the 3rd image is the output
Alt TextAlt TextAlt Text

Copy link

update-docs bot commented Mar 27, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@cdbharath cdbharath changed the title [Mesh] TriangleMesh's "+=" operator applies regardless of the presenc… [Mesh] TriangleMesh's "+=" operator applies regardless of the presence of existing features Mar 27, 2024
@cdbharath cdbharath changed the title [Mesh] TriangleMesh's "+=" operator applies regardless of the presence of existing features [Mesh] TriangleMesh's "+=" operator appends UVs regardless of the presence of existing features Mar 28, 2024
Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

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

@cdbharath Thanks for your PR, please have a look at the comment

@@ -77,30 +74,29 @@ TriangleMesh &TriangleMesh::operator+=(const TriangleMesh &mesh) {
if (HasAdjacencyList()) {
ComputeAdjacencyList();
}
if (add_textures) {
if (mesh.HasTriangleUvs()) {
size_t old_tri_uv_num = triangle_uvs_.size();
triangle_uvs_.resize(old_tri_uv_num + mesh.triangle_uvs_.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

If this does not have any UV coordinates then triangle_uvs_.size() will be different from the total number of triangles in the resulting mesh. This would break with the assumption that triangle_uvs_.size() == 3*number of triangles

@benjaminum
Copy link
Contributor

Hi @cdbharath , we think mixing meshes with and without texture uvs can be problematic. I added a special case for empty meshes that allows your workflow with the += operator.

} else {
triangle_uvs_.clear();
textures_.clear();
triangle_material_ids_.clear();
}
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep this portion - clear() properties if only one mesh has them (unless the other mesh is empty), else we will get meshes with only half the triangles with material ids, etc. (e.g. if this has material_ids, but mesh doesn't, output should not have material ids.) Half populated properties will cause seg faults.

Also, textures and triangle_material_ids go together, so we should have a single if() for them. We want n_textures == max(triangle_material_ids_)+1 and either both or none should be present always.

@cdbharath this PR may break other mesh functions if there is a bug -- please do the following tests and make sure there are no errors. Check for the case where:

  • a has texture_uvs but no textures / triangle_material_ids
  • b has all of texture_uvs, textures and triangle_material_ids
  • c has none of the 3 (but has triangles)

With these 3 meshes, perform this check:

  • a+=b and b+=a should produce the same results in a and b respectively (test mesh equality). Also for the pair a+=c, c+=a, and separately for the pair b+=c and c+=b. [Add this to C++ unit tests if not present already] Visualize the resulting mesh correctly with draw_geometries() and draw() [Just test visualization on your computer - no unit tests required]

@ssheorey ssheorey added the status / needs info Waiting for information from reporter / author label Jun 15, 2024
@cdbharath cdbharath requested a review from ssheorey June 26, 2024 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status / needs info Waiting for information from reporter / author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants