Better default material

[aclegg3]

Motivation and Context

The DEFAULT_MATERIAL was initialized with full white ambient and diffuse light, causing objects to appear flat shaded. These values have been modified for a subjectively better appearance.

How Has This Been Tested

Original:

New:

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have completed my CLA (see CONTRIBUTING)
• I have added tests to cover my changes.
• All new and existing tests passed.

[codecov]

Codecov Report

Merging #596 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #596   +/-   ##
=======================================
  Coverage   56.55%   56.55%           
=======================================
  Files         128      128           
  Lines        5621     5621           
  Branches       84       84           
=======================================
  Hits         3179     3179           
  Misses       2442     2442           

Flag	Coverage Δ
#CPP	52.01% <ø> (ø)
#JavaScript	10.00% <ø> (ø)
#Python	75.64% <ø> (ø)

---

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 d34c345...8920e2b. Read the comment docs.

[msavva]

Thanks @aclegg3 for the rapid PR. So it was indeed a default material configuration issue that was causing the lack of shading!

A comment unrelated to the content of the changes: the use of naked new makes loud alarm bells ring in my head whenever I see it outside the innards of class construction/destruction logic that is neatly paired up. 😅I assume this is safe because the ShaderManager takes responsibility to free the memory eventually?

[jturner65]

Sort of tangent to the PR, but in line with what was stated above : Since a PhongMaterialData has pointers to textures, and only a default destructor, is this not a possible source of memory leaking?

[aclegg3]

Sort of tangent to the PR, but in line with what was stated above : Since a PhongMaterialData has pointers to textures, and only a default destructor, is this not a possible source of memory leaking?

These textures live in the textures_ map as shared pointers. The pointers stored in the material structure are pointing here (assigned in getPhongShadedMaterialData), so no issue with memory leaks. We also don't allow textures to be freed during runtime, so none of these pointers should get stale.

[aclegg3]

A comment unrelated to the content of the changes: the use of naked new makes loud alarm bells ring in my head whenever I see it outside the innards of class construction/destruction logic that is neatly paired up. 😅I assume this is safe because the ShaderManager takes responsibility to free the memory eventually?

That is a valid concern. In this case we don't remove any materials once added and as a member of esp::assets::ResourceManager the shaderManager_ outlives a Simulator so there is no potential for missed deletion within the current assumptions.

@mosra I see the reference counting and freeing functionality in Magnum::ResourceManager. For these gfx::MaterialData pointers, free handles deletion, right?

[mosra]

Sorry, missed the comments above.

I assume this is safe because the ShaderManager takes responsibility to free the memory eventually?

For these gfx::MaterialData pointers, free handles deletion, right?

Yes to both. This API originates in early C++11 times and it was only updated to take r-value references and smart pointers quite recently (it was waiting for a lighter-weight unique pointer implementation), and most of existing code, docs and examples still uses the discouraged "naked new" way. The PR doesn't touch the relevant part anymore, but what could be done to hide the raw new is for example

shaderManager_.set<gfx::MaterialData>(DEFAULT_MATERIAL_KEY,
                                      gfx::PhongMaterialData{});

shaderManager_.set<gfx::MaterialData>(DEFAULT_MATERIAL_KEY,
                                      Cr::Containers::pointer<gfx::PhongMaterialData>());

or, using the standard unique pointers

#include <Corrade/Containers/PointerStl.h>

shaderManager_.set<gfx::MaterialData>(DEFAULT_MATERIAL_KEY,
                                      std::make_unique<gfx::PhongMaterialData>());

[mosra]

The change of specular to 0.2 also makes everything less extremely shiny, right? :)