Destroy material when a mesh is deleted #324

ahcorde · 2021-05-18T20:19:38Z

🎉 Enhancement

Summary

This PR allows to remove materials when a mesh is removed.

There are two cases:

When the material is loaded, the method in charge to load the meshes will create a cache of material. I added a method to remove this cache
When a mesh is removed the material/s are not removed from the memory. This PR also includes this step.

I had to break the ABI, I'm open to suggestions to be able to backport this PR

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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

Signed-off-by: ahcorde <[email protected]>

iche033 · 2021-05-19T05:43:49Z

ogre2/src/Ogre2MeshFactory.cc

@@ -88,6 +89,17 @@ void Ogre2MeshFactory::Clear()
  this->ogreMeshes.clear();
 }

+void Ogre2MeshFactory::ClearMaterialsCache()


I verified that this fixes the memory leak. A different issue is created after removing these materials though. Here are the steps to reproduce:

Insert a model like this one (I just dragged and dropped) https://app.ignitionrobotics.org/GoogleResearch/fuel/models/Weisshai_Great_White_Shark

it needs to be a model that references a texture in the dae or obj file but does not have a sdf element

Insert the same model again -> model appear grey

After debugging a little, I remembered that these materials were created as "templates". Later on in this line https://github.com/ignitionrobotics/ign-rendering/blob/6fd0c03ce32568f8bc91c1041fc169cc7622248f/ogre2/src/Ogre2MeshFactory.cc#L601 a "new instance" is created and assigned to the submesh. This is done by looking for the name of the template material stored in the submesh, getting the template material from the scene, then assigning a copy to the submesh. Now that this template material is removed, subsequent meshes requiring the same material will not be able to find it.

One possible solution is to remove the code for creating these template materials, i.e. the code here: https://github.com/ignitionrobotics/ign-rendering/blob/6fd0c03ce32568f8bc91c1041fc169cc7622248f/ogre2/src/Ogre2MeshFactory.cc#L455-L471

we just then need to always create a new material and assign it to the submesh, i.e. replace the logic here: https://github.com/ignitionrobotics/ign-rendering/blob/6fd0c03ce32568f8bc91c1041fc169cc7622248f/ogre2/src/Ogre2MeshFactory.cc#L590-L602

I ran out of time today and haven't found a way to do his cleanly yet..

@iche033, I don't fully understand why this is happening, I would expect this three things

when we load a new mesh then the "template" material is loaded and removed.

When we remove the mesh we remove the material created fom the template

At this point we should not have any material associated with the mesh in memory.

Finally we load again the same mesh and I would expect to reload everything again (template and material created from the template).

am I missing something ?

Finally we load again the same mesh and I would expect to reload everything again (template and material created from the template).

we have this check here to skip loading the mesh if it's already loaded in ogre: https://github.com/ignitionrobotics/ign-rendering/blob/6fd0c03ce32568f8bc91c1041fc169cc7622248f/ogre2/src/Ogre2MeshFactory.cc#L167

That was mainly done for performance reason, e.g. when launching a world with hundreds of the same model, esp if they have large complex meshes.

Signed-off-by: ahcorde <[email protected]>

ahcorde · 2021-05-19T20:38:59Z

With the last commit, I fixed the issue that @iche033 mentioned and also fixed the small leak

Signed-off-by: ahcorde <[email protected]>

codecov · 2021-05-19T21:14:03Z

Codecov Report

Merging #324 (e3b10e8) into main (ffd8e6d) will decrease coverage by 0.02%.
The diff coverage is 54.28%.

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
- Coverage   57.71%   57.68%   -0.03%     
==========================================
  Files         160      160              
  Lines       15887    15992     +105     
==========================================
+ Hits         9169     9225      +56     
- Misses       6718     6767      +49

Impacted Files	Coverage Δ
ogre/src/OgreMesh.cc	`0.00% <0.00%> (ø)`
ogre/src/OgreMeshFactory.cc	`1.06% <0.00%> (-0.06%)`	⬇️
ogre/src/OgreScene.cc	`27.77% <0.00%> (-0.30%)`	⬇️
ogre/src/OgreMaterial.cc	`38.90% <55.00%> (+1.10%)`	⬆️
ogre2/src/Ogre2MeshFactory.cc	`81.94% <56.25%> (-1.52%)`	⬇️
ogre2/src/Ogre2Material.cc	`92.18% <78.94%> (-0.84%)`	⬇️
ogre2/src/Ogre2Mesh.cc	`83.57% <100.00%> (+2.57%)`	⬆️
ogre2/src/Ogre2Scene.cc	`80.41% <100.00%> (+0.12%)`	⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh	`96.66% <0.00%> (-3.34%)`	⬇️

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 ffd8e6d...e3b10e8. Read the comment docs.

ahcorde · 2021-05-20T08:54:43Z

There is still an issue when you insert more than one model of the same type, from the second model they will not get the material

Signed-off-by: ahcorde <[email protected]>

iche033

confirms that the latest changes fix the missing texture issue for subsequent models. Memory consumption for repeatedly adding and removing models also look to be stable.

iche033 · 2021-05-21T00:24:33Z

ogre/src/OgreMesh.cc

@@ -58,6 +58,8 @@ void OgreMesh::Destroy()

  auto ogreScene = std::dynamic_pointer_cast<OgreScene>(this->Scene());

+  Ogre::MeshManager::getSingleton().remove(


this removes the template mesh. Should we have a similar logic as what you did for textures and check reference count to the mesh first before removing it? Same for the remove call in OgreSubMesh::Destroy()

iche033 · 2021-05-21T00:31:03Z

ogre/src/OgreMaterial.cc

+        if (this->textureName == res->getName() &&
+          res->getName().find("scene::RenderTexture") == std::string::npos)
+        {
+          this->Scene()->ClearMaterialsCache(this->textureName);


Here's an idea to prevent breaking ABI. We can add a new OgreScene::MeshFactory() accessor function to OgreScene class (and also Ogre2Scene) class to get the mesh factory then here we just need to do:

OgreScenePtr s = std::dynamic_pointer_cast<OgreScene>(this->Scene()); auto factory = s->MeshFactory(); factory->ClearMaterialCache(this->textureName);

This prevents adding and overriding a virtual function in Scene.

iche033 · 2021-05-21T00:56:59Z

Memory consumption for repeatedly adding and removing models also look to be stable.

Actually that was testing with ogre 1.x. I just tried ogre 2.x, I think there is still some leak.

Signed-off-by: ahcorde <[email protected]>

ahcorde · 2021-05-24T14:46:29Z

@mjcarroll and @iche033 I open this PR for review

Signed-off-by: ahcorde <[email protected]>

iche033

nice! the latest changes fix the ogre2 memory leak for me. I just have a few coding style and doc comments

ogre/include/ignition/rendering/ogre/OgreMeshFactory.hh

ogre/include/ignition/rendering/ogre/OgreScene.hh

ogre/src/OgreMeshFactory.cc

ogre2/include/ignition/rendering/ogre2/Ogre2Mesh.hh

ogre2/src/Ogre2Mesh.cc

ogre2/src/Ogre2MeshFactory.cc

ogre/src/OgreMaterial.cc

Signed-off-by: ahcorde <[email protected]>

iche033

looks good to me. just one minor typo.

ogre2/src/Ogre2Mesh.cc

Signed-off-by: ahcorde <[email protected]>

* Destroy material when a mesh is deleted Signed-off-by: ahcorde <[email protected]> * Fixed issue Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Improved implementation Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Ogre2 texture destruction some progress Signed-off-by: ahcorde <[email protected]> * Remove meshes when there is no reference Signed-off-by: ahcorde <[email protected]> * cleanup code Signed-off-by: ahcorde <[email protected]> * Avoid breaking ABI Signed-off-by: ahcorde <[email protected]> * make linters happy Signed-off-by: ahcorde <[email protected]> * Fixed tests Signed-off-by: ahcorde <[email protected]> * Fix macos warnings Signed-off-by: ahcorde <[email protected]> * Added feedback Signed-off-by: ahcorde <[email protected]> * Doc fix Signed-off-by: ahcorde <[email protected]> Signed-off-by: William Lew <[email protected]>

Destroy material when a mesh is deleted

6fd0c03

Signed-off-by: ahcorde <[email protected]>

ahcorde requested a review from iche033 as a code owner May 18, 2021 20:19

ahcorde marked this pull request as draft May 18, 2021 20:19

github-actions bot added the 🏯 fortress Ignition Fortress label May 18, 2021

ahcorde mentioned this pull request May 18, 2021

Mesh/material memory leak gazebosim/gz-gui#208

Open

iche033 reviewed May 19, 2021

View reviewed changes

traversaro mentioned this pull request May 19, 2021

Some Collada Materials not correctly loaded when using Ogre >= 1.10 gazebosim/gazebo-classic#3002

Open

Fixed issue

ba78813

Signed-off-by: ahcorde <[email protected]>

make linters happy

41c0dae

Signed-off-by: ahcorde <[email protected]>

ahcorde added 3 commits May 20, 2021 22:16

Improved implementation

aee6715

Signed-off-by: ahcorde <[email protected]>

Merge branch 'main' into ahcorde/detroy_material

4e15e94

make linters happy

901c059

Signed-off-by: ahcorde <[email protected]>

iche033 reviewed May 21, 2021

View reviewed changes

ahcorde added 6 commits May 21, 2021 22:27

Ogre2 texture destruction some progress

e5b3387

Signed-off-by: ahcorde <[email protected]>

Remove meshes when there is no reference

eb968a9

Signed-off-by: ahcorde <[email protected]>

cleanup code

95db6a6

Signed-off-by: ahcorde <[email protected]>

Avoid breaking ABI

8e3d392

Signed-off-by: ahcorde <[email protected]>

make linters happy

27e51f0

Signed-off-by: ahcorde <[email protected]>

Fixed tests

5ab7dcc

Signed-off-by: ahcorde <[email protected]>

ahcorde self-assigned this May 24, 2021

ahcorde marked this pull request as ready for review May 24, 2021 14:46

Fix macos warnings

0b4e401

Signed-off-by: ahcorde <[email protected]>

iche033 reviewed May 24, 2021

View reviewed changes

Added feedback

535731a

Signed-off-by: ahcorde <[email protected]>

ahcorde requested a review from iche033 May 25, 2021 08:51

iche033 approved these changes May 27, 2021

View reviewed changes

ogre2/src/Ogre2Mesh.cc Outdated Show resolved Hide resolved

Doc fix

e3b10e8

Signed-off-by: ahcorde <[email protected]>

ahcorde merged commit 3d93e9c into main May 27, 2021

ahcorde deleted the ahcorde/detroy_material branch May 27, 2021 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy material when a mesh is deleted #324

Destroy material when a mesh is deleted #324

ahcorde commented May 18, 2021 •

edited

Loading

iche033 May 19, 2021

ahcorde May 19, 2021

iche033 May 19, 2021

ahcorde commented May 19, 2021

codecov bot commented May 19, 2021 •

edited

Loading

ahcorde commented May 20, 2021

iche033 left a comment

iche033 May 21, 2021

iche033 May 21, 2021

iche033 commented May 21, 2021

ahcorde commented May 24, 2021

iche033 left a comment

iche033 left a comment

		@@ -58,6 +58,8 @@ void OgreMesh::Destroy()

		auto ogreScene = std::dynamic_pointer_cast<OgreScene>(this->Scene());

		Ogre::MeshManager::getSingleton().remove(

Destroy material when a mesh is deleted #324

Destroy material when a mesh is deleted #324

Conversation

ahcorde commented May 18, 2021 • edited Loading

🎉 Enhancement

Summary

Checklist

iche033 May 19, 2021

Choose a reason for hiding this comment

ahcorde May 19, 2021

Choose a reason for hiding this comment

iche033 May 19, 2021

Choose a reason for hiding this comment

ahcorde commented May 19, 2021

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

ahcorde commented May 20, 2021

iche033 left a comment

Choose a reason for hiding this comment

iche033 May 21, 2021

Choose a reason for hiding this comment

iche033 May 21, 2021

Choose a reason for hiding this comment

iche033 commented May 21, 2021

ahcorde commented May 24, 2021

iche033 left a comment

Choose a reason for hiding this comment

iche033 left a comment

Choose a reason for hiding this comment

ahcorde commented May 18, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading