[nRGBlib] W.I.P - Bibliothèque couleurs pour Ndless 3.1

[Lionel Debroux]

Il n'est pas utile d'utiliser un pointeur, utilise simplement le passage par valeur

Code: Tout sélectionner

typedef uint16_t Color;
#define RGB(r, g, b) (Color)(((((uint8_t)(r)) / 8) << 11) | ((((uint8_t)(g)) / 4) << 5) | (((uint8_t)(b)) / 8))

Il y a probablement quelques parenthèses en trop, mais mieux vaut trop que pas assez
En particulier, les parenthèses autour de r, g et b dans le corps de la macro sont importantes; un de mes exemples, sur la page précédente, n'en avait pas, mais c'est une erreur.
Le cast en uint8_t est a pour but de forcer le compilo à faire une division / un shift droit sur une valeur non signée, ce qui est préférable pour que l'extension du bit de signe n'ait pas lieu et produise des résultats incorrects (interaction entre r et g, et g et b).

[totorigolo]

J'en ai fini avec mes bugs Et Bubble Land m'a l'air beaucoup plus rapide

Cependant (ça serait trop beau), il me reste deux problèmes à résoudre avant la sortie de la v0.2 (synchro avec Ndless 3 ), donc je dois me dépêcher.

• Je n'ai pas réussi à inclure libndls dans nRGBlib pour pouvoir utiliser lcd_incolor/ingray(), lcd_isincolor(). J'ai pour le moment fait un copié-collé du code de libndls, du coup quid de la license ?
• Les variables Color.R/G/B sont fausses, alors que Color.raw est juste. D'où est-ce que ça peux venir ? Cela est préférable pour le portage sur non-CX (ou alors je convertie directement de raw vers R, G et B ?)
  

  Code: Tout sélectionner

  typedef union Color {
    struct {
        uint8_t R : 5;
        uint8_t G : 6;
        uint8_t B : 5;
    };
    uint16_t raw;
} Color;

Je rappelle https://bitbucket.org/totorigolo/nrgblib/overview si besoin de voir le code.

PS: Lionel : J'ai converti le code aux (u)int(8/16/32)_t

[Lionel Debroux]

Comme je l'ai écrit plus haut, l'inconvénient de la méthode avec l'union Color telle que je l'avais décrite est qu'il faut mettre les couleurs sur 5 ou 6 bits, ce qui est donc moins pratique, et aussi (potentiellement) que l'endianness pourrait interférer avec ce qu'on veut faire (je n'ai pas vérifié ce deuxième point).
C'est pour ça que je n'ai pas suggéré de nouveau cette méthode dans mon précédent post, qui faisait suite à un post où tu utilisais des couleurs 8 bits

Est-ce que tes routines sont clippées (= n'écrivent pas en-dehors de l'écran) ? C'est plutôt plus facile à faire avec 16 bpp qu'avec 4 bpp. EDIT: oui, la plupart des routines sont clippées par setPixel.

EDIT: attention, avec des tests CX / non CX dans setPixel, qui peut potentiellement être appelée des milliers de fois, toutes tes routines seront lentes


A la relecture de ta liste dans le premier post, et en admettant qu'on utilise le même vocabulaire, il me semble qu'il y a un manque important: les routines de sprite
Les routines de tile sont un cas particulier des routines de sprite, où les valeurs de x et y sont multiples de 8; les routines de sprite n'ont pas cette contrainte. Sans routines de sprite, entre bien d'autres programmes qui sont impossibles, pas de jeux de plate-forme à la Mario.
Là encore, c'est plus facile à faire avec 16 bpp qu'avec 4 bpp, d'autant plus que les contraintes d'alignement sont plus désagréables sur ARM que sur 68000.

Il est important de prévoir une suite de tests dès le début d'une librairie. ExtGraph n'en a pas vraiment eu jusqu'à devenir monstrueuse, et bien que la création tardive de code de tests m'ait permis de corriger un certain nombre de bêtises, la couverture de tests reste (et restera) assez basse...

[Lionel Debroux]

Je modifierais setPixel de la façon suivante (/!\ non testée):

Code: Tout sélectionner

void setPixel(int16_t x, int16_t y, Color c)
{
    if (x < 0 || x >= 320 || y < 0 || y >= 240) // SCREEN_WIDTH et SCREEN_HEIGHT ?
        return;

    // Nspire non-CX // Plutôt, 4 bpp
    if (!has_colors) // Devrait être !has_colors || !lcd_isincolors()
    {
        if (lcd_isincolor()) // Inutile
            lcd_ingray(); // Inutile.

        char col = ((c.R / 8) + (c.G / 4) + (c.B / 8)) / 48; // 3 * 16 // Incorrect: c.R, c.G et c.B ne sont pas des valeurs 8 bits ;)
        unsigned char* p = (unsigned char*)(SCREEN_BASE_ADDRESS + ((x >> 1) + (y << 7) + (y << 5))); //
        //*p = (x & 1) ? ((*p & 0xF0) | col) : ((*p & 0x0F) | (col << 4)); // Ceci serait correct.
        *p = c.raw; // Incorrect: écrit une couleur 16 bits qui n'a rien à voir, au lieu de col que tu viens juste de calculer :)
    }
    // Nspire CX // Plutôt, 16 bpp.
    else
    {
        if (!lcd_isincolor()) // Inutile
            lcd_incolor(); // Non. Il faut respecter la volonté de l'utilisateur d'utiliser l'écran de la CX en 4 bpp :)

        volatile unsigned char *scr_base = SCREEN_BASE_ADDRESS; // Pourquoi volatile ?
        volatile unsigned char *ptr; // Idem
        ptr = scr_base + 2*(x + SCREEN_WIDTH * y); // Pinaillage: 2 -> sizeof(uint16_t)

        //                               Rouge 0-31        Vert 0-63       Bleu 0-31
        *(volatile unsigned short*)ptr = c.raw; // OK, mais pourquoi volatile ? :)
    }
}


Pour que le compilo puisse optimiser beaucoup plus, je pense que comme ExtGraph, tu devrais faire des macros de pixel séparées pour 4 bpp et 16 bpp, et séparer les chemins de code 4 bpp et 16 bpp dans toutes les fonctions qui appellent actuellement setPixel

[totorigolo]

Oups, excuse moi j'ai pas fait mon dernier push hier soir, qui enlevait bcp de remarques que tu fais. Je viens de le faire, et j'ai appliqué tes suggestions qui ne me posent pas problème.
EDIT: Pas encore fini de faire ce que tu m'as dis. C'est bon le push est fait et le post suivant en parle

Code: Tout sélectionner

    if (x < 0 || x >= 320 || y < 0 || y >= 240) // SCREEN_WIDTH et SCREEN_HEIGHT ?
        return;

C'était pour un petit debug, et j'ai oublié de les remettre.

Code: Tout sélectionner

    // 4 bpp <- Ok
    if (!has_colors) // Devrait être !has_colors || !lcd_isincolors()

Le problème avec lcd_isincolor() maintenant, c'est que j'aurais encore mes problèmes de liaison de libndls -> obligé d'utiliser la fonction dans le main du programme.

Code: Tout sélectionner

        char col = ((c.R / 8) + (c.G / 4) + (c.B / 8)) / 48; // 3 * 16 // Incorrect: c.R, c.G et c.B ne sont pas des valeurs 8 bits ;)

Comment faire alors pour accéder à R, G et B ? Si je comprends bien, pas possible avec les bit fields (j'ai mis int16_t parce que je croyais que ça n'avait aucune importance avec le bit field) ? Si oui, je sais comment faire pour les récupérer autrement.

Code: Tout sélectionner

        unsigned char* p = (unsigned char*)(SCREEN_BASE_ADDRESS + ((x >> 1) + (y << 7) + (y << 5))); //
        //*p = (x & 1) ? ((*p & 0xF0) | col) : ((*p & 0x0F) | (col << 4)); // Ceci serait correct.
        *p = c.raw; // Incorrect: écrit une couleur 16 bits qui n'a rien à voir, au lieu de col que tu viens juste de calculer :)

Là encore un test, que j'ai enlevé avec mon commit non pushé ( ) J'ai essayé ça parce que vous aviez dis d'essayer en 16 bpp, ce que j'avoue ne pas avoir très bien compris. (faut pas oublier formation SdZ )

Code: Tout sélectionner

        volatile unsigned char *scr_base = SCREEN_BASE_ADDRESS; // Pourquoi volatile ?
        volatile unsigned char *ptr; // Idem
        ptr = scr_base + sizeof(uint16_t) * (x + SCREEN_WIDTH * y); // Je ne savais pas d'où venait ce "2" ^^

        //                               Rouge 0-31        Vert 0-63       Bleu 0-31
        *(volatile unsigned short*)ptr = c.raw; // OK, mais pourquoi volatile ? :)

J'utilise ces volatiles car j'ai repris, à l'origine, ces lignes du sample "color" de Ndless (le copié-collé-adapté-mais-conforme de ce code est disponible dans RGBstrips()).

Pour que le compilo puisse optimiser beaucoup plus, je pense que comme ExtGraph, tu devrais faire des macros de pixel séparées pour 4 bpp et 16 bpp, et séparer les chemins de code 4 bpp et 16 bpp dans toutes les fonctions qui appellent actuellement setPixel

Si tu veux que je fasse des macros, comment faire pour pouvoir avoir la compatibilité des progs' avec 4bpp & 16bpp sans recompiler ?

[totorigolo]

Voilà, j'ai corrigé TOUS les bugs et logiquement cette version est prête pour la release publique 0.2 (faut que je me dépêche ).

Lionel: J'ai appliqué un peu tout ce que tu m'as dis (enfin j'espère), sauf :

• Le coup de la macro car je ne vois pas comment rendre ça compatible avec toutes les Nspires
• Le "if (!has_colors) // Devrait être !has_colors || !lcd_isincolors()" car je trouve que ça ralentie l’exécution dans Bubble Land alors que je n'appelle pas la fonction -> j'ai recopié le code dans le if (enfin la ligne).

Si tu as encore des remarques à faire, n'hésite pas
Et je tiens à te remercier pour tout le soutient que tu m'apporte

PS: J'ai également profité de la suppression des volatiles pour découvrir que la Nspire pouvait afficher des couleurs transparentes Je vais creuser un peu là-dessus et peut-être newser ...

Dernier rappel non-officiel : https://bitbucket.org/totorigolo/nrgblib/overview

EDIT: J'avais encore oublié les push !!! <- C'est fait

[Lionel Debroux]

Si tu n'utilises pas !has_colors || !lcd_isincolors(), les fonctions ne vont tout simplement pas faire ce que le programmeur attend d'une Nspire CX (pour laquelle has_colors) dont l'écran est réglé en mode 4 bpp (i.e. !lcd_isincolors())
Pour porter les six demos de TICT vers la CX, je ne compte pas utiliser une lib externe - mais si je voulais le faire, je ne pourrais pas utiliser nRGBlib, telle qu'elle est actuellement, pour les demos dans lesquelles le passage à 16 bpp n'apporte rien

Dans nCOMMON.h:

Code: Tout sélectionner

#define getR(c) (((c & 0xF800) >> 11) / 2)
#define getG(c) (((c & 0x7E0) >> 5) / 4)
#define getB(c) ((c & 0x1F) / 2)

Il manque des parenthèses autour de c, en partie droite

Dans clearScreen.c:

Code: Tout sélectionner

    if (!has_colors) // Même remar
    {
        for (i = 0; i < SCREEN_WIDTH; i++)
        {
            for (j = 0; j < SCREEN_HEIGHT; j++)
            {
                setPixel(i, j, c);
            }
        } // TODO: Optimiser
    }

Oooooh. TODO: optimiser, en effet

[totorigolo]

Peux-tu m'expliquer à quoi servent les parenthèses autour de "c" ? Parce que à première vu c'est inutile pour moi. (Pour m'empêcher de refaire cette erreur plus tard)

[Lionel Debroux]

En effet, dans ce cas-ci, je n'arrive pas à construire un exemple où ça ne fonctionnerait pas si on oublie les parenthèses
Mais il est facile de construire un exemple où le compilo déclenche des warnings: dans la définition de c, il suffit d'utiliser une opération comme | ou ^ (les opérations de précédence plus basse que &).
Voir http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B pour quelques bizarreries de la précédence des opérateurs, par exemple la différence entre a & b == c et a + b == c.

Certaines entreprises, lors des entretiens d'embauche, posent des questions sur la précédence des opérateurs. Je préfère faire du code avec un peu plus de parenthèses que nécessaire, mais un peu moins susceptible de ne pas fonctionner dans certaines conditions

[totorigolo]

Merci pour le lien et l'info
Je viens de mettre ce que tu viens de me dire. J'ai pushé cette fois

Oooooh. TODO: optimiser, en effet

C'est fait

Code: Tout sélectionner

if (!has_colors) // Devrait être !has_colors || !lcd_isincolors()

Ca aussi Mais je trouve (à l’œil) que c'est plus lent. J'ai donc fait le code suivant pour que l'utilisateur puisse recompiler et avoir la pleine vitesse, si ça n'a pas d'importance pour ce que l'utilisateur veut faire.

nCOMMON.h :

Code: Tout sélectionner

// (Dés)active la possibilité d'utiliser l'écran en 4 bpp sur Nspires 16bpp, mais accélère le dessin
#define __FAST_SETPIXEL__ 0

setPixel.c & clearScreen.c :

Code: Tout sélectionner

#if (__FAST_SETPIXEL__ == 1) // nCOMMON.h
    if (!has_colors)
#else
    if (!has_colors || ((*IO_LCD_CONTROL & 0b1110) != 0b1100)) // R5G6B5 - libndls - lcd_isincolor())
#endif

Je me penche maintenant sur la Documentation (rapide -> README.txt). Penses-tu que je doivent traduire les commentaires de nRGBlib ?

Si quelqu'un voit quelque chose à changer / rajouter, n'hésitez pas