Handle materials referenced by name specified outside the link object

[seadeep42]

No description provided.

[gkjohnson]

Hey @Chaitanya-Deep, thanks for contributing!

This looks like a great change. Do you have an publicly available URDF files I can use to test it?

Thanks!

[gkjohnson]

Maybe a nit pick but can we change the materials argument here to materialMap so it's consistent?

[gkjohnson]

I see that the _processMaterial function takes an existing material and modifies it so each mesh has its own material instance. Considering these are links that share material nodes, does it make sense to preprocess material nodes into THREE.Material objects up front and share them across many THREE.Mesh instances? That way fewer material objects will be created. This also means that the meshes will share any material updates.

I'm also noticing we will always load a new texture when processing a material, even if that texture has been used elsewhere, which is maybe the bigger issue, memory-wise. I'll create a separate issue for that, though.

Edit: Created the texture duplication issue here #73

[seadeep42]

Hey,
The test_robot.urdf from ros/urdf repo can be used to test it.
https://github.com/ros/urdf/tree/melodic-devel/urdf/test

I'm not so sure about sharing materials. For the scenario that I'm using the library for, sharing a material object will be unexpected. If the library is used solely for viewing a urdf, that might be necessary. In my use case when a user loads a robot with two parts having the same color, he would want to select a part and change it's color rather than change the color value for the name the color corresponds to.

[gkjohnson]

Using Material.copy concerns me a little bit because it does a lot more than just copy the color and texture and it doesn't work well when using different material types. new THREE.MeshStandardMaterial().copy(new THREE.MeshLambertMaterial()) throws an error, for example, so it's not always safe to call copy on a material of a different type. And if someone has created a custom material as a part of the mesh load function then things like the specular values will be overwritten. It would overwrite the color even if it wasn't specified in the URDF, too.

How do you feel about copying just those specific values:

const sharedMatData = materialMap[materialName];
if (sharedMatData.color) material.color.copy(sharedMatData.color);
if (sharedMatData.map) material.map = sharedMatData.map;

And then the material map could store just those preprocessed values in an object. The color and texture node parsing could be pulled out into separate functions that return the texture and color objects.

[gkjohnson]

I'm not so sure about sharing materials ... In my use case when a user loads a robot with two parts having the same color, he would want to select a part and change it's color rather than change the color value for the name the color corresponds to.

Sounds fine for that use case -- I don't think I have strong opinions about this. We may want to consider sharing materials in the future if there's a need, though.

And thanks for handling the texture duplication in the shared materials case!

[seadeep42]

Made the change.
I took for granted that the materials would be phong since previously the STL loader was loading a mesh with basic material which was being copied to by phong material in _processVisualNode while the viewer was replacing it in updateMaterials

[gkjohnson]

Using || like this means that once the first object is created the return this._processMaterial function will never get called and the parameters defined by the second node will never get used. I'd suggest passing an object in to the _processMaterial function. Maybe something like:

const name = m.getAttribute('name');
materialMap[name] = {};
this.forEach(m.children, c => {
    this._processMaterial(materialMap[name], c, packages, path)
});

That way _processMaterial can modify the existing object and add any fields it needs to.

[gkjohnson]

Using Object.assign here won't work well for fields like color because then every instance of the materials used here will have the same instance of the THREE.Color object. Using material.color.copy(materialMap[materialName].color) seems like the better way to handle the color assignment.

It may be tedious but it might be necessary to handle every field assignment individually.

[gkjohnson]

This should be an instance of THREE.Color if we're planning to do a copy or assignment of the object to a material. Another option would be to keep it like this but use the THREE.Color.setRGB function when updating the material.

Thanks for bearing with me on the changes!

[gkjohnson]

Hey @Chaitanya-Deep,

I wanted to get these changes merged in before I started reorganizing some files in the repo -- would you like me to help wrap up some remaining changes and get this merged in?

Thanks!

[seadeep42]

Hi,
Sorry for the delay. Was busy with something else.
Made the changes to THREE.Color and cloning the color instead of assign.

I'm slightly confused regarding the || in materialMap[name]. That is actually intended. materialMap is supposed to keep track of the material corresponding to a name in the urdf file. While processing the visual nodes with type as material, materialMap is only used if either the node has no children or the node has a texture.

The assumption here is that two texture nodes with the same name will have the same texture hence the one in materialMap can be used rather than loading the texture again.
Am I missing something?

[gkjohnson]

Great! Looks like you've covered everything. I'll give it more of a test in a bit and try to get it merged today.

I'm slightly confused regarding the || in materialMap[name]. That is actually intended. materialMap is supposed to keep track of the material corresponding to a name in the urdf file. While processing the visual nodes with type as material, materialMap is only used if either the node has no children or the node has a texture.

The problem with this logic in the _processRobot function:

const name = m.getAttribute('name');
this.forEach(m.children, c => {
    materialMap[name] = materialMap[name] || this._processMaterial(c, packages, path);
});

Is that it will not include processing any of the information past the first material information node. Consider:

<material name="blue">
    <color rgba="0 0 .8 1"/>
    <texture filename=".../texture.png"/>
</material>

The color node will be processed in the loop first and set to materialMap[name] because it isn't defined yet. When the texture node is processed materialMap[name] will be already defined so _processMaterial will not get called and the texture data will not be added onto the material object. Hopefully that's more clear.