Associate library materials effect with meshes

[ahcorde]

This PR is related with this issue #91 (comment)

The material could be defined in Collada like this:

  <library_materials>
    <material id="material-cube">
      <instance_effect url="#effect-cube"/>
    </material>
  </library_materials>

This kind of material definition were never associated with the mesh. This PR try to fix this issue.

Signed-off-by: ahcorde [email protected]

[codecov]

Codecov Report

Merging #151 (2dfff6e) into ign-common3 (c98d0d2) will increase coverage by 0.04%.
The diff coverage is 94.73%.

@@               Coverage Diff               @@
##           ign-common3     #151      +/-   ##
===============================================
+ Coverage        75.01%   75.06%   +0.04%     
===============================================
  Files               72       72              
  Lines            10215    10234      +19     
===============================================
+ Hits              7663     7682      +19     
  Misses            2552     2552              

Impacted Files	Coverage Δ
graphics/src/ColladaLoader.cc	85.38% <94.73%> (+0.20%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c98d0d2...2dfff6e. Read the comment docs.

[chapulina]

Would it be possible to add a test? Maybe using the mesh provided in #91 ?

[ahcorde]

+1 for the test. Added here 188d06d

[peci1]

I'm sorry, @ahcorde , but this PR seems to break some other mesh parsing. X1 SubT robots are no longer blue, EXPLORER_DS1 has a weird red-ish color instead of carbon black...

Check the behavior of ColladaLoader with ~/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/x1 config 1/18/meshes/chassis.dae.

This file has one <vertices> and two <triangles>. Each triangle list defines a different symbol for material. However, after loading the mesh with ColladaLoader, both submeshes have material index 1 assigned. Without this PR, submesh 0 correctly has material index 0. Here's a simple (wrongly) failing test for me:

TEST_F(ColladaLoader, LoadX1)
{
  common::ColladaLoader loader;
  common::Mesh *mesh = loader.Load("/home/peci1/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/x1 config 1/18/meshes/chassis.dae");

  ASSERT_EQ(2u, mesh->SubMeshCount());
  ASSERT_EQ(2u, mesh->MaterialCount());

  EXPECT_EQ(0u, mesh->SubMeshByIndex(0).lock()->MaterialIndex());
  EXPECT_EQ(1u, mesh->SubMeshByIndex(1).lock()->MaterialIndex());
}

Reading through #91 and Collada spec, I think the mesh submitted by @jaelrod is not what he expected it to be. The "triangles" section (p. 188) says:

material xs:NCName Declares a symbol for a material. This symbol is bound to a material at the time of instantiation; see <instance_geometry> and <bind_material>. Optional. If not specified then the lighting and shading results are application defined.

@jaelrod was concerned about reusability of the library geometries. But I think there was a misunderstanding on what does the <triangles material=""> attribute actually mean. It does NOT specify the material to use. It specifies the symbol to which a material has to be bound in the <instance_geometry>. So it's not a "value", but a "variable name". I think there's nothing against naming the "variable" (material), as long as you can bind different materials to it (referencing the name in the <instance_material symbol=""> attribute and specifying the selected material in <instance_material target=""> attribute).

The mesh from #91 could work if the default value for the <triangles material=""> were "material". But the spec says that if this attribute is missing, shading is application-dependent. This is probably where application behavior starts to differ (and that's correct, as the spec says). To be compatible with Blender (which is generally a nice thing, I would say), this PR should be reverted and the change should be somewhere near

https://github.com/ignitionrobotics/ign-common/blob/abfecc8a262b32e3df7495f11b04fd38f5594f68/graphics/src/ColladaLoader.cc#L2009-L2029

I think this if could assume that there is a default value for the material attribute - "material". It should just silence the warnings in case it tried to find a material and did not succeed.