News:

The Forum Rules and Guidelines
Our forum has Rules and Guidelines. Please, be kind and read them ;).

Patch to move colours to 16 bit

Started by An_dz, December 29, 2016, 03:43:09 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

An_dz

I've created this patch that simply moves the current colours defined in simcolor.h to be defined in the gui_theme. It's the first step for moving them to 16bit.

I've attached the patch file, or you can check it on GitHub:

https://github.com/An-dz/simutrans/commit/1b1d1c0d4d42e90a5cca381de0cf15a081fc0de8

prissi

In most cases the performance lost of const versus static global is neqlible, but why should white ever be redefined? Would it not make sense to have some elemental colors as const? (Probably in the form 0xrrggbb as 16 int)?

An_dz

#2
Yes, good point, I just copied from the SYSCOL values and did not look at it, const makes more sense for those colours.

I'm still working on moving to 16 bit, this patch is just to move the values in the right places. The only two colours that I can set a fixed value are black and white, as they are always 0 and 0xFFFF, the others need to be calculated as it depends if it's RGB555 or RGB565 (which makes me wonder if now const would break this calculation). The current patch at least compiles, I can't say the same for the current working tree :)

prissi

Simutrans internally uses RGB555, so I would use this for colors too. Any color above 0x8000 is a special color ...

Ters

Doesn't Simutrans normally specify colors as byte triplets in the code? Isn't it only simsys_?.cc that knows whether colors are supposed to be RGB or BGR?

An_dz

I've read the code from the important areas and it works like this:
1) The 8 bit palette is defined as RGB triplets
2) The code generates the 16 bit values for all the 8 bit palette and saves that in an array
3) The code makes calls to functions sending the 8 bit index, those functions are #defines that call the 'real' function with the colour already converted to the correct system as it uses the index in the array above.
4) For the theme system the triplets loaded from the tab file are checked against a subset of the 8 bit palette triplets to choose the most similar index and uses this index as the colour.

Simutrans uses COLOR_VAL for the colours, which is 8 bit as the palette and stores the index. PLAYER_COLOR_VAL has 16 bit and is just an extended version of COLOR_VAL, the first 8 bits are the colour and the last 8 are flags, bit 15 is for setting as player colour, bit 14 to 11 are for transparency.

I already changed almost everything now, it's working well except for a few colours defined outside any functions in four files.

An_dz

Ok, now I see what prissi said. Simutrans is always RGB555, but it converts the colours to the system type.

Ters

Quote from: An_dz on January 03, 2017, 05:13:17 PM
Ok, now I see what prissi said. Simutrans is always RGB555, but it converts the colours to the system type.

That seems somewhat contradictory. The pak files are always pseudo-RGB555, but it is converted to RGB565/RGB555/BGR565/BGR555 half-way through simgraph16.cc.

prissi

That is not true, there could be easily a simgrap32.cc (which has 4x more memeory bandwidth), and there had been also a simgraph8. Also for copying on screen a 8x8x8 lookup-table can be used (and was at a time)

An_dz

Quote from: prissi on January 03, 2017, 10:33:19 PM
That is not true, there could be easily a simgrap32.cc (which has 4x more memeory bandwidth)
Is it so? I mean, with 24 bit colour there would have no need to have any array holding colours.

Ters

Quote from: An_dz on January 04, 2017, 02:47:19 AM
Is it so? I mean, with 24 bit colour there would have no need to have any array holding colours.

The only arrays holding colors in Simutrans are for converting the pseudo-RGB555 format to whatever color format simsys_?.cc has set up. If the latter sets up a 24-bit format, you still need the conversion due to the pseudo-part, which has to do with player colors and lights. There might be other ways of doing it than an array, but they might not be any better.

To eliminate the arrays (or similar alternatives), we need to drop the pseudo-RGB555 format used in the pak files, which means that we need a different solution for doing player colors and lights, something that has been touched upon in another recent discussion.

An_dz

I've tried prissi's suggestion but I ended up with a problem when converting the 8 bit palette to the pseudo 555. The current code already converts the 8 bit palette to the system type and not to the pseudo 555. So I continued my previous patch of using the system format everywhere and use the _rgb functions directly.

The only problem is getting the colours before they are initialised in karte.cc, factory_chart.cc & citylist_frame_t.cc. I'm still looking for what's the best solution.

An_dz

I've moved everything to 16 bit and even though it works on Linux it does not on Windows compiled with MinGW. Obviously because of my hackish code so I need help.

https://github.com/An-dz/simutrans/commits/master

http://simutrans-germany.com/files/upload/colour.txt

Dwachs

It compiles with my mxe installation (using whatever mingw). What errors do you get?
Parsley, sage, rosemary, and maggikraut.

Dwachs

It crashes for me due to uninitialized SDL stuff. backtrace:

342             return SDL_MapRGB(screen->format, (Uint8)r, (Uint8)g, (Uint8)b);
(gdb) bt
#0  0x0000000000768445 in get_system_color (r=36, g=75, b=103) at simsys_s.cc:342
#1  0x00000000004a9466 in init_colors () at display/simgraph16.cc:749
#2  0x00000000004bacec in __static_initialization_and_destruction_0 (__initialize_p=1, __priority=65535) at display/simgraph16.cc:762
#3  0x00000000004bad08 in _GLOBAL__sub_I_large_font_ascent () at display/simgraph16.cc:5573
#4  0x00000000007692cd in __libc_csu_init (argc=1, argv=0x7fffffffd978, envp=0x7fffffffd988) at elf-init.c:88
#5  0x00007ffff6950b75 in __libc_start_main () from /lib64/libc.so.6
#6  0x00000000004056a9 in _start () at ../sysdeps/x86_64/start.S:122
(gdb) print screen
$1 = (SDL_Surface *) 0x0
Parsley, sage, rosemary, and maggikraut.

An_dz

#15
I've compiled for GDI with MSYS2 + MinGW32 and the colours don't initialise. Everything is black, that's the problem.

On Linux I've compiled for SDL2 and everything works fine.

But I'll check SDL1 on Linux and both SDL on Windows to see.

Edit:
Same error on Windows & Linux with SDL1
SDL2 on Windows has the same behaviour as the GDI version

Ters

Skimming through the patch, it looks like color_idx_to_rgb is used to initialize variables with static storage. But it uses specialcolormap_all_day, and that is initialized by init_colors, which is called after color_idx_to_rgb has been used to initialize the aforementioned variables.

An_dz

Yes, this seems obvious, but my question is why? What defines the order? I thought the order they were defined in the makefile defined the order the declarations run. Even when I add a call to init_colors before running gui_theme gui_banner is still ran before and the colours of the scrolling text is black.

Dwachs

I think the order of initialization of static variables in different files is undefined.
Parsley, sage, rosemary, and maggikraut.

An_dz

Thanks, I've found some info about it. Let's see if I can work this out. :D

Ters

Quote from: Dwachs on January 26, 2017, 08:55:39 PM
I think the order of initialization of static variables in different files is undefined.

But they all happen before main is called, and that in turn is before any other method is called. Except things that are called to initialize those variables, usually constructors. Plus some other exotic things, which, as far as I know, Simutrans doesn't use.

I might have overlooked something, though, because I'm puzzled that some of it works at all. In particular gui_theme.cc initializes a lot of variables using color_idx_to_rgb in a static context. That should not work. But maybe the values are all overwritten later on.

prissi

#21
The order in the makefile does not matter. And simutrans uses alphabetical order.

But my concern is that PIXVAL and COL_VAL is used interchangable. I would just change COL_VAL to a different type, and maybe even intoduce COL_VALRGB, as it was done in other places. Because the PIXVAL can be 15 or 16 bit, depending on the backend driver. (Not to mention darkening). If you either use rgb 555 or rgb 888 consequently, then you can still use fixed indices and then either lookup using the darkening table or non-darkening 555* table when passing the index at runtime.

Also what stands FLAGGED_PIXVAL for? The name explains nothing to me. You have either COL_VAL, COL_VAL_DARKENING (not sure this is used much though). I suspect the flagges stands for something like that?

The init_colors must be called from simgraph_init, at the same position where the old tables were init (line 5401).

But having see that, I am no sure how to actually get the newest patch or diff file to the original master out of github. So I cannot comment if I really saw the right code.

Sorry for sounding so negative, though.

An_dz

I've worked on the problems and now it works correctly, initialisation will occur on the first call for a colour.

Tested on:
Windows GDI, SLD2
Linux SDL2, POSIX (no graphs)

It still crashes for SLD1. The patch is now up to r8087.

http://simutrans-germany.com/files/upload/16bitcolours.txt

Quote from: prissi on January 30, 2017, 09:26:21 AM
But my concern is that PIXVAL and COL_VAL is used interchangable.
COL_VAL is dead, it's only PIXVAL now.

Quote from: prissi on January 30, 2017, 09:26:21 AM
Also what stands FLAGGED_PIXVAL for?
PIXVAL + flags. Makes more sense than PLAYER_COLOR_VAL.

Dwachs

#23
The problem is that these COL_* definitions are used in static initialization way before the SDL stuff is initialized, which is however needed to initialize colors. This seems to be almost unfixable.

Edit: Attached are three patches against yours. I replaced gui_theme::get_color by color_by_idx in simgraph.h. Color initialization is done in simgraph_init. No crashes, but a lot of colors are wrong: for instance, the buttons in the finance window are black.
Parsley, sage, rosemary, and maggikraut.

An_dz

Ok, but that's basically how the code first came. I can do that much simpler without that init_colors_table function.

I have created that function and the get_color function to make sure the colours are initialised in time, what I did is necessary, The first use of one of the colours is even before simgraph_init is started. In fact, even before main is.

And SDL1 is deprecated, last version was in 2012.

Dwachs

The SDL1 crashes because colors are converted before the graphics system is set up. Accessing graphics stuff before everything is initialized is not nice imho.

What about changing the get_color routine to return ``const PIXVAL&'' instead of ``PIXVAL'' ? The reference would refer to the entry in the conversion table. At static initialization only these references would be set up. Colors will be accessed much later when the gui starts. The color table itself can be initialized safely after the graphics system is running. I will try this out.

This would allow also for changing color at run-time. In your patch, some gui colors will be set at static initialization and cannot be changed afterwards. (which is less flexible than the old system using these COL_* indices). With my proposal changing colors would be possible. This would hopefully achieve what you wanted with the patch: get rid of this COL-* indices while retaining the flexibility of the old system.
Parsley, sage, rosemary, and maggikraut.

Dwachs

This does not work - no arrays of references. So without further hacking, this patch will break SDL1 support forever. What to do???
Parsley, sage, rosemary, and maggikraut.

An_dz

I've tried using the pseudo RGB555, it can work, but will need a lot of changes and will end up with something that I really don't like that is an array to use in another array. All I know is that I tried it and stopped at some point because it would lead in a crazy code.

So the possibility I can think of now is to move the colours that are initialised before main inside a function to be initialised after the simgraph engine has started. I'll debug the code to see if there's any colour I don't know.

An_dz

#28
My solution now is to simply use those colours as the 8bit index as they already are, but keep using the rgb functions. So the color_idx_to_rgb is moved into the function calls. After all, those are fixed colours.

And I also noticed that in the SDL2 system it still has SDL_INIT_NOPARCHUTE, this is useless in SDL2 as it's by default.
http://wiki.libsdl.org/MigrationGuide#Some_general_truths (3rd paragraph)

Edit:
It almost works, if it was not for those colours in environment.cc

An_dz

It's done.

The patch is finally complete, it works perfectly for all systems as all colour stuff is only called after simgraph_init().

I decided that there's no need to keep the fixed colours as PIXVAL, after all they are fixed. The whole idea of the project was to have good colours for themes and not fixed colours. Time for a review of the whole thing:

Does it bring anything to pak maintainers?
Yes, at least if they are working with themes. Now the colours defined in the themes.tab will be much closer to what will show in the game. With the current buggy code the colours are confined to 224 colours, which is a subset of the 8bit palette. So even if you define full black (0x000000) in the theme you only get brown.

Does it bring anything to users?
Of course, as I said above now themes can have real colours and not only 224. This will give better themes to the users.

Does it bring any incompatibilities?
No, current paksets and themes will continue to work, though themes will look better. You can still use hex colours or the 8 bit index numbers, both still work. The settings file will be automatically updated.

There are though, two parameters that have died and two that took their place.

front_window_bar_color & bottom_window_bar_color are obsolete now. They won't be read any more, as their info is useless with this patch.

Instead now you will be using bottom_window_darkness, this will define the percentage of black over the titlebar of background windows. Using 0 will give no effect at all, while 100 will just make the titlebar fully black. You can use any value between those values, though the values 25, 50 & 75 have a code shortcut and execute faster (though it may not be so significant).

And the other new parameter is default_window_title_color, this one won't change the highlight of the titlebar, instead it defines the colour of the titlebar for windows that don't have special titlebar colours. So now the theme can define which will be that colour, which for years has been orange. Finance windows and other windows that show the player colour will still display the titlebar in the player colour, but now you can define the default colour of the titlebar. I know Leartin will like it ;D

Does it bring anything else to the code?
It will work even with 24bit or 32bit colours, RGB, BGR, ARGB or any other combination. The functions are already done in a way they won't need change.

Quirks of the code
- COLOR_VAL is dead, colours are now PIXVAL which is the colour in the system format. Currently as uint16.
- PLAYER_COLOR_VAL has become FLAGGED_PIXVAL, as the previous name made no sense after the introduction of transparency flags. It's uint32 and it's simply a PIXVAL with flags in the upper bits [16~23] 0x00FF0000
- Some colours in simcolor.h are in indexed format, while others are already as color_idx_to_rgb(X), this is to make sure colours defined as globals work as one can't have the system format while the graphic system has started (at least on SDL1)
- All functions are now their _rgb versions, the non-rgb version were removed as they are useless.
- The colours that are saved in settings.xml are saved as RGB888, this is to make the file compatible between systems. Saving them as the system format would kill the portability of the user settings. That's why those are duplicated, one that holds the colour as the system format and another as RGB888, identified with the _rgb suffix.
- The simuconf reader had a non-sense close() call. Functions should not close files they don't open, it also brought problems with my code as now we read simuconf again only for the colours when the graphic system has already started.
- Because with RGB you can't use sums to change brightness or lightness I make use of display_blend.

https://github.com/An-dz/simutrans
http://simutrans-germany.com/files/upload/colour16bit.txt

Dwachs

color_idx_to_rgb is missing here: (player income messages)

diff --git a/simutrans/trunk/player/simplay.cc b/simutrans/trunk/player/simplay.cc
index a5d019e..b9eae6a 100644
--- a/simutrans/trunk/player/simplay.cc
+++ b/simutrans/trunk/player/simplay.cc
@@ -226,7 +226,7 @@ void player_t::display_messages()

                const scr_coord scr_pos = vp->get_screen_coord(koord3d(m->pos,welt->lookup_hgt(m->pos)),koord(0,m->alter >> 4));

-               display_shadow_proportional( scr_pos.x, scr_pos.y, PLAYER_FLAG|(player_color_1+3), COL_BLACK, m->str, true);
+               display_shadow_proportional_rgb( scr_pos.x, scr_pos.y, PLAYER_FLAG|(color_idx_to_rgb(player_color_1+3)), color_idx_to_rgb(COL_BLACK), m->str, true);
                if(  m->pos.x < 3  ||  m->pos.y < 3  ) {
                        // very close to border => renew background
                        welt->set_background_dirty();


Can the routines get_player_color* be changed to return rgb colors? This might remove code as the call to color_idx_to_rgb happens inside the get-color routine.
Parsley, sage, rosemary, and maggikraut.

An_dz

#31
I can't see where it's missing.

Anyway I'll check the get_player_color*. I left it untouched as I had some problems before and I think this whole code could be later replaced by a better system to allow more colours.

Edit:
I just checked and remembered the problems, first was some problems with passing arguments to the functions (to choose brightness / replace the + n), something about warnings and errors (Could probably be fixed).

But the big problem is that in some places the index is needed:
gui\kennfarbe.cc:55~116
dataobj\settings.cc:1580:1641

Dwachs

Sorry, the diff in my post is the diff between your patch and my modifications :)  There was color_idx_to_rgb missing, just check the player income messages on the screen: they drawn in black not in player color.
Parsley, sage, rosemary, and maggikraut.

An_dz

Ah thought it was mine. Yes now I've found it. Thanks!

I've also removed a useless sum in factory_chart, it's on GitHub.

An_dz