Discussion :

[alpha_one_x86]

Bonjour,
J'ai besoin d'un coup de main pour actualiser la lib tiled sur CatchChallenger (open source, creer en 2011, on est a la 3eme version), je manque de temps, j'ai viré la lib tiled embeded de 2011 et je link sur la de debian 12, je pense avoir bien corrigé tout les problèmes de compilation, mais:

• https://github.com/alphaonex86/Catch...p2png.cpp#L823 l'activation de ces lignes fait un crash (et actuellement les bots ne sont pas sur le nouveau layer généré donc au mauvais niveau)
• https://github.com/alphaonex86/Catch...r-qtopengl.pro le client principal ne centre plus la vu sur le joueur (et il y as peu être d'autre bug graphique lié a tiled)

Si quelqu'un se motive ca aiderai fortement le projet un coup de main sur la partie tiled

[koala01]

Salut,

Peut-être devrais tu commencer par ... envisager une sérieuse refactorisation de ta fonction, car, honnêtement, avoir une fonction de plus de 190 lignes et contenant jusqu'à huit niveaux d'indentation, ca fait mal aux yeux

C'est d'autant plus vrai que l'on a des boucles dans des tests, dans des boucles, dans d'autres boucles, elles-même dans d'autres tests... Tout cela n'aide absolument en rien à se faire une idée ni de la logique ni de l'objectif de la fonction.

Si déjà, tu pouvais arriver à séparer les différents problèmes afin de les gérer séparément, je suis sur que la solution deviendrait rapidement une évidence

[alpha_one_x86]

Il n'y as pas cela dans https://github.com/alphaonex86/Catch...p2png.cpp#L823 -> exemple super simple, cour et propre.

Je ne mas rappelle pas qu'il y as cela dans le nouveau code, c'est juste des fonctions simple et courte. Et bien organisé car je savais ou j'allez.

L'ancien code du client c'est une super fonction de 50 000 lignes avec le genre de code sale que tu mentionne.

[koala01]

exemple super simple, cour et propre.

C'est sans doute beaucoup plus court et propre que ce que tu as pu avoir avant, je n'en disconvient pas, cependant, il ny a rien à faire : je ne cite pas les 190 lignes de code ou les huit niveau d'indentation "au hasard".

Je les cite parce que c'est la réalité du code que j'ai pu observer en cliquant sur le lien que tu nous as montré la ligne 823 sur laquelle pointe le lien se trouve bel et bien au huitième niveau d'indentation de la fonction de la fonction QString Map2Png::loadOtherMap(const QString &fileName), dont l'implémentation commence à la ligne 702 et s'étend jusqu'à la ligne 896.

Le le problème est, lorsqu'on regarde le code de cette fonction, qu'on se rend compte qu'il y a déjà pas mals de problème, ne serait-ce que dans les 40 premières lignes. Essayons de les passer en revue:

Or, voilà à quoi ressemble les quelques premières lignes de cette fonction:

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
    Map_full_map2png *tempMapObject=new Map_full_map2png(); 
    tempMapObject->logicalMap.group=0;
    tempMapObject->logicalMap.height=0;
    tempMapObject->logicalMap.id=0;
    tempMapObject->logicalMap.width=0;
    tempMapObject->logicalMap.teleporter=nullptr;
    tempMapObject->logicalMap.teleporter_list_size=0;
    tempMapObject->logicalMap.xmlRoot=nullptr;
    tempMapObject->objectGroup=nullptr;
    tempMapObject->tiledMap=nullptr;
    tempMapObject->tiledRender=nullptr;
    tempMapObject->x=0;
    tempMapObject->y=0;

Pour commencer, la toute première ligne de ce troçon de code va allouer dynamiquement la mémoire pour un objet de type Map+full+map2png.

Je voudrais déjà pouvoir partir du principe qu'il est cohérent d'effectuer cette allocation dynamique de la mémoire, alors que j'ai énormément de mal à m'en convaincre, et pourtant, je vais te faire confiance sur ce coup là, en considérant que c'est effectivement une nécessité.

Le seul truc, c'est que tu ne prend absolument pas en compte le fait qu'une allocation dynamique de la mémoire est toujours susceptible d'échouer, et que, si cela arrive, une exception (de type std::bad_alloc) peut être lancée ... ou non.

Si cette exception est effectivement lancée, tu évites la catastrophe car tu as la certitude que la suite du code ne sera jamais exécutée, et donc, que tu n'essayera pas d'accéder aux éléments interne d'une structure ... qui n'existe pas en mémoire.

Par contre, si l'exception n'est pas lancée (ce qui est une possibilité plus que vraisemblable), tempMapObject va pointer sur ... NULL / nullptr, ce qui fait que chacune des douze instructions -- ainsi que toutes les tentatives d'accès à tempMapObject qui suivent (et il y en a quelques unes) -- provoquera un comportement indéfini. Sans doute un plantage pur et simple du programme...

Les lignes 724 à 730 s'assurent que que tempMapObject->tiledMap existe et détruit tempMapObject avant d'abandonner la procédure si ce n'est pas le cas, ce qui est déjà pas si mal... Sauf qu'il n'y a aucune certitude que tempMapObject existe à la base

Par chance, si une exception est effectivement lancée lors de la tentative d'allocation de tempMapObject, rien de tout cela ne risque d'arriver (par contre, si l'exception n'est pas gérée en temps et en heure, ton programme finira par planter )

Les vrais problèmes vont arriver plus tard. Car on trouve une autre allocation dynamique à la ligne 733: Map_full *tempMapObjectFull=new Map_full(); et, pour faire bonne mesure, une troisième à la ligne 737 [B]tempMapObjectFull->objectGroup = new Tiled::ObjectGroup("text_Dyna_management",0,0);.
Comme toute bonne allocation dynamique qui se respecte, les deux que voici risquent d'échouer pour une raison ou une autre, et ce risque n'est absolument pas pris en compte dans ton code...

Car, au mieux, new lancera également une exception de type std::bad_alloc dans ces deux circonstances, ce qui empêchera le reste du code de s'exécuter. Sauf que:

• si l'exception survient à la ligne 733, tempMapObject ne sera jamais libéré (à moins qu'il ne soit intégré au système parent/enfant de Qt, ce dont je n'ai aucune preuve à la lecture du code), ce qui provoquera une fuite mémoire
• si l'exception survient à la ligne 737, non seulement tempMapObject ne sera jamais libéré, mais il en sera de même pour tempMapObjectFull

Alors, je ne sais pas si tu t'en rends compte, mais là, ce que je suis occupé à te dire, c'est que sur les quarante premières lignes de code de cette fonction, il y en a royalement quatre qui ne risquent pas de provoquer une catastrophe! Cela veut dire, que, sans commencer à fouiller dans le code pour en savoir un peu plus au sujet des types Map_full_map2png ou Map_full, "l'indice de confiance" que je peux avoir dans ce code, la garantie que "tout se passera comme il se doit" à l'exécution de ce code ne dépasse pas ... les 10%. C'est, purement et simplement inacceptable!


Mais, bon, je suis bonne poire, et je vais encore une fois essayer de me convaincre que les types Map_full_map2png et Map_full sont correctement intégrés au système parent / enfant de Qt. Je ne suis pas tout à fait convaincu, en ne m'en tenant qu'au code de cette fonction, que ce soit la meilleure des choses, cependant, je vais me contraindre à partir du principe que c'est une solution correcte, raisonnable et cohérente.

Je vais donc changer "mon fusil d'épaule" pour la suite de mon analyse, et simplement te donner quelques conseils "conceptuels".

Le premier que je vais te donner est que, lorsque tu crées une donnée -- sur ce soit de manière statique, sous la forme de MonType obj; ou de manière dynamique sous la forme de MonType * ptr = new MonType; -- il faut absolument que l'objet soit directement initialisé de manière cohérente.

C'est à dire que tu dois pouvoir l'utiliser sans te poser la question de savoir si "telle ou telle donnée interne a une valeur cohérente".

La raison est simple: plus tu augmentra le nombre d'étapes à effectuer pour placer ton objet dans un état "cohérent" après la création, plus tu prendra le risque que celui qui crée l'objet (et qui pourrait être toi, au demeurant) oublie l'une des étapes intermédiaires et provoque une catastrophe.

Reprenons donc les quelques premières lignes de la fonction :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
    Map_full_map2png *tempMapObject=new Map_full_map2png(); 
    tempMapObject->logicalMap.group=0;
    tempMapObject->logicalMap.height=0;
    tempMapObject->logicalMap.id=0;
    tempMapObject->logicalMap.width=0;
    tempMapObject->logicalMap.teleporter=nullptr;
    tempMapObject->logicalMap.teleporter_list_size=0;
    tempMapObject->logicalMap.xmlRoot=nullptr;
    tempMapObject->objectGroup=nullptr;
    tempMapObject->tiledMap=nullptr;
    tempMapObject->tiledRender=nullptr;
    tempMapObject->x=0;
    tempMapObject->y=0;

La création effective de l'objet de type Map_full_map2png a lieu à la ligne 1 de ce "snipset". Les douze lignes qui suivent sont ... autant d'étapes requises pour t'assurer que l'objet est "correctement initialisé", et donc, autant d'étapes qui risquent d'être oubliées.

Ce code me permet de me faire une idée "relativement précise" (à condition que tu n'aie pas oublié d'initialiser queqlue chose) du contenu de cette classe / structure, et de te dire qu'elle est composée

• d'une "carte logique" (logicaMap)
• d'un pointeur sur un groupe d'objets (objectGroup)
• d'un pointeur sur une carte de tiles (tiledMap)
• d'un pointeur sur un élément de rendu des tiles (tiledRender)
• d'une position qui lui est propre (x et y)

Hé bien, étant donné l'usage que tu fais de la variable tempMapOject, tous ces éléments devraient être initilalisés directement lorsque tu crées cette variable.
Cela rendrait ton code "plus facile à comprendre", en évitant le "bruit" occasionné par la définition des valeurs associées, mais, surtout, cela t'assurerait qu'aucun oubli ne risque de venir "foutre le boxon" dans la suite de la logique.

De la même manière, toute la logique que l'on retrouve de la ligne 733 à la ligne 740 a pour but -- si j'ai bien compris -- d'initialiser un objet de type Map_full qui aura lui-même pour objectif de remplir la map de tiles de tempMapObject.

Hé bien, tout cela devrait se faire au niveau ... du constructeur de Map_full_map2png pour plusieurs raisons:

D'abord, cela ne sert à rien de mettre un pointeur à nullptr si c'est pour le définir directement après : tu fais deux fois le travail alors que, si tu avais "un tout petit peu attendu", tu aurais pu ne le faire qu'une seule fois...

Ensuite, si quelque chose foire effectivement dans toute la logique qui va permettre de "remplire" tiledMap, c'est l'existence même de ton objet tempMapObject que tu remets en cause. Il faut donc que l'existence de cet objet soit "subordonnée" à toute la logique que tu décris entre les lignes 717 et 744.

Enfin, il y a la loi de Demeter qui nous dit

Si un objet a de type A (ici, cela correspond à tempMapObject qui est de type "pointeur sur Map_full_map2png") manipule en interne un objet b de type B (qui correspond ici --entre autres -- à tempMapObjectFull qui est de type "pointeur sur Map_full"), l'utilisateur de a (qui est ici la fonction membre loadOtherMap que nous définissons pour l'instant) ne doit pas avoir à connaitre le type B (le type "Map_full", en l'occurrence) pour manipuler son a ( tempMapObject, en l'occurrence)

Or le meilleur moyen d'arriver à ce résultat est de faire en sorte que ce soit ... le type Map_full_map2png qui s'occupe de toutes les manipulation relatives au type Map_full.

Et cela ne peut avoir que des avantages, car, d'une part cela signifie que c'est le type Map_full_map2png qui va décider de faire appel au type Map_full, avec pour conséquence que tu pourras décider de choisir une autre solution pour obtenir un résultat similaire si le besoin s'en faisait sentir.

D'autre part, le fait d'extraire cette logique signifie que cela te donne l'occasion de l'utiliser "sans te poser de quetion" dans d'autres circonstances (n'ayant rien à voir avec le type Map2Png ou avec le type Map_full_map2png) si le besoin s'en faisait sentir.

En outre, cela te permettrait de mettre en place une politique de tests unitaires sur le comportement associé à cette logique, ce qui n'est jamais une mauvaise chose.

Et, enfin, cela aurait pour résultat de ... réduire la taille de ta fonction qui passerait, du coup, de 190 lignes à 150. Ce ne serait pas encore parfait, mais ce serait déjà vachement mieux (pour information, même si je n'aime pas vraiment donner des chiffres arbitraires de la sorte, on considère généralement que la taille une fonction ne devrait "autant que se faire" pas dépasser les 50 lignes environs... nous en serions quand même à trois fois autant )

Le deuxième conseil majeur que je vais te donner est issu de ce que l'on appelle "l'Xtrem programming" (la programmation extrême), meme si on le retrouve sous différentes formes dans d'autres approches: DRY : Don't Repeat Yourself (ou, si tu es plus à l'aise en francais: ne te répète pas).

L'idée de ce principe est que, si tu te trouves, à un moment donné, à devoir copier / coller une logique, si tu es dans une situation dans laquelle la même logique doit être suivie, potentiellement avec des données de bases différentes, c'et qu'il est plus qu'intéressant d'extraire cette logique sous la forme d'une fonction spécifique.

Cela n'a l'air de rien, mais c'est exactement le genre de problème auquel nous sommes confrontés aux lignes 869 à 876 (entre autres): Nous appliquons quatre fois exactement la même logique, un nom de fichier à utiliser comme unique point de variation.

Dés lors, pourquoi ne pas faire en sorte que ces deux lignes respective (si le map_loader.map_to_send.border.<cote>.fileName n'est pas vide, mettre à jour tempMapObject->logicalMap.border_semi.bottom.fileName) conjointement dans une fonction qui appliquera cette logique sur base d'un coté unique, puis d'utiliser cette fonction pour en créer une deuxième qui s'occuperait de tester les quatres cotés intéressés

Cela te permettrait, au niveau de la fonction loadOtherMap, de faire exactement la même chose en appelant une fonction proche de updateAllFilenames(map_loader.map_to_send.border, tempMapObject->logicalMap.border_semi).

D'ailleurs, cette fonction mettant les noms des quatre cotés à jour pourrait tout aussi bien être une fonction membre du type correspondant à map_loader.map_to_send et qui prendrait une forme proche de updataAllFilenames(tempMapObject->logicalMap.border_semi)|/c], ce qui apporterait le même genre d'avantages que ceux que j'ai expliqués plus haut, et réduirait encore la taille de ta fonciton de 7 lignes.

D'ailleurs -- car je réfléchis aux problèmes "en direct" (au moment d'écrire cette intervention) -- on se rend compte que l'existance finale de l'objet tempMapObject (qui est le premier élément créé dans la fonction) dépend également du bon fonctionnement de toute la logique que l'on retrouve entre les lignes 848 et 876. Or, toute cette partie de la logique est à mettre en relation avec l'objet map_loader qui est de type Map_loader.

Et comme une même cause aura toujours les mêmes effets, le conseil que je t'ai donné plus haut de faire en sorte que ce soit le type Map_full_map2png qui s'occupe des manipulations relatives au type Map_full est on ne peut plus raisonnable en ce qui concerne le type Map_loader : toutes les manipulations relatives au type Map_loader devraient être prises en charge par le type Map_full_map2png et, sans doute, au moment de sa création.

En outre, comme il y a a toute une logique de préparation basée sur tempMapObject->tiledMap->layerCount() intervenant entre les deux aspects envisagés jusqu'à présent, il semble bien évident que cette logique soit elle aussi prise en charge par le processus de création de l'objet tempMapObject.

Enfin, les lignes 878 à 892 vont, de toute évidence, initialiser le morteur de rendu. Note que l'existence de tempMapObject ne prend tout son sens si il y a effectivement un moteur de rendu pour l'afficher, et que, si -- pour une raison ou une autre -- le moteur de rendu venait à ne pas s'initialiser, le programme ne pourrait de toutes facons pas faire grand chose à part ... planter (car jouer sans avoir de rendu est sans doute difficile ).

Il n'est donc "pas trop mal" de logger le problème (bien que tu aies commenté le code équivalent), mais ce n'est pas suffisant pour assurer une exécution correcte : il faut clairement prendre des dispositions supplémentaires pour t'assurer que l'on n'essayera jamais d'accéder à un moteur de rendu qui n'existe pas

Cependant, il est important de noter que nous avons déjà bel et bien identifié cinq responsabilités distinctes:

• Allouer la mémoire pour un objet de type Map_full_map2png,
• initialiser une partie de cet objet grâce à un objet de type Map_full
• effectuer une série de manipulations basées sur la valeur de tempMapObject->tiledMap->layerCount()
• initialiser une série de donnée grâce à un objet de type Map_loader
• initialiser le moteur de rendu

et que les quatre dernières responsabilités ne sont que "des aides" destinées à nous permettre d'atteindre l'objectif de la première.

Or, lorsqu'une fonction se retrouve à devoir faire cinq chose différentes, c'est qu'elle doit en faire quatre de trop...

Chacune de ces responsabilités mérite donc de prendre la forme d'une fonction "spécialisée" dans le but d'obtenir le résultat souhaité

Au final, il semblerait que la fonction QString Map2Png::loadOtherMap(const QString &fileName) devrait en réalité se limiter à quelque chose comme

Code :

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

1
2
3
4
5
6
7
8
9
10
QString Map2Png::loadOtherMap(const QString &fileName){
    Map_full_map2png *tempMapObject=new Map_full_map2png(filename); // toute l'initialisation de cette classe se passe ici
    /* Il faut voir si new va lancer une exception ou non
    * si ce n'est pas le cas et qu'elle renvoie nullptr, prévoir que cela puisse mal se passer
    */
   if(!tempMapObject)
       throw std::runtime_error("unable to load new map");
    other_map[tempMapObject->resolvedFileName]=tempMapObject;
    return tempMapObject->resolvedFileName;
}

avec un constructeur pour le type Map_full_map2png ressemblant à quelque chose comme

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
Map_full_map2png::Map_full_map2png(QString filename):
    /* utiliser la liste d'initialisation pour logicalMap, objectGroup, tiledMap, tiledRender x, et y */{
    QFileInfo fileInformations(fileName);
    resolvedFileName=fileInformations.absoluteFilePath(); // resolvedFileName devient une donnée membre de la classe
    if(! readMap())  // lignes 722 à 744
        throw std::runtime_errir("unable to read map");
    if(!loadLayers()) // lignes 746 à 847, sans doute subdivisée en plusieurs fonctions spécifiques
        throw std::runtime_error("unable to load layers");
    if(!load_neighbors()) // lignes 848 à 876
        throw std::runtime_error("unable to load neighbors");
    if(!setUpRender()) //lignes 879 à 889
        throw std::runtime_error("unable to setting up render");
}

(note au passage que les exceptions pourraient être lancées par les fonctions appelées; je ne les présente ici que pour bien montrer qu'une exception est lancée à chaque fois que "quelque chose se passe mal" lorsque l'on essaye de charger la map )

Et, de même, les lignes 746 à 847 seraient elle-mêmes subdivisées en différentes fonctions su base de l'indentation actuelle, ce qui te permettrait de savoir exactement à quelle situation correspond le fameux test [/c]if(tempMapObject->objectGroup!=nullptr)[/c], et donc, ce qu'il y a effectivement lieu de faire dans ce cas

Je tiens à préciser que je n'ai en aucun cas été voir comment les autres fonctions se présentaient, et que je n'ai même pas essayé de comprendre ce qui se passait dans la fonction QString Map2Png::loadOtherMap(const QString &fileName) de manière plus approfondie que ce que le code me permettait d'en déduire.

La seule chose que j'aie faite, c'est d'essayer de te convaincre par l'exemple que la toute première qualité d'un code, avant même de faire ce que tu attends de sa part, est d'être facile à lire et à comprendre et que le meilleur moyen pour y arriver est d'avoir des fonctions "aussi courtes que possibles" (maximum 50 lignes environs) qui ne s'occupent que d'une seule chose à la fois.

Si tu arrives à refactoriser ton code pour appliquer cette "simple" règle de manière systématique, je peux t'assurer que tu te facilitera énormément la vie, que ce soit maintenant ou dans le futur