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é.

[Hardware31]

Super merci ! Il n'y a plus aucune fuites de mémoire

Je savais pas qu'il fallait quand même mettre un destructeur virtuel dans une classe abstraite. Il faut en mettre un systématiquement ou pas ? Parce que la classe AbstractScene que j'ai depuis le début de mon projet n'en possède pas et il n'y avait pas de fuites de mémoire.

Et une dernière question
Connais-tu d'autres flags utiles à activer pour repérer ce genre d'erreur. Si j'avais activé ce flags avant, j'aurais pas perdu 3 jours comprendre d'où venait le problème.

[jo_link_noir]

Je savais pas qu'il fallait quand même mettre un destructeur virtuel dans une classe abstraite. Il faut en mettre un systématiquement ou pas ?

Dans une classe abstraite oui. À moins de vraiment savoir se que l'on fait et dans des cas très particuliers.
Le destructeur se comporte comme n'importe quelle fonction. Si une fonction n'est pas virtuelle, le compilateur va chercher celle correspondante au type manipuler, qui n'est pas forcement le type réel. Et donc ici comme le type manipulé est un AbstracModelManager c'est le destructeur de ce dernier qui est appeler.

Pour le AbstractScene c'est un coup de chance. Les classes fille n'utilise sûrement d'allocation dynamique ou alors c'est le type réel qui est utilisé et le bon destructeur est appelé.

Connais-tu d'autres flags utiles à activer pour repérer ce genre d'erreur.

Plein :p. Voici ceux de mon alias:

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
-Wall -Wextra -Wundef -Wcast-align -Wformat-security -Wunreachable-code -Wformat=2 -Werror-implicit-function-declaration -Wfloat-equal -Wshadow -Wpointer-arith -Wconversion -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wpacked -Wredundant-decls -Winline -Wdouble-promotion -Winit-self -Wcast-qual -Wold-style-cast -Woverloaded-virtual -Wnon-virtual-dtor
 
-Wall
-Wextra
-Wundef // macro inconnu dans un #if (passe avec #ifdef ou #ifndef)
-Wcast-align // problème d'alignement avec des casts
-Wformat-security // pour les problèmes de sécurités avec les fonctions de formatage: scanf/printf avec un format qui n'est pas une chaîne littéraire, etc
-Wformat=2 // vérifie le format utilisé dans printf %d pour int, etc
-Wmissing-format-attribute
-Wunreachable-code
-Werror-implicit-function-declaration // pas trouvé dans le man... équivalent de -Wimplicit-function-declaration qui est uniquement valide pour le C et l'objectif-C ?
-Wfloat-equal // utilisation d'égalité sur des flottants
-Wshadow // pour les variables locales et types cachés par d'autres variables, paramètres, etc
-Wpointer-arith // sizeof de void, opération arithmétique sur NULL
-Wconversion // conversion de type signer vers non signé, double vers int, etc
-Wmissing-declarations // pas de déclaration préalable (sauf si inline ou template)
-Wmissing-noreturn // pas d'attribut noreturn ([[noreturn]] en C++11, dépend du compilo dans les autres cas)
-Wpacked // lié à l'utilisation de__attribute__((packed)).
-Wredundant-decls // déclaration multiple
-Winline // fonction inline qui ne peut être inline (car récursive, trop grosse ou autre).
-Wdouble-promotion // conversion implicite de float -> double
-Winit-self // variable initialisé avec soit même: int i = i;
-Wcast-qual // cast qui enlève le qualifier `char const *` -> `char *` ou `char **` -> `const char **` (bon exemple pour le danger de ce dernier dans la doc)
-Wold-style-cast // cast à la C
-Woverloaded-virtual // fonction qui cache une fonction virtuelle de la classe de base (il faut mettre `using fonction_base;` pour la rendre accessible)
-Wnon-virtual-dtor // pas de destructeur virtuelle pour les classes avec une fonction virtuelle
-pedantic

À dire vrai, beaucoup ne sont pas vraiment utiles et certains comme -Wshadow peuvent même devenir contraignant. Alors des fois je compile en les supprimant: -Wno-shadow ...

[Hardware31]

Merci pour cette liste. Par contre j'ai plus de 2000 warnings qui s'affichent maintenant mais la plupart se trouvent dans les sources de Qt. Il y a pas une option à rajouter à GCC pour dire qu'on ne veut pas certains headers ?

EDIT :

En fait il faut ajouter ces lignes dans le fichier de configuration de qmake (.pro) :

Code :

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

1
2
3
4
5
6
7
QMAKE_CXXFLAGS += -isystem "/usr/include/qt/QtCore" \
                  -isystem "/usr/include/qt/QtGui" \
                  -isystem "/usr/include/qt/QtOpenGL" \
                  -isystem "/usr/include/qt/QtWidgets" \
                  -isystem "/usr/include/qt/QtOpenGLExtensions" \
                  -isystem "$$PWD/includes" \
                  -isystem "$$PWD/build/tmp/moc"

Les deux dernières lignes sont spécifiques à mon projet. De manière générale il faut ajouter toutes les librairies externes (ex: Qt, boost...etc) afin d'afficher que les warnings de son projet.

[grim7reaper]

Code :

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

1
2
3
[…]
-Wunreachable-code
[…]

Cette option n’existe plus depuis janvier 2010, tu peux donc la retirer

[JolyLoic]

Dans une classe abstraite oui. À moins de vraiment savoir se que l'on fait et dans des cas très particuliers.
Le destructeur se comporte comme n'importe quelle fonction. Si une fonction n'est pas virtuelle, le compilateur va chercher celle correspondante au type manipuler, qui n'est pas forcement le type réel. Et donc ici comme le type manipulé est un AbstracModelManager c'est le destructeur de ce dernier qui est appeler.

Il se peut que ce soit le comportement de tel ou tel compilateur, dans tel ou tel cas simple (pas d'héritage multiple, par exemple), mais en fait, le problème est plus grave que ça encore : Si on détruit un objet d'un type dérivé par l'intermédiaire d'un pointeur sur une classe de base, et que l'on n'a pas déclaré le destructeur comme virtuel dans cette classe de base, on a formellement un comportement indéfini. Je suis tombé sur de cas où, plutôt que d'appeler le mauvais destructeur, ça corrompait totalement la mémoire.

D'où la règle : classe de base => destructeur virtuel.
Une version plus raffinée de la règle étant : classe de base => (destructeur virtuel et public) ou (destructeur non virtuel et protégé). Mais la règle de base est déjà pas mal.

[jo_link_noir]

Cette option n’existe plus depuis janvier 2010, tu peux donc la retirer

Je me disais aussi, c'est étrange qu'elle ne soit pas dans le man. Mais comme gcc ne dit rien quand je met le flag, je me suis dit: on sais jamais ^^.
Hop, je l'enlève de mon alias.

JolyLoic: J'ai toujours crus ce comportement comme standard, c'est bon à savoir.