Topic: r8193 - Wrong array index in get_color_rgb?

[Ters]

When compiling, I noticed warnings about "array subscript is above array bounds" on simgraph16.cc line 734. And as far as I can tell, there is a bug there, and I've added a patch showing how I expected the code to be. The catch is that I can't see any visible difference in the game whether I use the suspected buggy code, or the possibly fixed code. (I though this might be the cause for unreadable titles in inactive windows, but apparently, this is not the cause of that.)

[Dwachs]

Thanks! It seems this code path is not used. Anyway, this is now r8197

[An_dz]

Good catch, but as Dwachs said it's never used, maybe only for one or two users that changed any colour to one of these.

(I though this might be the cause for unreadable titles in inactive windows, but apparently, this is not the cause of that.)

What bug is that?

[Ters]

What bug is that?

See the attached screen screenshot of an unfocused depot window. It's not exactly very readable, but I don't know what has caused those colors, whether it is Simutrans itself, the pak set or some botched upgrading on my part.

[An_dz]

The code is correct so the problem was with the theme. I've changed the default colour to #DDDDDD in r8200.

[Ters]

So it was just that no other players cared?

[An_dz]

Not exactly, my colour patch has changed how the titlebar is rendered. Before it the colours were not a problem because the 8 bit index guaranteed that the "brightness" would reduce correctly (in HSV space, or even more closely the Lightness in Lab space) and so the text colour managed to match more perfectly.

With 16 bit colours there's no easy way to change brightness, unless you move the RGB to HSV or Lab, change the value and go back to RGB. But this would be expensive, so instead the code just adds a black overlay over the titlebar (and the transparency can be tweaked by the theme) as a means of "fake value/brightness reduction" which in turn also ends up reducing saturation. This ended up making the text colour clash more easily with the player colours. The colour was moved to RGB so the theme can also change the default titlebar colour to any RGB value.

So my patch helped but the colours were not very good since the beginning.

[Ters]

Not exactly, my colour patch has changed how the titlebar is rendered. Before it the colours were not a problem because the 8 bit index guaranteed that the "brightness" would reduce correctly (in HSV space, or even more closely the Lightness in Lab space) and so the text colour managed to match more perfectly.

That was not what I meant. What I meant was the fact that it took so long for anyone, including myself, to notice, care and/or complain about the text having become unreadable. I thought it was only on my computer things had become messed up, since people reported several of other GUI issues, but not this.

[prissi]

Actually, blending is much more expensive than coverting a few colors ...

[An_dz]

Actually, blending is much more expensive than coverting a few colors ...

So, is it less expensive to move to Lab, change Lightness and move back to RGB?

[DrSuperGood]

So, is it less expensive to move to Lab, change Lightness and move back to RGB?

If each pixel is 16 bits then a 65,536 element LUT (~120 kb) could be used to perform the conversion very efficiently. As long as the result is still 16 bits then another such LUT could perform the inverse conversion very efficiently. That said there are non trivial performance implications of this due to the LUT size and poor data locality hence small simple calculations might be faster in some cases.

Performing 16 bit blending has to be done mathematically using 2 pixels to produce an output pixel. In theory for the blending to be correct similar LUTs should already be used as blending has to be performed in linear colour space rather than sRGB (all Simutrans art is sRGB) and the results output in sRGB (most consumer displays are sRGB) however this step is skipped for performance resulting in strange but acceptable blending response. Modern GPU hardware can do both blending and sRGB <-> linear conversions extremely efficiently, but Simutrans does not use GPU acceleration.

[prissi]

The blending routines does the "conversion" for each pixel Hence it is much more efficient to calculate this once at the start if the background color is known already. This is the case for the blending of window colors. The code below is what actually happening in simgraph16.cc for each pixel (for RGB565):

darker1 = (color >> 1) & 0x7BEF;
darker2 = (color >> 2) & 0x39E7;

//handle overflow for brighten in a very simple way
brighter = ((color >> 1) & 0xF7DE) | 0x08021 | (color & 0x8000 ? 0x8000 : 0) | (color & 0x0400 ? 0x0400 : 0) | (color & 0x0010 ? 0x0010 : 0);

Anyway I will add routines for blending colors in the same way than images (which is rather trivial).
Thus it is trivial to add either a blend_pixval_rgb routine and a darken_pixval_rgb routine to convert the color once.

[An_dz]

The code works nice. But I'm refraining from committing as I believe a change should be done.

I think that the names src & dest should be renamed to foreground & background and their position transposed.

The reasons are that the new names make more sense graphically, the background will be blended with the foreground. And the background should come first so the foreground and the percentage are one right after the other, following display_blend_wh_rgb(..., colour, percentage)

[prissi]

Please rename them too, I just used their name copying from the original blending which used src and dest pixels ...

Has been submitted before r8208

[An_dz]

Function was introduced in r8204, a typo fix was made in r8205.

I've renamed and used it on titlebar & tooltips in r8207.

I guess now everything was fixed for this bug