Discussion :

[AbsoluteLogic]

Bonjour,

Je fais face aux joies du c++ avec des problèmes que je considère comme des "effets de bord". En effet une variable change de valeur sans raison. Je fais donc du débogage pas à pas avec gdb, en suivant la valeur d'une de mes variables "FILE* file" (Je sais que je devrais utiliser fstream en c++, mais j'obtiens le même problème, j'ai donc utilisé FILE* pour le mettre en évidence). En déboguant avec gdb, j'obtiens:
- initialisation de file dans le constructeur d'une classe Replay:

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
Breakpoint 1, Replay::Replay (this=0x7ffffffed9d8, pixels_per_unit=32, width=24, height=24)
    at ../SimplePvP/Replay.cpp:38
38      Replay::Replay(int pixels_per_unit, float width, float height)
(gdb) next
40              file = fopen("0.replay", "wb");
(gdb) next
42              WriteUint16(pixels_per_unit);
(gdb) p file
$16 = (FILE *) 0x84d2c20
(gdb) p &file
$17 = (FILE **) 0x7ffffffed9e0
(gdb) p *(FILE**)0x7ffffffed9e0
$18 = (FILE *) 0x84d2c20
(gdb) next
43              WriteFloat32(width);
(gdb) next
44              WriteFloat32(height);
(gdb) next
45      }

- bazar dans la classe de base dont file est l'attribut:

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
(gdb) next
GameManager::GameManager (this=0x7ffffffed9c0, nb_players_=3, pixels_per_unit=32, width=24, height=24)
    at ../SimplePvP/GameManager.cpp:10
10              unsigned int seed = time(NULL);
(gdb) next
11              srand(seed);
(gdb) next
12              std::cout << "Seed: " << seed << std::endl;
(gdb) next
Seed: 1567342599
13              turn = -1;
(gdb) next
14              current_player = -1;
(gdb) p *(FILE**)0x7ffffffed9e0
$19 = (FILE *) 0x84d2c20
(gdb) next
15              scores = new int[nb_players];
(gdb) next
16              for (int playerId = 0; playerId < nb_players; playerId++)
(gdb) next
17                      scores[playerId] = 1;
(gdb) next
16              for (int playerId = 0; playerId < nb_players; playerId++)
(gdb) next
17                      scores[playerId] = 1;
(gdb) next
16              for (int playerId = 0; playerId < nb_players; playerId++)
(gdb) next
17                      scores[playerId] = 1;
(gdb) next
16              for (int playerId = 0; playerId < nb_players; playerId++)
(gdb) next
18      }
(gdb) p *(FILE**)0x7ffffffed9e0
$20 = (FILE *) 0x84d2c20

- et dès que je quitte le constructeur de la classe de base (GameManager) pour revenir dans le constructeur de la classe fille qui l'avait appelée (SimplePvPGameManager), le pointeur change par magie:

Code :

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

1
2
3
4
5
(gdb) next
SimplePvPGameManager::SimplePvPGameManager (this=0x7ffffffed9c0) at ../SimplePvP/SimplePvPGameManager.cpp:27
27              for (int i = 0; i < NB_WALLS; i++) {
(gdb) p *(FILE**)0x7ffffffed9e0
$21 = (FILE *) 0x60000000a

Sachant que ces trois parties s'enchaînent, et qu'il ne s'est donc rien passé entre, auriez-vous une idée d'où peut venir un tel problème? (Vous me redonneriez la foi dans le c++ si vous dites que c'est ma faute, mais pour le moment, c'est mieux le c# )

Merci d'avance pour votre expertise!


EDIT: Je mis ici en annexe le code correspondant au débogage, si vous souhaitez avoir une vue d'ensemble:

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
Replay::Replay(int pixels_per_unit, float width, float height)
{
	file = fopen("0.replay", "wb");
	//file.open("0.replay", ios::binary | ios::out);
	WriteUint16(pixels_per_unit);
	WriteFloat32(width);
	WriteFloat32(height);
}
 
GameManager::GameManager(int nb_players_, int pixels_per_unit, float width, float height) : 
	nb_players(nb_players_),
	replay(pixels_per_unit, width, height)
{
	unsigned int seed = time(NULL);
	srand(seed);
	std::cout << "Seed: " << seed << std::endl;
	turn = -1;
	current_player = -1;
	scores = new int[nb_players];
	for (int playerId = 0; playerId < nb_players; playerId++)
		scores[playerId] = 1;
}
 
SimplePvPGameManager::SimplePvPGameManager() : GameManager(NB_PLAYERS, 32, SIZE_MAP, SIZE_MAP)
{
    ...
}

(Dommage qu'il n'y ait pas la possibilité de créer de spoiler pour cacher les choses facultatives, et ainsi moins encombrer les posts...)

[jo_link_noir]

Caster des adresses en FILE n'a aucun sens et ta liste d'initialisation est foireuse: l'initialisation doit se faire dans le même ordre que la déclaration et les classes héritées sont avant n'importe quel membre. En plus, la moitié des membres ne sont pas initialisés, mais modifier dans le corps de la fonction, l'ensemble n'est pas cohérent.

Perso, avant d'utiliser le moindre débuggeur, je passe par les sanitizers: -fsanitize=address au minimum et éventuellement valgrind.

Pour le code en lui-même:

- score devrait être un std::vector, nb_score ne devrait pas exister
- l'utilisation de rand/srand en plein milieu d'un constructeur est en mon sens bien foireux. C'est intestable et le C++ offre des générateurs de nombre aléatoire qui ne dépendent pas d'état global.

[koala01]

Salut,

Dejà, il y a un fameux problème avec ta classe Replay, car son constructeur veut automatiquement écrire des données, du moins, si on s'en tient au nom des fonctions que le constructeur appelle

Or, si l'on sen tient au nom de ta classe (Replay) et au contexte dans lequel elle est utilisée -- le constructeur de GameManager (Quel horrible nom pour une classe !! -- on se rend compte qu'il y a beaucoup plus de chance pour que, ce qu'elle doive faire au moment de sa création soit plutot de la lecture,(appel de fonctions "ReadXXX) de manière à pouvoir ... charger des informations précédemment enregistrées.

Pire encore (si j'ai tiré les bonnes conclusions sur base du nom de cette classe "Replay") : on se rend compte que GameManager ne devrait -- a priori -- faire appel à une instance de cette classe qu'à quelques moments clés tels que:

• au lancement du programme (s'il souhaite charger automatiquement "la dernière sauvegarde")
• juste avant de quitter le programme (s'il souhaite sauvegarder automatiquement le dernier état du jeu)
• à quelques moments choisis par le joueur pour effectuer une sauvegarde "de sécurité" (des fois que, par la suite, la partie soit mal engagée, pour pouvoir reprendre "sur de bonnes bases")
• à quelques moments choisis par le joueur pour charger la sauvegarde "de sécurié" (quand, justement la partie est mal engagée, et qu'il souhaite reprendre sur "de bonne bases")

Le mieux de l'histoire étant que le point (1) s'accorde tout à fait avec le point (4) et que le point (2) s'accorde lui tout à fait avec le point (3), et qu'il ne sera donc même pas nécessaire de dupliquer du code

Mais, l'important, c'est que, en dehors de ces périodes bien particulières, il n'y a absolument aucune raison de maintenir une instance de Replay en mémoire (et donc, d'en faire une donnée membre de la classe GameManager ).

Un code proche de

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
GameManager::GameManager(/* paramètres */):/*initialisation des membres */{
    LoadGame();
    /* le reste du constructeur */
}
GameManager::~GameManager(){
    SaveGame();
}
void GameManager::LoadGame(){
    /* nettoyage éventuel des données de la partie "abandonnée" */
    Replay r{/* paramètres nécessaire*/ };
    r.Load(/* paramètres spécifiques */);
}
void GameManager::SaveGame(){
    Replay r{/* paramètres nécessaire*/ };
    r.Save(/* paramètres spécifiques */);
}

faisant parfaitement l'affaire

De même, il n'y a absolument aucune raison de vouloir déclarer un membre (de type fstream "simple", qui plus est) au niveau de la classe Replay : en ouvrant un fichier d'entrée (std::ifstream) dans la fonction Load (ou Read, comme tu préfères), qui le transmettra (sous forme de référence, bien sur) à toutes les fonctions ReadXXX spécifiques ou un fichier de sortie (std::ofstream) dans la fonction Save (ou Write, comme tu préfères) qui le transmettra (sous forme de référence, toujours) à toutes les fonctions WriteXXX spécifiques, tu n'auras plus aucun problème

[AbsoluteLogic]

Merci pour ta réponse rapide. J'avais déjà essayé Valgrind, mais il signalait seulement qu'il y avait une corruption lorsque j'essayais d'écrire dans le fichier dont le pointeur était modifié. Pas comment le pointeur a été modifié. Bref, j'ai donc maintenant utilisé -fsanitize=address comme tu me l'as suggéré (que je ne connaissais pas), et je ne sais pas quoi dire:
Premièrement, le programme compilé avec cette option fonctionne, plus de changement de pointeur. J'ai cru comprendre que cette option servait à détecter les erreurs de mémoire, pas à les corriger . Bon, je peux comprendre que le programme doit gérer la mémoire différemment avec cette option, et que de cette manière, l'erreur n'arrive plus. Au passage, j'obtenais l'erreur sous Linux, mais sous Windows, ça fonctionnait. Je n'ai pas ressenti le besoin de le dire puisque je me suis dit que c'est juste que Windows gère la mémoire d'une manière faisant que "par hasard", mes effets de bord n'ont pas fait de dégâts.
Ensuite, j'ai donc de nouveau retiré l'option sanitize pour retrouver mon erreur, et contre toute attente, ça fonctionne toujours! Là, je ne vois pas comment l'expliquer...

Bon, donc d'une certaine manière, le problème est réglé. Mais je laisse le sujet ouvert par peur que le bug revienne par la même magie qui a fait qu'il est parti. Je vais donc faire les améliorations que tu m'as suggéré, et si le bug revient avec ses améliorations, je me remanifesterai.

Donc merci beaucoup pour ton aide!

[AbsoluteLogic]

Merci pour l'attention que tu as apporté à mon sujet, mais malheureusement, tu te méprends sur le rôle de mes classes.
- GameManager est une classe "vaste" qui gère les actions générales de jeu (tour par tour), comme la gestion des tours, le score des joueurs, ou l'interface avec les joueurs. D'où son nom en français "gestionnaire de jeu",je ne vois pas mieux
- SimplePvPGameManager est une classe qui en hérite, implémentant toutes les fonctionnalités d'un jeu en particulier.
- Replay est un outil que j'ai créé permettant de créer facilement des animations, qu'il enregistre alors dans un fichier (la lecture se fait par un logiciel à part). Cela permet à SimplePvPGameManager de sauvegarder le replay du jeu courant en quelques lignes.
Désolé donc si les noms ont porté à confusion

[koala01]

- GameManager est une classe "vaste" qui gère les actions générales de jeu (tour par tour), comme la gestion des tours, le score des joueurs, ou l'interface avec les joueurs. D'où son nom en français "gestionnaire de jeu",je ne vois pas mieux

Mais, justement: cette classe est donc très loin de respecter le premier des principes SOLID (le S, mis pour SRP ou Single Responsability Principle) ce qui est déjà en soi un très gros problème de conception qui ne tardera pas à t'enfoncer sous "des tonnes de m...de"...

- SimplePvPGameManager est une classe qui en hérite, implémentant toutes les fonctionnalités d'un jeu en particulier.

Et qui ne sert donc à rien, à moins que tu ne prévoie de permettre au joueur de choisir le genre de jeu auquel il veut jouer

- Replay est un outil que j'ai créé permettant de créer facilement des animations, qu'il enregistre alors dans un fichier (la lecture se fait par un logiciel à part). Cela permet à SimplePvPGameManager de sauvegarder le replay du jeu courant en quelques lignes.

Mais donc, nous en revenons toujours au même point de base : il n'y a aucune raison que l'instance de Replay soit créée dés la création du GameManager et qu'elle soit "maintenue en vie" aussi longtemps qu'il existe : L'instance de Replay ne devrait, en tout état de cause, n'être créée que ... lorsque l'utilisateur décide se sauvegarder sa partie (ou son morceau de partie), et n'exister que ... le temps nécessaire à la création du fichier de sauvegarde.

En outre, le nom du fichier de sauvegarde ne devrait en aucun cas être codé en dur mais :

1. soit déterminé automatiquement en fonction des noms des fichiers de sauvegardes précédants et / ou sur base d'un timestamp quelconque
2. soit fournis directement par l'utilisateur

Désolé donc si les noms ont porté à confusion

Oh, tu n'as pas à être désolé, moi, je n'en soufirai absolument pas Toi, par contre...

[jo_link_noir]

Bon, donc d'une certaine manière, le problème est réglé. Mais je laisse le sujet ouvert par peur que le bug revienne par la même magie qui a fait qu'il est parti.

C'est le genre de bug qui, s'il n'est pas corrigé, va créer plein d'autres bugs de manière aléatoire et subtile. Je suis assez surpris du comportement de asan, mais ça peut arriver. Il faudrait voir plus de code pour déterminer la cause, les plus fréquentes étant dépassement de tableau et utilisation d'un pointeur invalide.

[AbsoluteLogic]

Je ne sais pas si ces remarques avaient un lien avec mes fuites mémoires, mais je me sens obligé de continuer à me défendre, dans la mesure où tu interprètes les classes sans leur contexte... que je vais te donner de ce pas :
Mon projet consiste à confronter des IAs (programmé dans divers langages) programmées par des tiers, sur différents jeux. Mon programme prend en entrée deux IAs et quelques paramètres. Il simule la confrontation et renvoie le score résultant, plus un replay (systématiquement!) pour que les programmeurs des IAs puissent voir ce qui s'est passé. Mon programme effectue la simulation sur un objet GameManager qui est une sorte d'interface. Ensuite, les IAs sont confrontés sur un jeu dont le fonctionnement est défini par la "surcharge" de GameManager, ici avec la classe fille SimplePvPGameManager. Et donc oui, je m'arrange pour qu'il puisse y avoir plusieurs jeux,et que le travail pour la création de ce nouveau jeu soit au maximum mâché (de plus, l'héritage m'assure la compabilité avec mon programme).
Donc avec ce contexte, je peux répondre:
- GameManager a pour rôle de fournir le fonctionnement de base du jeu, et des fonctions "à surcharger". Ca ne respecte peut-être pas le principe du SOLID, mais c'est ce que j'ai trouvé de mieux pour simplifier au maximum la tâche des programmeurs de jeu.
- Le replay reste ouvert pendant toute la simulation pour que lorsque une IA demande au "SimplePvPGameManager" de déplacer le personnage dans une direction, le replay enregistre cette action dans un fichier, etc... Encore une fois, la classe Replay est là pour mâcher un maximum le travail à celui qui programmera un nouveau jeu.
- Le fait que le nom soit en dur est évidemment juste pour les tests. Quand ça fonctionnera, je l'améliorerai!

[AbsoluteLogic]

C'est le genre de bug qui, s'il n'est pas corrigé, va créer plein d'autres bugs de manière aléatoire et subtile. Je suis assez surpris du comportement de asan, mais ça peut arriver. Il faudrait voir plus de code pour déterminer la cause, les plus fréquentes étant dépassement de tableau et utilisation d'un pointeur invalide.

Oui, j'espérai qu'isoler la ligne de l'anomalie permette d'éclaircir le problème, mais si ça ne suffit pas, je ne peux pas me permettre de poster tout le code source de mon projet
Et je pensais que Valgrind serait en mesure de me signaler un tel problème, mais comme je l'ai dit, il ne m'a rien signalé avant que le programme ne plante...

[koala01]

Je ne sais pas si ces remarques avaient un lien avec mes fuites mémoires, mais je me sens obligé de continuer à me défendre, dans la mesure où tu interprètes les classes sans leur contexte... que je vais te donner de ce pas :

Avec ou sans contexte, le choix des noms, lorsque l'on développe une application, a une incidence majeure sur la suite du développement.

Par exemple, lorsque tu appelle une clase XxxManager (quel que soit le terme remplacé par Xxx), tu vas très rapidement te retrouver dans une situation dans laquelle dés qu'il s'agira d'ajouter une fonctionnalité quelconque qui se rapporte (de près ou de loin) à Xxx, et que tu te posera la question (sommes toutes logique) de savoir "qui va prendre cette fonctionnalité en charge?", la réponse sera invariablement "ben, XxxManager est là pour le faire", uniquement parce que le terme "manager" (gestionnaire ou tout autre terme du même acabit ) est ... Beaucoup trop vague.

Les choses sont encore pire lorsque Xxx est un terme comme "Game", parce que le terme GameManager va englober:

• la gestion du modèle de donnée
• la gestion de la video
• la gestion du son
• la gestion des effets spéciaux
• la gestion des joueurs
• la gestion de l'IA
• la gestion des quètes
• la gestion des objets d'inventaires
• la gestion des décors
• la gestion des lumières
• et j'en oublie surement, et sans doute de meilleures

Bref, tu vas très rapidement te retrouver avec un objet tout à fait monolithique, qui dépend de toute une série d'autres classes qui dépendent elles-même du manager en question pour toute une série de (bonnes ou de mauvaises) raisons.

Et, très rapidement, il deviendra très difficile d'apporter des modifications à ce machin tout monolithique sans y passer des heures à résoudre les problèmes que "de tous petits changements" vont générer:

La première fois, ce sera la faute à "pas de bol"; la deuxième, ce ne sera qu'une "malencontreuse coïncidence". La troisième fois ... en fait, tu priera pour qu'il n'y ait pas de troisième fois (tout en sachant que tu n'aurais de toutes façons pas le choix si elle survenait) parce que les deux premières fois t'auront servi de leçons, et que tu ne pourra envisager d'apporter des modifications que la peur au ventre.

Mon projet consiste à confronter des IAs (programmé dans divers langages) programmées par des tiers, sur différents jeux. Mon programme prend en entrée deux IAs et quelques paramètres. Il simule la confrontation et renvoie le score résultant, plus un replay (systématiquement!) pour que les programmeurs des IAs puissent voir ce qui s'est passé.

C'est très bien: sur les onze points que je viens de citer, il y en aura peut-être trois dont tu n'auras pas besoin Et combien que je n'ai pas cités

Mon programme effectue la simulation sur un objet GameManager qui est une sorte d'interface.

C'est justement là qu'est tout le problème : cela ne doit pas être une classe monolithique qui prend l'ensemble en charge, mais bien une série de classes particulièrement spécialisées qui ne prennent chacune qu'un des aspect spécifique du problème.

Ensuite, les IAs sont confrontés sur un jeu dont le fonctionnement est défini par la "surcharge" de GameManager, ici avec la classe fille SimplePvPGameManager.

C'est encore pire du coup, car cela veut dire que tu vas régulièrement être bon pour redéfinir systématiquement la plus grosse partie des fonctionnements internes de ta classes!

Et donc oui, je m'arrange pour qu'il puisse y avoir plusieurs jeux,et que le travail pour la création de ce nouveau jeu soit au maximum mâché (de plus, l'héritage m'assure la compabilité avec mon programme).

Sauf que, a priori, si tu as besoin du SimplePvPGameManager, tu le connaitra comme tel, et que l'héritage en lui-même ne servira à rien A la limite, un système basé sur les templates et les politiques s'avérerait bien plus utile

Donc avec ce contexte, je peux répondre:
- GameManager a pour rôle de fournir le fonctionnement de base du jeu,

De quel jeu

Si les différents jeux sont classés en catégories bien particulières, dis toi bien que ce n'est pas sans raison: c'est parce que un jeu d'échecs n'a absolument rien à voir avec un jeu comme World Of Warcraft, ou qu'un jeu comme Mario Bross n'a rien à voir avec un jeu comme Call Of Duty!

et des fonctions "à surcharger". Ca ne respecte peut-être pas le principe du SOLID, mais c'est ce que j'ai trouvé de mieux pour simplifier au maximum la tâche des programmeurs de jeu.

Et c'est la pire erreur que tu puisse faire, car chaque fois que tu prendra des libertés par rapport à SOLID, tu te prépare une fosse remplie de pieux dans laquelle tu finira tôt ou tard par tomber.

- Le replay reste ouvert pendant toute la simulation pour que lorsque une IA demande au "SimplePvPGameManager" de déplacer le personnage dans une direction, le replay enregistre cette action dans un fichier,

Alors que l'accès au disque dur compte parmi les accès les plus lents, c'est une pure aberration, car, tôt ou tard, ton Replay finira par être complètement largé

Tant qu'à faire, pourquoi ne fonctionnerais tu pas, alors, sous la forme d'un memo ou d'un undo redo, qui te permettrait de garder une trace de tout ce qui s'est passé, et de tout enregistrer en une fois dans le fichier

etc... Encore une fois, la classe Replay est là pour mâcher un maximum le travail à celui qui programmera un nouveau jeu.

Oh, la classe Replay est encore la classe sur laquelle j'émets le moins de réserves, en dehors du fait de garder le fichier ouvert en permanence