[Python/kandinsky] Add a new color type

RedGl0w commented 3 years ago

Problem you'd like to fix

Currently, the python kandinsky module works mostly in RGB888, but the numworks screen uses RGB565. When we start to compare a color we set on a pixel, and a pixel we get, we don't get the same color, and if we use the operator ==, it returns false. This can be very strange for newcomers.

Describe the solution you'd like

In kandinsky, there's also the color function, which currently returns a tuple in RGB888. The best solution would be to have a new type in the module, which stores a KDColor (so a RGB565 color), but which behaves like currently : when printing or accessing to an index of it, we would have the RGB888 conversion. The only difference would be in the == operator, which would return true if the KDColor stored are the same.

Example :

from kandinsky import *
c = color(126,46,39)
set_pixel(0,0,c)
print(get_pixel(0,0) == c)

It would return true with this feature, while it currently returns false But some other scripts would still work :

from kandinsky import *
c = color(126,46,39)
print(c[0],c[1],c[2])

It would return exactly the same result with this feature.

fime-space commented 3 years ago

I noticed a problem that seems to be the same that looks like a color compression problem: when I define set_pixel(0.0, (255,255,255,255)), the get_pixel(0,0) function returns me another color that seems to be compressed, (248,252,248)... (It posed me a problem when I was designing a program or I couldn't compare the variables as you showed)

RedGl0w commented 3 years ago

I noticed a problem that seems to be the same that looks like a color compression problem: when I define set_pixel(0.0, (255,255,255,255)), the get_pixel(0,0) function returns me another color that seems to be compressed, (248,252,248)... (It posed me a problem when I was designing a program or I couldn't compare the variables as you showed)

This is not a "compression" issue, but it is this is due to the fact that the kandinsky module uses RGB888, whereas the screen is using RGB565, and so the conversion between both can't be exact. This idea above should partially solve this issue.

Ecco commented 3 years ago

Hi @RedGl0w ! You're right, there's a problem. That being said, overriding the equality testing would not really be a great fix either. Your example would definitely work (which is good), but this would still be a problem:

from kandinsky import *
c = color(126,46,39)
set_pixel(0,0,c)
d = get_pixel(0,0)
print(d[0],d[1],d[2])

The options we have:

Don't make colors tuples, but make them an opaque type instead.
Override the equality operator.
Add an explicit comparison operator. Something like c.looks_like(d). Not sure it would be very helpful for beginners.
get_pixel could return a list of all the potential RGB888 colors that match 🤓
get_pixel could try and return colors that were recently used in set_pixel? That would be kind of cheating, but we could keep the last color used in set_pixel, and if the get_pixel value is close enough, then we return the one used in set_pixel
We could improve the RGB565 -> RGB888 conversion by forcing 0,0,0 to be 0,0,0 and 255,255,255 to be 255,255,255. This would not fix the "inbetween" cases but it would fix the black and white ones.

Solution 5 seems smart, but will not always work, and might be way too "magic". The real problem is that our framebuffer is 16bpp, so we will not be able to reinvent lost precision. Solution 2 (your proposal) is also a bit too magic.

I would vote for either option 5 or option 3. Maybe both.

RedGl0w commented 3 years ago

1. Don't make colors tuples, but make them an opaque type instead.

That would have been the best solution when the kandinsky module was added, but now it's too late to change :/

2. Override the equality operator.

That was the idea of the issue indeed

3. Add an explicit comparison operator. Something like `c.looks_like(d)`. Not sure it would be very helpful for beginners.

Nope, it would be way to hidden...

4. `get_pixel` could return a list of all the potential RGB888 colors that match nerd_face

Indeed, when you get the color, you don't get a set of color. This would be a bit too weird

5. `get_pixel` could try and return colors that were recently used in `set_pixel`? That would be kind of cheating, but we could keep the last color used in `set_pixel`, and if the `get_pixel` value is close enough, then we return the one used in `set_pixel`

Seems very strange. And this wouldn't work in a lot of case (for instance in this game in which the get_pixel is used on a totally different pixel that the previous set one)

6. We could improve the RGB565 -> RGB888 conversion by forcing 0,0,0 to be 0,0,0 and 255,255,255 to be 255,255,255. This would not fix the "inbetween" cases but it would fix the black and white ones.

Yeah, why not indeed...

Solution 2 (your proposal) is also a bit too magic.

It's a bit magic, but it doesn't break current scripts, and is easy to use... For me, even if it's a very strange behavior, it's the best solution

numworks / epsilon