Une class Sprite

Bonjour,

J'ai programmé une class Sprite pour gérer plus simplement les feuilles de sprite. La class fonctionne, et j'aimerai avoir votre avis pour la perfectionner si besoin est et pour savoir si ce que j'ai fait est correct (et pourquoi pas la mettre en source par la suite).

Voila le code:
- sprite.h
- sprite.cpp

Un modèle de fichier de configuration:
- sprites/spr_perso.txt


Explications : Les grandes lignes
La class contient :
- une surface SDL (sur laquelle on va charger l'image par la suite)
- un pointeur vers une couleur transparente
- un ensemble de couple string/SDL_Rect (autrement dit une map de STL)

Ainsi une animation est un couple "nom de l'animation" de type string et un ensemble de coordonnées (x, y, w et h) stockée dans une structure SDL_Rect. J'ai codé les principaux accèsseurs (même s'il ne serviront presque jamais) tandis que le constructeur par recopie est vide (je ne pense pas qu'il soit possible à faire à cause du pointeur sur la SDL_Surface*, quoiqu'il en soit je ne pense pas qu'il soit nécessaire de recopier une feuille de sprite mais cela risque d'être problématique pour l'utilisation de feuille de sprite en tant qu'argument de fonction).

Les méthodes principales:
- charger(string fichier) : charge le fichier image sur la surface
- definir_coul_trans(r,v,b) : définit une couleur transparente
- ajouter() : permet d'ajouter un couple animation/coordonnées. La fonction est surchargée pour permettre une souplesse d'utilisation.
- blitter(surface, nom_anim, x, y) : blitte un sprite sur la surface


Class FeuilleSprite : Extension de la class
J'ai codé aussi une class FeuilleSprite qui hérite de Sprite.
Je l'ai ajouté pour pouvoir gérer plus simplement la définition des animations; ainsi on va aller charger les infos du sprite et les animations (nom et coordonnées) dans un fichier texte externe (c'est là qu'intervient le sprites/spr_perso.txt). J'ai fait en sorte de pouvoir commenter le fichier texte.

La class contient une liste chainée d'erreur (au format string) c'est pourquoi j'ai ajouté deux accèsseurs pour lire les messages par la suite (et un alias de l'iterateur). Ce système de message d'erreur est assez lourd à gérer, si vous avez une meilleur idée je suis preneur !

Sinon la class ne contient qu'une méthode en plus: charger_feuille(string chemin) qui permet d'aller lire le fichier .txt pour en extraire les informations et ensuite définir entièrement le Sprite.



Merci à tous ceux qui auront pris la peine de lire mes sources :)

Bonjour voici mes remarques :

Citation:

---

le constructeur par recopie est vide (je ne pense pas qu'il soit possible à faire à cause du pointeur sur la SDL_Surface*, quoiqu'il en soit je ne pense pas qu'il soit nécessaire de recopier une feuille de sprite mais cela risque d'être problématique pour l'utilisation de feuille de sprite en tant qu'argument de fonction).

---

- Si on pourrait créer une nouvelle surface et faire la copie mais c'est un gaspillage mémoire.
- Copier simplement le pointeur en ajoutant un booléen pour savoir qui doit libérer la feuille de style -> dangereux, faut que la désallocation se fasse dans le bon ordre.
- Ajouter un gestionnaire de ressources qui centralise le tout et qui gére l'allocation/désallocation des ressources...



- Je ne vois pas pourquoi mettre la couleur transparente en tant que membre de la classe Sprite ou même pourquoi la mettre en pointeur...
Du coup, ta fonction get_coul_trans est dangereuse, tu ne testes pas si ton pointeur est NULL.

- Pas de test après SDL_DisplayFormat.

Code:

---

1 2 3 4 5 6 7 8

// Indique si on a définit une couleur transparente bool Sprite :: a_coul_trans() {   if( this->couleur_clee != NULL) return true; else return false; }

---

Peut être simplifié en :

Code:

---

1 2 3 4 5 6

// Indique si on a définit une couleur transparente bool Sprite :: a_coul_trans() {   return (this->couleur_clee != NULL); }

---

ou encore :

Code:

---

1 2 3 4 5 6

// Indique si on a définit une couleur transparente bool Sprite :: a_coul_trans() {   return (couleur_clee != NULL); }

---

Code:

---

1 2 3

// On mémorise la couleur this->couleur_clee = new SDL_Color;

---

Sympa la fuite de mémoire :wink:

Personnellement, je ne vois pas pourquoi stocker cette valeur, cela ne devrait regarder personne quelle est la couleur transparente sauf la fonction de blit. Or, on stocke la valeur de la couleur transparente grâce à la fonction SDL_SetColorKey...

Code:

---

1 2 3

if( iter != this->coord.end() ) return true; else return false;

---

peut devenir :

Code:

---

1 2	return (iter != coord.end());

---

Code:

---

1 2 3 4 5

// Supprimer un couple identifiant/coordonnée bool Sprite :: supprimer(string id) {   if( !this->existe(id) ) return false;

---

Pas la peine de tester l'existence de l'élément, au pire la suppression ne fera rien.

Code:

---

1 2	nbanim = atoi( s.c_str() );

---

beurk atoi, déjà en C on n'utilise pas atoi alors en C++...
http://c.developpez.com/faq/cpp/?pag...RINGS_strtonum

Citation:

---

La class contient une liste chainée d'erreur (au format string) c'est pourquoi j'ai ajouté deux accèsseurs pour lire les messages par la suite (et un alias de l'iterateur). Ce système de message d'erreur est assez lourd à gérer, si vous avez une meilleur idée je suis preneur !

---

Je ne vois pas vraiment mieux. J'ai une tendance de sortir le plus proprement possible à la première erreur que je vois pas à faire un iterateur comme toi. Question de goût je pense.

Attention tout de même :
- Ta solution est sympathique mais est-elle vriament pratique. Lorsqu'on va vouloir gérer les animations, comment passer d'une animation à l'autre ? S'il faut recréer la chaîne clé à chaque image, cela ne va pas être pratique.

- Limiter le nombre de recherche dans ta map. Ta fonction existe est pratique dans le code de debuggage mais généralement, ce sera plus intelligent de simplement chercher l'élément est le comparer à l'élément end...

Jc

Si on veut autoriser la copie, le mieux pour ce genre de classes est de séparer en deux : une classe interne "implémentation", et une classe publique qui l'encapsule avec un pointeur intelligent à base de comptage de référence.

Si la copie n'a aucun sens ou est déconseillée, il faut l'interdire en mettant le constructeur par copie et l'opérateur d'affectation en accès privé et ne pas les implémenter.

Autre chose : je trouve l'utilisation de l'héritage pas très appropriée pour l'objectif recherché. Mieux vaut faire une fonction non membre qui prend en paramètre le nom du fichier et qui renvoie un sprite. Ou au pire une classe s'il y a des traitements complexes à faire et des données à stocker, mais pas dérivée de sprite.

Tout d'abord, merci de vos réponse.
Attaquons le vif du sujet !

Citation:

---

Envoyé par fearyourself

Si on pourrait créer une nouvelle surface et faire la copie mais c'est un gaspillage mémoire.

---

Je retiendrais cette solution comme la meilleur. Quoiqu'il en soit, la recopie d'un objet de type Sprite n'a vraiment presque aucun interêt de toute façon, en réalité seulement lors de l'appel à une fonction prenant un Sprite en paramètre quoi.

Je ne voie pas comment réaliser cette solution, peut-être faire une copie de surface ?
Puisque nous utilisons des SDL_Surface*, faire sprite1.surface = sprite2.surface reviendrait à copier le pointeur (et donc la libération d'une surface de mémoire renderait le second objet non-intègre).



Pour ce qui est de la couleur transparente, en réalité je réalise que c'est un mauvais choix de la conserver. A la base je l'ai ajouté dans la class pour que l'on puisse, à tout moment, connaitre la couleur de la feuille ce qui n'est en réalité pas nécessaire. J'avais aussi penser pouvoir changer de couleur et de feuille, mais à ce compte là autant créer un nouvel objet... il n'est pas pas du tout nécessaire de la stocker en réalité.

En revanche, comment supprimer la couleur transparente d'une surface ?
Il y a-t-il une fonction inverse à SDL_SetColorKey() ?



Je vais améliorer un peu le code avec les simplifications, et surtout ne plus utiliser de atoi() et itoa() car apparement c'est vraiment beurk, erf !



Je pense aussi que la liste chainée d'erreur est un bon modèle, puisque ca permet de garder les erreurs dans l'ordre sur lequelle elles sont arrivées et d'en avoir plusieur.
Comme tu l'as dit c'est une question de gout après, mais si une erreur arrive puisqu'il est impossible de définir la couleur transparente (ce qui n'altère en rien le déroulement du programme) je préfère continuer. Et puis, l'utilisateur verra bien qu'il y a un problème !

Le gros problème c'est que le traitement derrière est assez lourd... utiliser un itérateur déjà, puis ajouter deux accèsseurs c'est pas top.

Citation:

---

Ta solution est sympathique mais est-elle vriament pratique. Lorsqu'on va vouloir gérer les animations, comment passer d'une animation à l'autre ? S'il faut recréer la chaîne clé à chaque image, cela ne va pas être pratique.

---

A la base j'ai créer cette classe pour simplifier un peu ma class de type Personnage justement. Je préférais manier des noms d'animations en string, plutot que de constantes provenant de listes enum{} ou autres.
Lors d'une animation (plusieurs images) l'utilisation ressemble à:

Code:

---

1 2 3 4 5 6 7 8

switch( /* vecteur vitesse */ ) { case ...: anim = "haut" case ...: anim = "bas" }   if( /* gestion des animations par exemple */ ) { anim += framecounter; }

---

Ainsi je retombe sur le nom de l'animation haut0, haut1,... je trouve cela assez parlant à la relecture du code. Mais l'utilisation de string est parfois un peu plus lourde à gérer.

Citation:

---

Envoyé par fearyourself

Limiter le nombre de recherche dans ta map. Ta fonction existe est pratique dans le code de debuggage mais généralement, ce sera plus intelligent de simplement chercher l'élément est le comparer à l'élément end...

---

Je n'ai pas bien compris sur ce point.
Puis la fonction existe n'apparait que très peu non ?

Citation:

---

Envoyé par Laurent Gomila

Si la copie n'a aucun sens ou est déconseillée, il faut l'interdire en mettant le constructeur par copie et l'opérateur d'affectation en accès privé et ne pas les implémenter.

---

Mais si un developpeur veut utiliser ma class et créer une fonction qui la prends en paramètre, il y aura forcement une copie non? Du coup si je ne l'autorise pas, il sera impossible de passer un Sprite en argument (à moins de le passer par référence avec &), je me trompe ?

Citation:

---

Envoyé par Laurent Gomila

Autre chose : je trouve l'utilisation de l'héritage pas très appropriée pour l'objectif recherché. Mieux vaut faire une fonction non membre qui prend en paramètre le nom du fichier et qui renvoie un sprite. Ou au pire une classe s'il y a des traitements complexes à faire et des données à stocker, mais pas dérivée de sprite.

---

J'ai fait une class héritée FeuilleSprite pour séparer en deux les deux class; la première plus générale et la seconde plus ciblée. En effet, dans la seconde la structure du fichier que l'on va charger est fixe et déterminée et à moins de mettre des méthodes pour gérer le format du fichier (ce qui peut sembler assez compliqué!) j'ai préféré faire une autre class, utilisant un schéma de donnée fixe (nom de l'image;couleur transparentes;animations).

Citation:

---

Envoyé par FabaCoeur

Je ne voie pas comment réaliser cette solution, peut-être faire une copie de surface ?

---

Il faut créer une surface avec SDL_CreateRGBSurfaceFrom de même taille et copier les pixels... Par contre, pour vraiment faire une copie, il faudrait utiliser la couleur pour la transparence.

Citation:

---

Pour ce qui est de la couleur transparente, en réalité je réalise que c'est un mauvais choix de la conserver. A la base je l'ai ajouté dans la class pour que l'on puisse, à tout moment, connaitre la couleur de la feuille ce qui n'est en réalité pas nécessaire. J'avais aussi penser pouvoir changer de couleur et de feuille, mais à ce compte là autant créer un nouvel objet... il n'est pas pas du tout nécessaire de la stocker en réalité.

---

Donc en effet, si on veut faire une copie, faudrait garder la couleur de la transparence.

Citation:

---

En revanche, comment supprimer la couleur transparente d'une surface ?
Il y a-t-il une fonction inverse à SDL_SetColorKey() ?

---

Il suffit de passer 0 au deuxième paramètre.

Citation:

---

Je pense aussi que la liste chainée d'erreur est un bon modèle, puisque ca permet de garder les erreurs dans l'ordre sur lequelle elles sont arrivées et d'en avoir plusieur.
Comme tu l'as dit c'est une question de gout après, mais si une erreur arrive puisqu'il est impossible de définir la couleur transparente (ce qui n'altère en rien le déroulement du programme) je préfère continuer. Et puis, l'utilisateur verra bien qu'il y a un problème !

Le gros problème c'est que le traitement derrière est assez lourd... utiliser un itérateur déjà, puis ajouter deux accèsseurs c'est pas top.

---

Suffit de tout internaliser pour ne pas laisser l'utilisateur s'en servir et laisser juste une fonction pour afficher les erreurs.

Citation:

---

A la base j'ai créer cette classe pour simplifier un peu ma class de type Personnage justement. Je préférais manier des noms d'animations en string, plutot que de constantes provenant de listes enum{} ou autres.
Lors d'une animation (plusieurs images) l'utilisation ressemble à:

Code:

---

1 2 3 4 5 6 7 8

switch( /* vecteur vitesse */ ) { case ...: anim = "haut" case ...: anim = "bas" }   if( /* gestion des animations par exemple */ ) { anim += framecounter; }

---

Ainsi je retombe sur le nom de l'animation haut0, haut1,... je trouve cela assez parlant à la relecture du code. Mais l'utilisation de string est parfois un peu plus lourde à gérer.

---

Oui ce sera largement plus long qu'utiliser directement des entiers. Si tu veux garder cette solution, utilise au moins un tableau directement qui donnera le string correspondant.

Citation:

---

Je n'ai pas bien compris sur ce point.
Puis la fonction existe n'apparait que très peu non ?

---

Apparaît très peu mais dans des fonctions qui seront appelées souvent.

Citation:

---

Mais si un developpeur veut utiliser ma class et créer une fonction qui la prends en paramètre, il y aura forcement une copie non? Du coup si je ne l'autorise pas, il sera impossible de passer un Sprite en argument (à moins de le passer par référence avec &), je me trompe ?

---

Justement, il ne faut jamais passer directement la classe mais passer une référence ou le pointeur.

Jc

Citation:

---

Mais si un developpeur veut utiliser ma class et créer une fonction qui la prends en paramètre, il y aura forcement une copie non? Du coup si je ne l'autorise pas, il sera impossible de passer un Sprite en argument (à moins de le passer par référence avec &), je me trompe ?

---

D'où l'intérêt d'avoir interdit la copie, comme ça il sera obligé de passer par référence ou pointeur comme dit par JC, au lieu de faire plein de copies lourdingues silencieusement ;)

Citation:

---

J'ai fait une class héritée FeuilleSprite pour séparer en deux les deux class; la première plus générale et la seconde plus ciblée. En effet, dans la seconde la structure du fichier que l'on va charger est fixe et déterminée et à moins de mettre des méthodes pour gérer le format du fichier (ce qui peut sembler assez compliqué!) j'ai préféré faire une autre class, utilisant un schéma de donnée fixe (nom de l'image;couleur transparentes;animations).

---

Tout ce que fait ta classe dérivée c'est créer un sprite d'une manière particulière. Mais une fois créé, c'est juste un sprite. Il n'y a rien qui justifie que l'instance se trimballe en tant que FeuilleSprite tout au long du programme.

Je vais ajouter la possibilité de supprimer la couleur transparente, mais la stocker ne servirait en effet à rien (mis à part pour la recopie que je vais interdire).

La recopie n'est pas utile du tout dans ce cas, le passage par référence semblant ne poser aucun problème. Pour rendre la copie interdite, je vais donc mettre en privé:
- le constructeur par recopie
- l'opérateur d'affectation operator+()
Et laisser les blocs vide si j'ai bien compris.

Citation:

---

Envoyé par fearyourself

Suffit de tout internaliser pour ne pas laisser l'utilisateur s'en servir et laisser juste une fonction pour afficher les erreurs.

---

Cela me semble une mauvaise solution; en effet si on fait comme ça on perd un peu la notion de "donnée à l'état brut". Si avec la même class je vais avoir un traitement personnalisé des erreurs, cela sera alors impossible. De plus avec cette solution il va falloir choisir un flux (cerr par exemple) et il ne sera pas possible de changer.
C'est pourquoi j'ai préféré la liste chainée. Le prix à payer, une utilisation assez lourde.

Citation:

---

Envoyé par fearyourself

Oui ce sera largement plus long qu'utiliser directement des entiers. Si tu veux garder cette solution, utilise au moins un tableau directement qui donnera le string correspondant.

---

Je n'ai pas compris cette réplique! Quel tableau veut-tu que je mette en place? Qui fasse correspondre un nombre (par exemple 2) avec un string (par exemple "haut2")? Au final je perds un peu l'interêt que je voyais à la classe en premier: avoir du sens à la relecture du code.

Puis faire un tableau du style:
string haut[7] = {"haut0", "haut1", ... , "haut7"};
C'est un peu redondant et ca alourdi le code je trouve.

Citation:

---

Envoyé par fearyourself

Apparaît très peu mais dans des fonctions qui seront appelées souvent.

---

Est-ce vraiment important ?
Il ne s'agit après tout que d'un test if et d'une ligne de recherche dans la map.
Quoique sur le coup je me rend compte que je ne sais pas du tout si une recherche dans une map (de taille faible, inférieur à 50 éléments grand max) prends autant de temps/ressource que cela.

Citation:

---

Et laisser les blocs vide si j'ai bien compris.

---

Non. Ne pas définir de bloc du tout, de façon à ce que toute tentative d'utilisation génère une erreur à l'édition de liens (pour les petites classes futées qui auraient passé l'erreur de compilation induite par l'accès privé).

Citation:

---

Quoique sur le coup je me rend compte que je ne sais pas du tout si une recherche dans une map (de taille faible, inférieur à 50 éléments grand max) prends autant de temps/ressource que cela.

---

La recherche est en temps logarithmique, ce qui est assez faible.

Citation:

---

Envoyé par Laurent Gomila

Non. Ne pas définir de bloc du tout, de façon à ce que toute tentative d'utilisation génère une erreur à l'édition de liens (pour les petites classes futées qui auraient passé l'erreur de compilation induite par l'accès privé).

---

J'ai compris l'idée, en gros il s'agit de définir le prototype dans le .h (et dans le .cpp aussi?) mais ne pas l'implémenter du tout histoire que son utilisation provoque une erreur.

Citation:

---

Envoyé par Laurent Gomila

La recherche est en temps logarithmique, ce qui est assez faible.

---

Donc je ne voie pas en quoi l'utilisation récurrente de la méthode existe() pose problème !

Citation:

---

J'ai compris l'idée, en gros il s'agit de définir le prototype dans le .h (et dans le .cpp aussi?) mais ne pas l'implémenter du tout histoire que son utilisation provoque une erreur.

---

Juste dans le .h ;)

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12

class NonCopyable { protected :   NonCopyable() {}   private :   NonCopyable(const NonCopyable&);   NonCopyable& operator =(const NonCopyable&); };

---

D'ailleurs il est recommandé d'utiliser ce genre de classe, ça permet de définir une classe non copiable très rapidement (par héritage) sans avoir à faire ces manips à chaque fois.

Code:

---

1 2 3 4

class Personnage : /* private */ NonCopyable {   };

---

Citation:

---

Donc je ne voie pas en quoi l'utilisation récurrente de la méthode existe() pose problème !

---

Ca ne pose pas réellement de problème ici, mais puisque tu utilises l'élément juste après, ce serait plus logique d'aller le chercher directement. Là tu vas faire deux parcours de ta map pour aller chercher deux fois l'élément.

Après avoir pris en compte toutes les remarques, conseils et suggestions au long de ce topic j'ai repris tout le code de la class et voila la nouvelle version de la class:

> sprite2.h
> sprite2.cpp
> spr_perso.txt

Si vous avez encore des remarques, n'hésitez pas !
Pensez-vous que cette classe pourrait faire l'objet d'une source ou pas ?


Chose que je n'ai pas comprise, si je met le constructeur par recopie et l'opérateur = en partie privée, je suis d'accord que la recopie devient impossible. Cependant, comme on ne l'implémente pas dans le .cpp, qu'est ce qui empêche les developpeurs à l'implémenter eux-même? Pourquoi n'auraient-il pas le droit de le faire eux-même via l'opérateur de portée?

Genre:

Code:

---

1 2 3 4 5

#include "sprite.h"   Sprite :: Sprite( const Sprite &s ) { /* Ici l'implémentation du developpeur */ }

---

Citation:

---

Chose que je n'ai pas comprise, si je met le constructeur par recopie et l'opérateur = en partie privée, je suis d'accord que la recopie devient impossible. Cependant, comme on ne l'implémente pas dans le .cpp, qu'est ce qui empêche les developpeurs à l'implémenter eux-même? Pourquoi n'auraient-il pas le droit de le faire eux-même via l'opérateur de portée?

---

Rien. Mais le but de la manip c'est de prévenir un mauvaise utilisation ; après, le mec qui fait ça je pense qu'il sait exactement qu'il contourne le comportement souhaité, et là on ne peux pas l'en empêcher ;)
De toute façon l'accès privé couvre déjà 95% des cas ; seules les classes amies arriveront sur l'erreur d'édition de liens.