changed Color and added Colormaps #152
Conversation
|
Comme il n'y a pas d'urgence à accepter cette PR je passerai à l'occasion sur les codes afin d'uniformiser les notations et d'ajouter de commentaires. Si des parties ne te semblent pas claires et te semblent mériter une attention particulière n'hésite pas à commenter la PR ! |
|
Merci pour ta proposition ! J'ai fait quelques observations mais je te laisse décider. Pour l'ambiguité du mode RGB/HSV converti en alpha, effectivement il faut éviter ça. Le plus simple c'est d'utiliser les Voir aussi les héritages |
|
A priori il ne me reste plus qu'à modifier les TU et ça devrait être bon. Il y a juste une chose notable : Je suis pas fan de cette différence, il y a peut-être une modif à apporter au binding ? |
Effectivement c'est pas top. Alors peut-être nommer l'énumération "Model" et la place en dehors de la classe Color, pour que ce soit cohérent des deux côtés... |
Je suis parti sur cette solution du coup. Tous les tests passent de mon côté (C++ et python) Si je n'ai rien raté j'ai tenu compte de tous tes commentaires. Si certains sont passés à travers les mailles du filet n'hésite pas à réouvrir le commentaire associé. |
|
C'est super ! 👍 Je me suis permis quelques petites discussions (voir plus bas) mais on peut clore cette PR 👍 |
| py::class_<ColorMap> exported_colormap(m, "ColorMap", COLORMAP_MAIN); | ||
| exported_colormap | ||
|
|
||
| .def_readwrite("m", &ColorMap::m, |
src/graphics/styles/codac2_Color.h
Outdated
| explicit Color(float r, float g, float b, float alpha = 1.); | ||
| explicit Color(int r, int g, int b, int alpha = 255); | ||
|
|
||
| Model m = Model::RGB; //RGB or HSV |
There was a problem hiding this comment.
Idéalement il faudrait que ce soit const et protected (sinon l'utilisateur peut changer le mode et mettre des valeurs incohérentes avec le mode). Mais il faudrait aussi rendre float& std::array<float,4>::operator[](size_t) en protected (ne laisser que son équivalent const en public) pour qu'une Color ne soit pas éditable. Bref c'est pas critique vu l'usage qu'on en fait :)
C'est juste pour info.
There was a problem hiding this comment.
Passer le Model en const protected semble poser des problèmes pour les ColorMap.
De ma compréhension quand on fait :
map[key]=value;
Si la clé n'existe pas il crée d'abord la paire (key,value) en utilisant le constructeur par défaut (ici le constructeur Color::Color()) puis fais l'affectation de la couleur ... ce qui se passe mal vu que le Model est déjà défini dans le constructeur par défaut
Compromis : j'ai mis le Model m en protected avec un accesseur en read-only model()
There was a problem hiding this comment.
Pour ce qui est de protéger le tableau de valeur, définir deux operateurs [] semble lourd point de vue utilisateur car besoin de spécifier const float x= color[i] à chaque fois, sinon message d'erreur car C++ pense qu'on souhaite attaquer l'opérateur protégé (non const)
| Color color_lb = prev(it_ub)->second; | ||
| Color color_ub = it_ub->second; | ||
|
|
||
| // Interpolation according to the ColorMap model |
There was a problem hiding this comment.
J'aurais supprimé les lignes 36 à 48 : l'interpolation se fait dans l'espace de couleur défini par ses points de contrôles. Si la CM est faite de Color en HSV, alors l'interpolation est en HSV, et réciproquement pour le RGB.
There was a problem hiding this comment.
J'y ai pensé, mais le problème est que rien n'empêche un utilisateur de créer sa propre ColorMap avec des couleurs en RGB et d'autres en HSV, auquel cas il faut convertir tout dans le même type avant interpolation .
Edit :
Ou alors on suppose/force que toutes les couleurs sont au même format
| */ | ||
| struct ColorMap : public std::map<float,Color> | ||
| { | ||
| Model m; //RGB or HSV |
There was a problem hiding this comment.
Et donc plus besoin de cet attribut.
|
Je me suis mal exprimé hier, *tous les test unitaires passent de mon côté. j'ai tenté un nouveau commit avec le changement que tu as suggéré on va voir si les actions git passent |
|
Bien vu, tous les tests passent ! |
|
👍 👍 |
@SimonRohou les test unitaires passent tous, j'ai juste dû autoriser des marges de tolérance dans les conversions (RGB/HSV, float/int ...).
Il y a juste un point qui me dérange un peu et que je n'ai pas réussi à résoudre :
Mettons qu'en python je veuille créer une couleur HSV, j'écrirais quelque chose de la forme :
Color(0, 100, 100, InterpolMode.HSV)
Seulement le casting automatique python préfère appeler ce constructeur en convertissant le char en int (pensé pour les couleurs HSVA et RGBA):
.def(py::init<int,int,int,int,InterpolMode>(),
Plutôt que celui prévu à cet effet :
.def(py::init<int,int,int,InterpolMode>(),
La solution que j'ai trouvée et d'écrire :
Color(0, 100, 100, interpol_mode=InterpolMode.HSV)
Mais je me demande si il n'y a pas moyen de faire mieux