Discussion :

[Hardware31]

Bonsoir,

J'ai décidé depuis quelques jours de revoir l'architecture et l'organisation des classes de mon logiciel de visualisation d'objets en 3D avec Qt5/OpenGL4. Mon programme fonctionne plutôt bien et affiche correctement les models (de type .obj, .lwo, .3ds ...etc) que je lui fournis donc pas de soucis à ce niveau là. Je décide de tester le tout avec Valgrind pour voir si il n'y a pas de problème au niveau de la gestion de la mémoire... Bon c'est un peu la douche froide de ce côté là :

• 264 bytes in 1 blocks are definitely lost in loss record 686 of 839 dans src/objects/model.cpp:33
• 264 bytes in 1 blocks are definitely lost in loss record 687 of 839 dans src/objects/model.cpp:34
• 264 bytes in 1 blocks are definitely lost in loss record 688 of 839 dans src/objects/model.cpp:35
• 288 (48 direct, 240 indirect) bytes in 1 blocks are definitely lost in loss record 693 of 839 dans src/materials/texturemanager.cpp:35
• 1,584 (48 direct, 1,536 indirect) bytes in 1 blocks are definitely lost in loss record 768 of 839 dans src/objects/meshmanager.cpp:40
• 1,584 (48 direct, 1,536 indirect) bytes in 1 blocks are definitely lost in loss record 769 of 839 dans src/materials/materialmanager.cpp:41
• 262,140 bytes in 1 blocks are possibly lost in loss record 829 of 839 dans src/objects/modelloader.cpp:124
• 524,280 bytes in 1 blocks are possibly lost in loss record 830 of 839 dans src/objects/modelloader.cpp:120
• 12,582,912 bytes in 3 blocks are possibly lost in loss record 837 of 839 dans Texture::load() dans src/materials/texture.cpp:58
• 15,456,715 (48 direct, 15,456,667 indirect) bytes in 1 blocks are definitely lost in loss record 838 of 839 dans src/objects/modelmanager.cpp:31

Je vous mets à disposition le code source de la classe ModelManager.h associée à la dernière erreur signalée par Valgrind. Je vais peut-être résoudre le reste si on y arrive pour celle-çi.

Voici le code de la classe qui doit charger et créer un model dans la scene. J'ai mis en rouge la ligne qui alloue dynamiquement de la mémoire pour un model et qui est signalée par Valgrind:

AbstractModelManager.h

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
#include <string>
#include <memory>
 
using namespace std;
 
class AbstractModel;
 
class AbstractModelManager
{
 
public:
    AbstractModelManager();
 
    virtual AbstractModel* getModel(const string& name) = 0;
    virtual void loadModel(const string& name, const string& filename) = 0;
    virtual unique_ptr<AbstractModel> createModel(const string& name) = 0;
};

ModelManager.h

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
#include "abstractmodelmanager.h"
#include "modelloader.h"
 
class Scene;
class Model;
class AbstractModel;
 
class ModelManager : public AbstractModelManager
{
 
public:
    ModelManager(Scene* scene);
    virtual ~ModelManager();
 
    virtual AbstractModel* getModel(const string& name);
    virtual void loadModel(const string& name, const string& filename);
    virtual unique_ptr<AbstractModel> createModel(const string& name);
 
private:
    Scene* m_scene;
    ModelLoader m_modelLoader;
    map<string, unique_ptr<Model>> m_models;
 
};

ModelManager.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
#include "modelmanager.h"
#include "scene.h"
#include "model.h"
#include "abstractmodel.h"

ModelManager::ModelManager(Scene* scene)
    : m_scene(scene),
      m_modelLoader(ModelLoader())
{}

ModelManager::~ModelManager() {}

AbstractModel* ModelManager::getModel(const string& name)
{
    if(m_models.find(name) != m_models.end())
    {
        return m_models[name].get();
    }

    return nullptr;
}

void ModelManager::loadModel(const string& name, const string& filename)
{
    vector<shared_ptr<ModelData>> modelData = m_modelLoader.loadModel(name, filename);
    m_models[name] = unique_ptr<Model>(new Model(m_scene, modelData));
}

unique_ptr<AbstractModel> ModelManager::createModel(const string& name)
{
    if(m_models.find(name) != m_models.end())
    {
        return unique_ptr<AbstractModel>(new Model(*m_models[name].get()));
    }

    return unique_ptr<AbstractModel>(nullptr);
}

Voila, malgré l'utilisation de unique_ptr, il y a quand même une fuite de mémoire de 15Mo... je ne sais vraiment pas comment résoudre ce problème

Merci d'avance

EDIT :

Le problème est peut-être causé par une autre classe, le reste du code source est disponible dans mon dépôt sur github : https://github.com/hardware/ObjectVi...er/src/objects
Et voici le diagramme de classes que j'ai fait récemment. il n'est pas finalisé, il y a peut-être des erreurs : http://image.noelshack.com/fichiers/...assdiagram.png

[Kaamui]

Tu n'as pas à faire cette allocation en utilisant le mot clé new, c'est justement le principe des pointeurs intelligent qui vont "gérer" le new et le "delete" à ta place. <= oui grosse bêtise dsl

[jo_link_noir]

Les pointeurs intelligents ne s'occupent pas des new, juste des delete. Il est néanmoins possible de cacher le new avec make_unique (C++14 il me semble).

À mon avis, la fuite mémoire sur le unique_ptr est un effet de bord dû à une fuite mémoire sur ModelManager. Il faut plutôt chercher où son détruites les instances de ModelManager.

EDIT: en regardant la fonction createModel, je me dit que shared_ptr serait plus approprier. Il n'y aurait pas de copies des modèles.

[Hardware31]

EDIT: en regardant la fonction createModel, je me dit que shared_ptr serait plus approprier. Il n'y aurait pas de copies des modèles.

Yep bonne idée, c'est plus simple à gérer comme ça et ça évite une copie inutile. J'ai modifier mon code en tenant compte de ce que tu as dit et ça marche nickel

Bon j'ai réussi à résoudre quelques fuites de mémoire en utilisant shared_ptr au lieu d'un pointeur nu pour stocker les adresses des instances de Meshes, Textures et Materials et de leurs Managers.
Par la suite je pense utiliser un conteneur intelligent pour stocker les meshes, materials et textures d'un model grâce à la librairie fournit par boost. Apparemment ça permet d'éviter l'overhead à cause du compteur de référence de ce type de pointeur, en tout cas c'est ce qui est recommandé sur certains sites, est-ce une bonne idée ?

Il me reste encore 4 erreurs signalées par Valgrind :

• 3,317 (56 direct, 3,261 indirect) bytes in 1 blocks are definitely lost in loss record 784 of 847 dans src/objects/modelmanager.cpp:31
• 8,709 (56 direct, 8,653 indirect) bytes in 1 blocks are definitely lost in loss record 818 of 847 dans src/materials/materialmanager.cpp:41
• 6,314,366 (56 direct, 6,314,310 indirect) bytes in 1 blocks are definitely lost in loss record 844 of 847 dans src/objects/meshmanager.cpp:40
• 6,507,279 (56 direct, 6,507,223 indirect) bytes in 1 blocks are definitely lost in loss record 845 of 847 dans src/materials/texturemanager.cpp:35

Quelqu'un aurait une idée pour résoudre les erreurs qui restes ?

[Hardware31]

J'ai peut-être pas très bien expliqué l'organisation de mon programme. Pour vous aider à comprendre voila comment est organisé le processus de chargement d'un modèle :

1] La scène crée le ModelManager :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

unique_ptr<AbstractModelManager> m_modelManager = unique_ptr<AbstractModelManager>(new ModelManager(this));

2] Ensuite elle crée les 3 managers pour manipuler les Meshes, les Textures et les Materials :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
shared_ptr<AbstractMeshManager>     m_materialManager = make_shared<MaterialManager>(shader);
shared_ptr<AbstractTextureManager>  m_textureManager  = make_shared<TextureManager>(shader);
shared_ptr<AbstractMaterialManager> m_meshManager     = make_shared<MeshManager>(shader);

3] Puis elle charge le modèle et appelle la méthode "render()" pour le faire apparaitre dans la zone de rendu :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

shared_ptr<Model> m_model = m_modelManager->loadModel("TANK", "assets/tank/awesome_tank.3ds");

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
void Scene::render()
{
     // ...
     m_model->render();
     // ...
}

4] Pour charger un modèle tout commence dans cette méthode :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
shared_ptr<Model> ModelManager::loadModel(const string& name, const string& filename)
{
    // Récupération de toutes les informations liées au modèle
    vector<shared_ptr<ModelData>> modelData = m_modelLoader.loadModel(name, filename);
 
    // Création d'un nouveau model avec les informations récupérées au dessus puis stockage dans un conteneur
    m_models[name] = make_shared<Model>(m_scene, modelData);
 
    return m_models[name];
}

5] Ensuite le constructeur de la classe Model initialise le modèle en appelant la méthode Model::initialize() :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
void Model::initialize(vector< shared_ptr<ModelData> > modelData)
{
    m_meshManager = m_scene->meshManager();
    // ... 
 
    // Chaque Mesh / Texture / Materiaux sont ajoutés 1 par 1
    for(shared_ptr<ModelData>& data : modelData)
    {
        shared_ptr<Mesh> mesh = m_meshManager->getMesh(data->meshData.name);
 
        if(mesh == nullptr)
        {
            mesh = m_meshManager->addMesh(data->meshData.name,
                                          data->meshData.positions,
                                          data->meshData.colors,
                                          data->meshData.texCoords,
                                          data->meshData.normals,
                                          data->meshData.tangents);
        }
 
	m_meshes.push_back(mesh);
 
	// ... Ensuite c'est au tour des textures et des matériaux mais j'ai enlevé le reste car c'est la même chose qu'au dessus
    }
}

6] Et pour finir voici la méthode qui permet d'ajouter un nouveau Mesh :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
shared_ptr<Mesh> MeshManager::addMesh(const string& name,
                                      const vector<QVector3D>& positions,
                                      const vector<QVector4D>& colors,
                                      const vector<QVector2D>& texCoords,
                                      const vector<QVector3D>& normals,
                                      const vector<QVector3D>& tangents)
{
    if(m_meshes.find(name) != m_meshes.end() && m_meshes[name].get() != nullptr)
    {
        return m_meshes[name];
    }

    m_meshes[name] = make_shared<Mesh>(name, positions, colors, texCoords, normals, tangents, m_shader);

    return m_meshes[name];
}

Selon Valgrind il y aurait une fuite de mémoire lorsque l'on alloue dynamiquement de la mémoire lors de la création d'un Mesh/Texture/Material mais je vois pas pourquoi...
Selon vous, ce que j'ai fait semble logique / correcte en C++11 ? Comment améliorer le tout pour éviter au maximum les fuites de mémoires ?

[jo_link_noir]

Tu as oublié de mettre le destructeur virtuelle dans AbstractModelManager. Et comme tu manipules des pointeurs sur AbstractModelManager le destructeur des classes fille n'est pas appelé.
Gcc envoie un warning si le flag -Wnon-virtual-dtor est utilisé.