Support for Gazebo materials

[quarkytale]

🎉 New feature

Closes #10

Summary

Ported the OgreMaterialParser from the Ogre community, also used in gzweb to add minimal parsing capabilities of Gazebo materials. This would make migration from Gazebo Classic to Gazebo Sim a little bit easier.
Limitations:

• Only parse solid colors
• Custom material scripts are not supported

Task list:

• Install gazebo.material at gz-sim/media/
• Add warning with documentation page linked
• Write integration test for the parser
• Check for a solution to handle dependent materials

Test it

Run Gazebo from the source directory
gz sim src/sdformat/test/integration/model/double_pendulum.sdf -v 3

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[azeey]

Works well, but there are a few changes we need to make before merging:

[mjcarroll]

Maybe a style nit, but there is a good chunk of code here that uses ::iterator types rather than range-based for loops. I know that this is copied code so it's probably not a huge deal, but it looks inconsistent with the rest of the codebase.

[codecov]

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (f8b1a46) 65.61% compared to head (5455d1e) 65.80%.
Report is 6 commits behind head on gz-sim8.

Files	Patch %	Lines
src/rendering/MaterialParser/ConfigLoader.cc	78.67%	45 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           gz-sim8    #2269      +/-   ##
===========================================
+ Coverage    65.61%   65.80%   +0.19%     
===========================================
  Files          324      327       +3     
  Lines        30925    31212     +287     
===========================================
+ Hits         20292    20540     +248     
- Misses       10633    10672      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iche033]

Tested different Gazebo/* material names and seems to work:

I also tried Gazebo/Invalid - It rendered the link black which is probably expected but maybe add a warning that the material name is not recognized?

[iche033]

does the #:~:text=Materials part work? It takes me to the migration page but not the Materials section.

[quarkytale]

Its using text fragments, and mostly browser dependent. For me, it highlights and scrolls to the Material section but scrolls back up the page. Do you think it would be useful?

[iche033]

ah ok I thought it would jump to that section. Yes I see it highlighted. We can leave the text fragment here.

[azeey]

We can't guarantee that getValuesInFloat will return a vector of 3 (at least). For example, if the user modifies the gazebo.material file and forgets to put the third value for a color, we'd end up with a vector of size 2, but then we'd try to access ambientValues[2] which would be undefined behavior. So I think we should add a check here and other places getValuesInFloat is used in this file. Alternatively, we can have a fuction in ConfigNode that returns a color instead of a vector.

[quarkytale]

Instead of gz/math dependency in ConfigLoader and placing extra checks in MaterialParser, I'm thinking of updating getValuesInFloat to check if the vector has 3 or more values, which is common for all colors in the material file. If not then throwing an error saying "Bad material file", what do you think about that?

[azeey]

If we do that, my preference would be to create another getColorValues function or something similar so getValuesInFloat can remain a generic function that gets a list of floats for non-colors. Either way, by "throwing an error", you mean printing an error message, I definitely agree. But if you mean throw an exception, I would say we should avoid throwing exceptions since that's our general practice in the Gazebo codebase.

[quarkytale]

Added another function with the size check. Yeah that's right just printing an error message, not throwing an exception.

[azeey]

Okay, I see the size check has been added, but it would still allow out of bounds access on the vector since the code continues to assign the colors after getColorValues whether that fails or not. I would suggest something like the following:

            std::vector<float> ambientValues;
            ambientNode->getColorValues(ambientValues);
            values->ambient.emplace();
            for (std::size_t i=0; i < ambientValues.size(); ++i)
            {
                   values->ambient[i] = ambientValues[i];
            }

However, it looks like there's a bug in Color::operator[] since it doesn't return a reference. So another way would be to modify the getColorValues function to resize the vector.

  inline void getColorValues(std::vector<float> & colorValues)
  {
    getValuesInFloat(colorValues);
    if (colorValues.size() < 3) 
   {
      gzerr << "Bad material file." << std::endl;
      colorValues.resize(3);
    }
  }

[azeey]

Okay, I see the size check has been added, but it would still allow out of bounds access on the vector since the code continues to assign the colors after getColorValues whether that fails or not. I would suggest something like the following:

            std::vector<float> ambientValues;
            ambientNode->getColorValues(ambientValues);
            values->ambient.emplace();
            for (std::size_t i=0; i < ambientValues.size(); ++i)
            {
                   values->ambient[i] = ambientValues[i];
            }

However, it looks like there's a bug in Color::operator[] since it doesn't return a reference. So another way would be to modify the getColorValues function to resize the vector.

  inline void getColorValues(std::vector<float> & colorValues)
  {
    getValuesInFloat(colorValues);
    if (colorValues.size() < 3) 
   {
      gzerr << "Bad material file." << std::endl;
      colorValues.resize(3);
    }
  }

[azeey]

I have a couple more minor comments. Thanks for iterating!

There are also a few style issues, but for the sake of expediency, we'll address them later.

Feedback has been addressed