[model_io] Fix the color parsing in the model_io

[GiulioRomualdi]

Once VisualElement::childElementForName is called with input equal to 'material' a pointer to the
material info is stored inside the VisualElement.

Rationale

Differently from GeometrcElement, the constructor of MaterialElement takes as input a copy of std::shared_ptr<MaterialInfo> and not a reference to a shared pointer

idyntree/src/model_io/urdf/src/MaterialElement.cpp

Lines 23 to 30 in cbb0bf5

	MaterialElement::MaterialElement(std::shared_ptr<MaterialInfo> materialInfo)
	: iDynTree::XMLElement("material")
	, m_info(materialInfo)
	{
	if (!m_info) {
	m_info = std::make_shared<MaterialInfo>();
	}
	}

For this reason in the following line

idyntree/src/model_io/urdf/src/VisualElement.cpp

Line 51 in cbb0bf5

return std::make_shared<MaterialElement>(m_info.m_material);

Since m_info.m_material = nullptr and MaterialElement copies the value, m_info.m_material was not updated by

     if (!m_info) { 
         m_info = std::make_shared<MaterialInfo>(); 
     } 

Passing a reference to a shared ptr cannot solve the problem in this particular case. Indeed given the following code:

idyntree/src/model_io/urdf/src/RobotElement.cpp

Lines 53 to 56 in cbb0bf5

	} else if (name == "material") {
	// These materials constitute a model-level database of materials
	// TODO: the result of the parsing should be added to the database
	return std::make_shared<MaterialElement>(nullptr);

the compiler will complain since nullptr is a r-value and it cannot be converted into a l-value.

Outcome of the PR

This is how Talos is now displayed with

meshcat

irrlicht

For iCub there is the usual problem with the colors

meshcat

irrlicht

[traversaro]

Great, quite a clean fix. Can you add a line on the CHANGELOG?

If you plan to work on robotology/icub-models-generator#216 we can wait for that also to be ready and merge/release both things together, otherwise no problem, we can merge this and enjoy a colorful iCub!

[traversaro]

By the way, checking the CI failure, the bugfix revealed a bug in the test models that was "hidden" by this bug:

Error:  URDFDocument :: addVisualPropertiesToModel : Material for link head has not rgba and it is not in the global material database.
Error:  URDFDocument :: documentHasBeenParsed : Failed to add visual elements to model
Model loaded from /home/runner/work/idyntree/idyntree/src/tests/data/iCubGenova02.urdf

Can you fix the vendored /home/runner/work/idyntree/idyntree/src/tests/data/iCubGenova02.urdf model? We could also try to fix it with a latest model from icub-models,but I am not sure if then we need to also fix other tests that have hardcoded expectations on the number of elements (however, we can try).

[traversaro]

By the way, checking the CI failure, the bugfix revealed a bug in the test models that was "hidden" by this bug:

Error:  URDFDocument :: addVisualPropertiesToModel : Material for link head has not rgba and it is not in the global material database.
Error:  URDFDocument :: documentHasBeenParsed : Failed to add visual elements to model
Model loaded from /home/runner/work/idyntree/idyntree/src/tests/data/iCubGenova02.urdf

Can you fix the vendored /home/runner/work/idyntree/idyntree/src/tests/data/iCubGenova02.urdf model? We could also try to fix it with a latest model from icub-models,but I am not sure if then we need to also fix other tests that have hardcoded expectations on the number of elements (however, we can try).

I looked into the iCubGenova02.urdf model and actually it is a corner case that perhaps it is not clear from the URDF spec (i.e. http://wiki.ros.org/urdf/XML/link). The head link refer to material head (see

idyntree/src/tests/data/iCubGenova02.urdf

Line 1394 in cbb0bf5

<material name="white"/>

), that is defined by the material element under the link r_foot (see

idyntree/src/tests/data/iCubGenova02.urdf

Line 1394 in cbb0bf5

<material name="white"/>

), and not by a material element under the robot element. An easy fix could be to either add all link materials to the database, or jus fix the /home/runner/work/idyntree/idyntree/src/tests/data/iCubGenova02.urdf model.

[GiulioRomualdi]

Hi, @traversaro I think it's easier to fix the model. Should update the model to the latest one stored in icub-models?

[traversaro]

Should update the model to the latest one stored in icub-models?

You can try.

[GiulioRomualdi]

Should update the model to the latest one stored in icub-models?

You can try.

After a short discussion with @traversaro, we decided to fix directly the material tag in the urdf stored in the repo.

[GiulioRomualdi]

The PR introduces also the idyntree-model-view-meshcat application. Thanks to this a model can be easily displayed in meshcat using

idyntree-model-view-meshcat -m <model-path>

a webpage with the visualizer will be automatically opened

Since robotology/icub-models-generator#217 has been merged the robots will be displayed as follows

IRRLICHT	MESHCAT

[traversaro]

This will hardcode the python executable used at build time in this script. Do you have any specific reason for which you do not want to use the usual Python3 shebang?

Suggested change

	#!@Python3_EXECUTABLE@
	#!/usr/bin/env python3

[GiulioRomualdi]

I was thinking if a virtual enviroment is used

[traversaro]

For that using /usr/bin/env python3 should work fine.

[GiulioRomualdi]

Ok! perfect!

[GiulioRomualdi]

Hi @traversaro, 3c9e243 should implement what you suggested

[traversaro]

Can you update the changelog?

[GiulioRomualdi]

Can you update the changelog?

Done in 69378f6

I opened this PR on master and I noticed that the folder structure has been changed in devel. How we should proceed? We merge it in master and then me merge master on devel (fixing the conflicts)?

[traversaro]

@GiulioRomualdi can I merge or you want to do further changes?

[GiulioRomualdi]

Feel free to merge it