Merge primitive materials

[ibozhilov]

Initial implementation of a merge of the primitive's materials when loading gltf with merge_primitive=True

[mikedh]

Thanks for the PR! Could you add an example model that does this for unit tests?

My one thought is that do we need a new visual type? What if we just used TextureVisuals and made visual.material optionally a list of materials rather than always a single material?

[ibozhilov]

Yeah, I thought about that too. I opted for creating a new visual type, because otherwise we have to change a bit the TextureVisuals. I'm not entirely sure if it's a good idea to change the visual.material to be sometimes a single material and sometimes a list of materials. That can be confusing and it's inconsistent. I believe it will be better if it is always a list of materials, but optionally it may contain only 1 material in that list. My only issue with that approach is that we will change the TextureVisuals API and potentially break other projects that use the Trimesh library. What do you think?

P.S. In the mean time I will add a simple cube with multiple primitives as a test material.

[mikedh]

Thanks for adding the model! Yeah seems like the options are:

1. Replace TextureVisuals with FaceMaterialsVisuals (as implemented here)
2. Make Texturevisuals.material always a list of materials and add a TextureMaterials.materials_face property which defaults to None but can be set, and responds to .update_faces
3. Same as (2) but TextureVisuals.material is sometimes a list, sometimes a Material object

4. Same as (2) but we add a new MultiMaterial material which is a thin container for a list.

I'd personally kind of lean towards 4 though am definitely open to others, what do you think?

[ibozhilov]

Hi, so I believe that 1 will cause a lot of conflicts as it will change the visual API. 2 is essentially the same as 1 or am I missing something? 3 won't break the API, but as I mention earlier I find it confusing. I'm not entirely sure I understand 4. The idea is to keep the TextureVisuals, add the materials_face field and set the material field to be sometimes PBRMaterial and sometimes MultiMaterial, which is essentially a list of materials, but wrapped in a new class. Am I understanding it correctly?

[mikedh]

Apologies for the delay. Looks like there's a linked issue for people asking for this too.

Yeah I would strongly prefer a "multi-material" material rather than a separate visual object if possible. If you have cycles to do that here it would be awesome otherwise I can hopefully take up the torch on this one 😄. I think the unit test this needs regardless of implementation is:

• Import models with multiple materials from OBJ exports correctly back to OBJ and GLTF/B

[ibozhilov]

I hear you. Two more things though. First what do you think should the materials_face be in the visual object or in the material object. I believe that probably in should be in the visual object as it is related to the mesh, however this means that we will change the API of the TextureVisuals. And if so then we sometimes have materials_face and some times we don't, which I don't really like. That is why I see it as we have two options to proceed:

1. Add a new field to the TextureVisuals called materials_face and have it set all times, but simply if there is only one materials it will be a list of 0 with length same as the one of the mesh.faces
2. Make the MultiMaterial a bit more complicated with adding materials_face to it.

The second option may be a bit confusing as the materials_face is related to the mesh, but I believe that it is the cleaner way to go. What do you think?

As on the test. I totally agree, but the aim should be the export to create a valid gltf, not the same one. So if we have a mesh with multiple primitives in the same color after import/export it should be single mesh and if we have a mesh with multiple primitives each in different color after import/export it should be the same. Are you okay with that?

While working on that I spot a bug with the naming of the primitives, but I will open a separate PR for that.

[mikedh]

Awesome sounds good!

I hear you. Two more things though. First what do you think should the materials_face be in the visual object or in the material object. I believe that probably in should be in the visual object as it is related to the mesh, however this means that we will change the API of the TextureVisuals. And if so then we sometimes have materials_face and some times we don't, which I don't really like. That is why I see it as we have two options to proceed:
Add a new field to the TextureVisuals called materials_face and have it set all times, but simply if there is only one materials it will be a list of 0 with length same as the one of the mesh.faces
Make the MultiMaterial a bit more complicated with adding materials_face to it.
The second option may be a bit confusing as the materials_face is related to the mesh, but I believe that it is the cleaner way to go. What do you think?

My inclination would be to have visual.material_face as a property which is either None or an (n,) int array, where None indicates every material is visual.material. Lots of properties set themselves to None, and then you can check with a relatively conventional if mesh.visual.material_face is not None

As on the test. I totally agree, but the aim should be the export to create a valid gltf, not the same one. So if we have a mesh with multiple primitives in the same color after import/export it should be single mesh and if we have a mesh with multiple primitives each in different color after import/export it should be the same. Are you okay with that?

Agreed! Producing the same input is definitely not necessary as long as it passes the validators (run in unit tests).

While working on that I spot a bug with the naming of the primitives, but I will open a separate PR for that.

Sounds good! Yeah the primitive:mesh mapping is a little flaky.

[mikedh]

Thanks for the changes! Let me know when you're ready to merge and I'll make a pass before releasing in #1271

[ibozhilov]

I believe that I have fixed things for the time being. One think to mention is that the test file I use was created with Blender having red, blue and green materials. Once exported the baseColorFactor of the materials was something like [1, 0, 0, 1]. However, when loaded with trimesh the values were converted to [0.00392156863, 0, 0, 0.00392156863]. I took a look that you decide on whether to divide the values by checking whether the type is float or integer, which doesn't work in that case. I edited the text file to have values like [1.0, 0.0, 0.0, 1.0], as I don't have a better solution right now. Though it's worth mentioning.

Another thing that is unresolved issue is the naming of the primitives. Let's say we have two nodes named cube each with two primitives. When loading you check if the mesh name is in the list, so that you don't have conflicts. However, once we have primitives you don't have the mesh name in the list with meshes, but you have it with index for each primitive. So you check that cube does not exist, but on the second node you load the first primitive with name cube_0, which effectively overwrites the primitive of the first node. I thought about adding a check for each primitive, but that made the loading a lot slower. Another option is to always add unique string to the name of each node. However, this way we always loose the names of the nodes and probably is not the best decision. Would love to hear your input on that. Probably also we can close this PR and I can open a new one for that?

[mikedh]

Looks great thanks for the updates! Yeah the primitive:mesh mapping is a bit rough.. I also don't know what the best way to handle that is. Maybe a key we can check in metadata? Or possibly add metadata to the nodes in the scene graph? I'll merge this and make a pass and see if anything jumps out. Thanks for all the work on this!