Open anamani opened 11 years ago
eatse21
commented
11 years ago Cela est dû aux nombreuses erreurs mémoire provoquées en jeu et à sa fermeture.
Ça arrive notamment lorsqu'un élément d'une liste/vecteur est supprimé (via erase) pendant une boucle sur l'ensemble des éléments.
Il y a aussi des variables non initialisées qui sont utilisées.
Elvoniel
commented
11 years ago Yes va falloir reprendre tout ça petit à petit, ça va être marrant... Le truc c'est que ça ne le fait pas chez nous... étrange ! Ça dépend du comportement du système d'exploitation face à ces erreurs mémoire ?
eatse21
commented
11 years ago Oui, à ce niveau là ça dépend comment est géré le processus par le système.
La plupart des erreurs mémoires sont ignorées, tant qu'elles restent dans la zone de mémoire que le système autorise au processus.
Cependant il est possible de les traquer et de les afficher via des outils de débogage (comme valgrind sous Linux, qui permet aussi de voir les delete oublié et plein d'autres choses).
Une bonne chose à faire pour commencer c'est de faire en sorte que votre jeu compile sans aucun warning. Si vous êtes sur code::blocks par exemple, allez dans project->build options->Other options et rajoutez les flags -Wall -Wextra -Werror, ça peut être pénible mais ça permet de corriger bien des erreurs ;)
Sinon concernant les conteneurs il y a deux choses importantes:
1- On parcours toujours les conteneurs via ses itérateurs, jamais à la façon C avec un compteur et à coup de at()
2- Lorsqu'on supprime un élément du conteneur pendant son parcours, soit on revient au premier élément (peu efficace), soit on récupère l'itérateur retourné par erase, qui n'est d'autre que l'élément suivant celui qui a été supprimé, afin éviter les exceptions du type std::out_of_range (ligne 208, Map.cpp), et on évite la boucle for, puisqu'on ne peut alors pas maitriser l'incrémentation de l'itérateur.
Ainsi, la première partie de Map::update:
for(unsigned int n=0; n < AllBullets.size(); n++)
{
AllBullets.at(n).UpdatePosition();
if(Engine::getInstance()->getCollisionManager()->CheckIfOutOfWindow(AllBullets.at(n).getPosition().x, AllBullets.at(n).getPosition().y, 0.0f, 0.0f, 0.0f) == true)
AllBullets.erase(AllBullets.begin()+n);
else if(Engine::getInstance()->getCollisionManager()->CollisionObstacles(AllBullets.at(n).getGlobalBounds()))
{
AllBullets.erase(AllBullets.begin()+n);
}
else if(Engine::getInstance()->getCollisionManager()->CollisionEnnemy(AllBullets.at(n).getGlobalBounds(), EnnemyArray))
{
EnnemyTouche = Engine::getInstance()->getCollisionManager()->getAdresseEnnemyTouche();
EnnemyTouche->subirDegats(AllBullets.at(n).getDamage());
}
}Pourrait devenir:
//On récupère d'avance le CollisionManager pour éviter des appels de fonction inutiles
CollisionManager& collisionManager = *Engine::getInstance()->getCollisionManager();
std::vector<Bullet>::iterator n = AllBullets.begin();
while (n != AllBullets.end())
{
n->UpdatePosition();
if(collisionManager.CheckIfOutOfWindow(n->getPosition().x, n->getPosition().y, 0.0f, 0.0f, 0.0f) == true)
n = AllBullets.erase(n);
else if(collisionManager.CollisionObstacles(n->getGlobalBounds()))
n = AllBullets.erase(n);
else
{
if(collisionManager.CollisionEnnemy(n->getGlobalBounds(), EnnemyArray))
{
EnnemyTouche = collisionManager.getAdresseEnnemyTouche();
EnnemyTouche->subirDegats(n->getDamage());
}
++n;
}
}Notez que la solution que je propose fonctionne avec tous types de conteneurs, pas seulement les vector.
Elvoniel
commented
11 years ago Merci beaucoup de tes conseils Kratos !
Il est vrai que la gestion de la mémoire précédente était dégueulasse. Je me suis plongé dans le code et ait suivit tes conseils : j'ai modifié le parcours des conteneurs pour qu'il soit correcte et utilise les iterators et j'ai modifié les boucles pour mieux gérer l'incrémentation de l'itérateur. Il faudrait juste passer un coup de valgrind pour voir ce qui se passe maintenant.
Enfin j'ai supprimé tous les warnings et j'ai compilé avec les options -Wall -Wextra -Werror dont tu nous a parlé.
Il ne devrait plus y avoir de message d'erreur dans le style de celui qu'anamani à eu.
Encore merci de l'intérêt que tu portes au projet Kratos et des conseils précieux que tu nous a fournit !
Francis1993Z
commented
11 years ago Merci beaucoup!
eatse21
commented
11 years ago (Cette réponse est en relation directe avec #3 et #4) Après quelques vérifications, il reste encore 2-3 erreurs qui persistent.
J'ai remarqué qu'en jouant sur une map ne contenant qu'une seule vague d'un seul ennemi, il y a environ 2 chances sur 3 que le jeu quitte directement.
Cela vient des attributs m_fx et m_fy de Ennemy qui ne sont pas initialisés.
Du coup, au premier passage dans Zombie::update, les ennemis peuvent se retrouver à l’extérieur du jeu.
Deuxièmement, il y a un petit soucis dans la deuxième boucle de Map::update (ligne 207).
D'une part la méthode erase du conteneur est appelée (mais ceci est correctement géré maintenant), et de l'autre part, via (*itEnnemy)->die(), lorsque l'ennemi est un Splitter, la méthode push_back du conteneur est aussi appelée(Splitter.cpp ligne 103-104), et là un problème se pose. En effet selon http://www.cplusplus.com/reference/vector/vector/push_back/#validity (paragraphe Iterator validity), si le conteneur est plein et doit ré-allouer de la mémoire pour ajouter un nouvel élément (un nouveau Splitter), tous les itérateurs en rapport avec ce conteneur deviennent invalide.
Donc lorsque que les anciens itérateurs continuent d'être utilisés après ce fameux push_back, ça peut mener à la catastrophe :(
Deux solutions à ça :
erase):player->addPoints((*itEnnemy)->die());
delete *itEnnemy;
itEnnemy = EnnemyArray.erase(itEnnemy);En quelque chose comme :
Ennemy* to_delete = *itEnnemy;
EnnemyArray.erase(itEnnemy);
player->addPoints(to_delete->die());
delete to_delete;
itEnnemy = EnnemyArray.begin();Troisièmement, bien qu'il n'y a pas encore de souci là dessus, les classes contenant des méthodes virtuelles.
Toujours mettre un destructeur virtuel, même s'il ne fait rien, et toujours placer le mot virtual derrière les méthodes qui doivent l'être (ce n'est pas le cas dans Zombie/Splitter.hpp), et ne pas en mettre là où ce n'est pas nécessaire (CollisionManager pour le destructeur par exemple)
Cela permettra le bon fonctionnement de vos classes génériques (Ennemy par exemple) sans erreur étrange.
Enfin, en tant que conseil, initialisez toujours les attributs de vos classes (c-f le premier point) et utilisez la forme canonique (dite de Coplien). Cela permet d'éviter au compilateur de générer des contructeurs/méthodes de lui même et vous permet donc de bien contrôler ce qui se passe.
La forme canonique, c'est une classe qui contient un constructeur par défaut (avec ou sans argument), un constructeur par copie, un destructeur et un opérateur =
En faisant ça, si vous ne voulez pas qu'une classe puisse être copiée, vous déclarez juste le constructeur par copie et l'opérateur= en private, ça permet d'éviter que dans Engine::Run on puisse faire widgetManager = widgetManager et que cela compile par exemple.
progyold
commented
11 years ago Salut !
Tout d'abord je tiens à te remercier très vivement pour l'intérêt que tu portes à notre projet. Tes conseils sont très bons, et nous permettent de régler de gros soucis de mémoire ou d'incohérences dans notre programme. Ainsi, grâce à eux, on avance !
En ce qui concerne l'explication que tu viens de donner, le nécessaire va sûrement être fait (ce n'est pas moi qui suis chargé de le faire ;) ). Je sais que Francis a déjà commencé par initialiser m_fx et m_fy, dans les constructeurs de Zombie et Splitter. Ensuite, je pense que le vector qui pose problème va être changé par une liste.
A+
constructeur06
Dès que je quitte le jeu, que ce soit avec "Échap" ou à la fin des vagues, le jeu m'affiche toujours la console (normal) mais elle est accompagnée d'une fenêtre avec un message d'erreur..... Que voici ! Message d'erreur