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

Fix for Issue #5924: #5938

Merged
merged 4 commits into from
Oct 14, 2023
Merged

Fix for Issue #5924: #5938

merged 4 commits into from
Oct 14, 2023

Conversation

dbs4261
Copy link
Contributor

@dbs4261 dbs4261 commented Feb 20, 2023

Fix for Issue #5924:

Changed TriangleMesh::materials_ to be a std::vector<std::pair<std::string, Material>> so that the order of materials when loading a mesh is respected. Using the unordered map caused issues when a Default Material and a texture were loaded and the texture would end up as the second material in the iterator. Now TriangleMesh::triangle_material_ids_ will indicate what material index should be used. A warning is given when passing a TriangleMesh to the visualizer and more than one material index is found, and the minimum value found in TriangleMesh::triangle_material_ids_ is used. If no triangle_material_ids_ exist then the first material in the order they are loaded will be used.

Type

Motivation and Context

When loading a textured mesh with Assimp or TinyOBJ the materials are loaded in order, and a material index is provided for each triangle. However these materials are stored in an unordered map. A common occurrence is loaded a textured mesh along with a default material. The texture might be loaded as index 1 which can be seen in the TriangleMesh::triangle_material_ids_. By instead loading to a vector, the order is maintained.

This change breaks any access to the TriangleMesh::materials_ parameter. Though no instances of this access occur within the Open3d C++ library, it is exposed publicly in the Python API. Instead of a dictionary, the user would get a list of pairs.

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


This change is Reviewable

@update-docs
Copy link

update-docs bot commented Feb 20, 2023

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

@ssheorey
Copy link
Member

Hi @dbs4261 please check the CI failures.

@dbs4261
Copy link
Contributor Author

dbs4261 commented Apr 13, 2023

I made some updates to address the merge issues. Lets try the workflows again.

Changed TriangleMesh::materials_ to be a std::vector<std::pair<std::string, Material>> so that the order of materials when loading a mesh is respected.
Using the unordered map caused issues when a Default Material and a texture were loaded and the texture would end up as the second material in the iterator.
Now TriangleMesh::triangle_material_ids_ will indicate what material index should be used.
A warning is given when passing a TriangleMesh to the visualizer and more than one material index is found, and the minimum value found in TriangleMesh::triangle_material_ids_ is used.
If no triangle_material_ids_ exist then the first material in the order they are loaded will be used.
Copy link
Collaborator

@errissa errissa left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dbs4261)

@errissa errissa merged commit 3ddc69b into isl-org:master Oct 14, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For TriangleMesh triangle_material_ids_ do not relate to the materials_
3 participants