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

Merged
merged 10 commits into from
Sep 2, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
- Fix KDTreeFlann possibly using a dangling pointer instead of internal storage and simplified its members (PR #6734)
- Fix RANSAC early stop if no inliers in a specific iteration (PR #6789)
- Fix segmentation fault (infinite recursion) of DetectPlanarPatches if multiple points have same coordinates (PR #6794)
- `TriangleMesh`'s `+=` operator appends UVs regardless of the presence of existing features (PR #6728)
- Fix build with fmt v10.2.0 (#6783)
- Fix segmentation fault (lambda reference capture) of VisualizerWithCustomAnimation::Play (PR #6804)
- Add O3DVisualizer API to enable collapse control of verts in the side panel (PR #6865)
Expand Down
18 changes: 11 additions & 7 deletions cpp/open3d/geometry/TriangleMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,12 @@ TriangleMesh &TriangleMesh::Rotate(const Eigen::Matrix3d &R,

TriangleMesh &TriangleMesh::operator+=(const TriangleMesh &mesh) {
if (mesh.IsEmpty()) return (*this);
bool add_textures = HasTriangleUvs() && HasTextures() &&
HasTriangleMaterialIds() && mesh.HasTriangleUvs() &&
mesh.HasTextures() && mesh.HasTriangleMaterialIds();
bool is_empty = IsEmpty();
bool add_triangle_uvs =
mesh.HasTriangleUvs() && (HasTriangleUvs() || is_empty);
bool add_textures_and_material_ids =
mesh.HasTextures() && mesh.HasTriangleMaterialIds() &&
((HasTextures() && HasTriangleMaterialIds()) || is_empty);
size_t old_vert_num = vertices_.size();
MeshBase::operator+=(mesh);
size_t old_tri_num = triangles_.size();
Expand All @@ -77,19 +80,21 @@ TriangleMesh &TriangleMesh::operator+=(const TriangleMesh &mesh) {
if (HasAdjacencyList()) {
ComputeAdjacencyList();
}
if (add_textures) {
if (add_triangle_uvs) {
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

for (size_t i = 0; i < mesh.triangle_uvs_.size(); i++) {
triangle_uvs_[old_tri_uv_num + i] = mesh.triangle_uvs_[i];
}

} else {
triangle_uvs_.clear();
}
if (add_textures_and_material_ids) {
size_t old_tex_num = textures_.size();
textures_.resize(old_tex_num + mesh.textures_.size());
for (size_t i = 0; i < mesh.textures_.size(); i++) {
textures_[old_tex_num + i] = mesh.textures_[i];
}

size_t old_mat_id_num = triangle_material_ids_.size();
triangle_material_ids_.resize(old_mat_id_num +
mesh.triangle_material_ids_.size());
Expand All @@ -98,7 +103,6 @@ TriangleMesh &TriangleMesh::operator+=(const TriangleMesh &mesh) {
mesh.triangle_material_ids_[i] + (int)old_tex_num;
}
} 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]

Expand Down
123 changes: 121 additions & 2 deletions cpp/tests/geometry/TriangleMesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ namespace tests {
/// \param eq_triangle_vertex_order If true then triangles are only equal if the
/// order of the vertices is the same. If false any permutation of the triangle
/// indices is allowed.
void ExpectMeshEQ(const open3d::geometry::TriangleMesh& mesh0,
const open3d::geometry::TriangleMesh& mesh1,
void ExpectMeshEQ(const open3d::geometry::TriangleMesh &mesh0,
const open3d::geometry::TriangleMesh &mesh1,
double threshold = 1e-6,
bool eq_triangle_vertex_order = true) {
ExpectEQ(mesh0.vertices_, mesh1.vertices_, threshold);
Expand Down Expand Up @@ -599,6 +599,125 @@ TEST(TriangleMesh, OperatorADD) {
}
}

TEST(TriangleMesh, OperatorAdditionAssignment) {
const size_t size = 100;
const size_t texture_size = 10;

// Define the minimum and maximum bounds for random data generation
Eigen::Vector3d dmin(0.0, 0.0, 0.0);
Eigen::Vector3d dmax(1000.0, 1000.0, 1000.0);
Eigen::Vector3i imin(0, 0, 0);
Eigen::Vector3i imax(size - 1, size - 1, size - 1);
Eigen::Vector2d uvmin(0.0, 0.0);
Eigen::Vector2d uvmax(1.0, 1.0);

// Mesh 0 contains only UVs but no textures and material ids
geometry::TriangleMesh tm0;
tm0.vertices_.resize(size);
tm0.triangles_.resize(size);
tm0.triangle_uvs_.resize(3 * size);

// Mesh 1 contains all of UVS, textures and material ids
geometry::TriangleMesh tm1;
tm1.vertices_.resize(size);
tm1.triangles_.resize(size);
tm1.triangle_uvs_.resize(3 * size);
tm1.triangle_material_ids_.resize(size);
tm1.textures_.resize(texture_size);

// Mesh 2 does not contains any of UVs, textures or material ids
geometry::TriangleMesh tm2;
tm2.vertices_.resize(size);
tm2.triangles_.resize(size);

// Randomly generate data for tm0, tm1 and tm2
Rand(tm0.vertices_, dmin, dmax, 0);
Rand(tm0.triangles_, imin, imax, 0);
std::vector<Eigen::Vector2d, open3d::utility::Vector2d_allocator>
tm0_triangle_uvs(3 * size);
Rand(tm0_triangle_uvs, uvmin, uvmax, 0);
tm0.triangle_uvs_ = std::vector<Eigen::Vector2d>(tm0_triangle_uvs.begin(),
tm0_triangle_uvs.end());

Rand(tm1.vertices_, dmin, dmax, 0);
Rand(tm1.triangles_, imin, imax, 0);
std::vector<Eigen::Vector2d, open3d::utility::Vector2d_allocator>
tm1_triangle_uvs(3 * size);
Rand(tm1_triangle_uvs, uvmin, uvmax, 0);
tm1.triangle_uvs_ = std::vector<Eigen::Vector2d>(tm1_triangle_uvs.begin(),
tm1_triangle_uvs.end());
for (size_t i = 0; i < texture_size; i++) {
geometry::Image texture_image;
texture_image.Prepare(100, 100, 3, 1);
Rand(texture_image.data_, 0, 255, 0);
tm1.textures_[i] = texture_image;
}
Rand(tm1.triangle_material_ids_, 0, texture_size, 0);

Rand(tm2.vertices_, dmin, dmax, 0);
Rand(tm2.triangles_, imin, imax, 0);

geometry::TriangleMesh temp_tm0 = tm0;
geometry::TriangleMesh temp_tm1 = tm1;
geometry::TriangleMesh temp_tm2 = tm2;
// Function to reset values of temp_tm0, temp_tm1 and temp_tm2
auto reset_values = [&tm0, &tm1, &tm2](geometry::TriangleMesh &temp_tm0,
geometry::TriangleMesh &temp_tm1,
geometry::TriangleMesh &temp_tm2) {
temp_tm0 = tm0;
temp_tm1 = tm1;
temp_tm2 = tm2;
};

// Test addition of tm1 with itself
temp_tm1 += tm1;
EXPECT_EQ(temp_tm1.triangles_.size(), 2 * size);
EXPECT_EQ(temp_tm1.triangle_uvs_.size(), 6 * size);
EXPECT_EQ(temp_tm1.vertices_.size(), 2 * size);
EXPECT_EQ(temp_tm1.textures_.size(), 2 * texture_size);
EXPECT_EQ(temp_tm1.triangle_material_ids_.size(), 2 * size);
for (size_t i = 0; i < size; i++) {
ExpectEQ(temp_tm1.vertices_[i], temp_tm1.vertices_[i + size]);
ExpectEQ(temp_tm1.triangles_[i],
(Eigen::Vector3i)(temp_tm1.triangles_[i + size] -
Eigen::Vector3i(size, size, size)));
EXPECT_EQ(temp_tm1.triangle_material_ids_[i],
temp_tm1.triangle_material_ids_[i + size] - texture_size);
}
for (size_t i = 0; i < 3 * size; i++) {
ExpectEQ(temp_tm1.triangle_uvs_[i],
temp_tm1.triangle_uvs_[i + 3 * size]);
}
for (size_t i = 0; i < texture_size; i++) {
ExpectEQ(temp_tm1.textures_[i].data_,
temp_tm1.textures_[i + texture_size].data_);
}

// Test addition of tm0 and tm1
reset_values(temp_tm0, temp_tm1, temp_tm2);
temp_tm0 += tm1;
temp_tm1 += tm0;
ExpectMeshEQ(temp_tm0, temp_tm1);
EXPECT_EQ(temp_tm0.textures_.size(), 0);
EXPECT_EQ(temp_tm0.triangle_material_ids_.size(), 0);

// Test addition of tm1 and tm2
reset_values(temp_tm0, temp_tm1, temp_tm2);
temp_tm1 += tm2;
temp_tm2 += tm1;
ExpectMeshEQ(temp_tm1, temp_tm2);
EXPECT_EQ(temp_tm0.textures_.size(), 0);
EXPECT_EQ(temp_tm0.triangle_material_ids_.size(), 0);

// Test addition of tm2 and tm0
reset_values(temp_tm0, temp_tm1, temp_tm2);
temp_tm2 += tm0;
temp_tm0 += tm2;
ExpectMeshEQ(temp_tm2, temp_tm0);
EXPECT_EQ(temp_tm0.textures_.size(), 0);
EXPECT_EQ(temp_tm0.triangle_material_ids_.size(), 0);
}

TEST(TriangleMesh, ComputeTriangleNormals) {
std::vector<Eigen::Vector3d> ref = {{-0.119231, 0.738792, 0.663303},
{-0.115181, 0.730934, 0.672658},
Expand Down
Loading