fix Material names

[RemiArnaud]

A Material is an effect instance, and can change values on the instaced effect.
This PR introduces effect instances, and apply the material name to the instance, since this instance is directly the GLTF Material.
This fixes Material names in the exported gltf.

Other Material attributes marked as TODO in this PR.

Example material from the spec:

  <material id="Blue"> 
    <instance_effect  url="#phongEffect"> 
      <setparam ref="AMBIENT"> 
        <float3>0.0 0.0 0.1</float3> 
      </setparam> 
      <setparam ref="DIFFUSE"> 
        <float3>0.15 0.15 0.1</float3> 
      </setparam> 
      <setparam ref="SPECULAR"> 
        <float3>0.5 0.5 0.5</float3> 
      </setparam> 
      <setparam ref="SHININESS"> 
        <float>16.0</float> 
      </setparam> 
      </instance_effect> 
  </material>  

[pjcozzi]

Thanks @RemiArnaud! @lasalvavida do you have any time to look at this?

[lasalvavida]

This functionality (minus copying other material properties) should already be available via GLTF::Material::Clone inherited from GLTF::Object::Clone which does copy the name.

However, I'm pretty sure this isn't necessary at all since glTF materials are mapped 1:1 with COLLADA instanced effects (not COLLADA materials) already, so it isn't clear to me what this pull request is actually supposed to do.

@RemiArnaud, in the future to expedite review, please include a model that does not convert correctly currently with your pull request so that it is clear what your change does and is easy for me to test.

[RemiArnaud]

Just take any collada model and observe that the material name in collada does not end up as the material name in gltf, but instead the effect name is used.

[lasalvavida]

Ah, okay. The writing of the effect name was an intentional decision on my part. This is because glTF materials map to COLLADA instanced effects, not COLLADA materials, despite their name.

I'm fine with a change to keep the material name and store it if the instanced effect does not already have a name and/or a command like flag like --materialNames that overwrites effect names with material names.

[RemiArnaud]

To be correct, the current implementation maps the collada effect to the gltf material.
The instanced effect - by definition the material in collada - should be mapped to the gltf material, which is the object of this PR, although still leaving some TODOs to implement the case where materials apply changes to the instances effect.

[RemiArnaud]

@lasalvavida. Thinking about uploading a sample model. I think it could be a good idea to collect unit tests collada files and corresponding unit tests - checking the json has the expected value. What do you think?

[lasalvavida]

I think it could be a good idea to collect unit tests collada files and corresponding unit tests - checking the json has the expected value. What do you think?

It is a good idea. There is already a unit test framework, feel free to add tests.

Thank you for the clarification. I see the issue now. Please see how instanced geometry gets resolved using the clone function when instance meshes have different materials. The solution for this should use the same approach.

[lasalvavida]

collect unit tests collada files

Though I think it's worth saying that this should be done with small tailored inline COLLADA strings and not file I/O on full COLLADA files.