AGS4: Extend Allegro palette to 0-255 #2356

AlanDrake · 2024-03-11T09:27:00Z

This is a WIP attempt to increase Allegro's internal palette from 0-63 to 0-255.

EDIT2: I managed to fix the bestfit algorithm, but I'm still converting to 6bit for the rbg_map

I upgraded some pieces from the ancient demo quest, which can be used as a test project.
DemoQuest_ags4.zip

You can also import other 256 games, but they will need a full rebuild to update the palettes in game data, and manual palette manipulation that were previously restricted to 0-63 will obviously work differently.

ericoporto · 2024-07-07T01:34:04Z

I am curious about this being rebased after the recent changes.

AlanDrake · 2024-07-07T12:43:16Z

Alright, I rebased and revised this PR.
I made create_color_table and bestfit_color create a temporary 6bit palette for compatiblity with their uber haxx algorithms, which I'm not good enough to upgrade.

Should work fine both for antialiased sprites on and off.
I didn't test BMP/PCX/FLI load/save/display.

ivan-mogilko · 2024-07-21T00:04:01Z

I will try to finally review this in the following days.

I upgraded some pieces from the ancient demo quest, which can be used as a test project.

Idk if useful, but a while ago I added "test--4.0.0" branch to the old demo repository:
https://github.com/adventuregamestudio/ags-demo-quest-old/tree/test--4.0.0

ivan-mogilko · 2024-07-22T17:05:51Z

Editor/AGS.Editor/ImportExport.cs

-                writer.Write((byte)(game.Palette[i].Colour.B / 4));
+                writer.Write((byte)(game.Palette[i].Colour.R));
+                writer.Write((byte)(game.Palette[i].Colour.G));
+                writer.Write((byte)(game.Palette[i].Colour.B));


ExportCharacter272 must not be adjusted, because this exports to a old data format; and by changing palette range you are changing data specs. Better keep this as-is (maybe with some commentary).

OTOH, Export/ImportCharacter272 functions should be removed as obsolete.

ivan-mogilko · 2024-07-22T17:07:18Z

Engine/ac/room.cpp

+            if (palette[ff].g > 255)
+                palette[ff].g = 255;
+            if (palette[ff].b > 255)
+                palette[ff].b = 255;


This code chunk may be removed completely, because palette components are declared as unsigned char and can never be > 255.

ivan-mogilko · 2024-07-22T17:10:39Z

libsrc/allegro/src/allegro.c

+  { 255, 255, 255, 0 },{ 255, 0,  0,  0 },{ 0,  255, 0,  0 },{ 255, 255, 0,  0 },
+  { 0,  0,  255, 0 },{ 255, 0,  255, 0 },{ 0,  255, 255, 0 },{ 65, 65, 65, 0 },
+  { 125, 125, 125, 0 },{ 255, 125, 125, 0 },{ 125, 255, 125, 0 },{ 255, 255, 125, 0 },
+  { 125, 125, 255, 0 },{ 255, 125, 255, 0 },{ 125, 255, 255, 0 },{ 0,  0,  0,  0 }
 };


Apparently, desktop_palette may also be removed, as it's not used anywhere in our stripped allegro sources.

ivan-mogilko · 2024-07-22T17:43:52Z

Besides the in-code comments above,

Game data version (GameDataVersion enum) should be raised, because palette unit range has changed. Project version may be left, I think, as palette was saved in 256-unit range there.
For a proper behavior, there should be a palette upgrade from 64-range to 256-range on game load in the engine (UpdateGameData) if loaded a game of lower version.
"Game State" save component version should be raised, for the same reasons, as palette is written there. And palette upgraded on loading an older save.
I am bothered by temporary palette in bestfit_color, because that function is run per pixel, meaning this temporary palette will also be recreated over and over again for each processed pixel. We need to find way to optimize this, such as create a fixed palette beforehand and pass into this function instead of original palette.
I am also bothered by the potential use of rgb_map in fixup_loaded_bitmap, which I do not fully understand. But it looks like the engine only goes this path if a hi/true-color image is being converted to 8-bit image, which is unlikely to happen in practice.
There's also BestFitColor in PaletteUtilities.cs, which i ported to C# for remapping backgrounds. Likely it should have same fix?
~~Need to test FLIC playback, to see if its looks changed (but I guess the worst that may happen is that there will be more color variation?) EDIT: well, it looks fine at the first glance.~~
There's still this statement in the PR's description:

It should execute as expected, but will crash when enabling sprite antialiasing, which calls the unfinished create_rgb_table function.

Is this still true, or has been fixed?

BTW, I got confused by SetPalRGB script function. The manual states that "The RED, GREEN and BLUE parameters each range from 0 to 63". But the function itself has no parameter validation whatsoever. Git Blame shows no changes done to this function, nor to wsetrgb function in the last 12 years. Which means that this function was unsafe all this time, if user tries to pass any invalid value (negative, or >63) there, the engine will malfunction or crash.

AlanDrake · 2024-07-23T14:36:24Z

Good news.

Thanks to divine inspiration I managed to fix the bestfit algorithms for real.
Not sure if formally correct, but I finally found the weights and values to make it looks like it's working without glaring color mismatches.

Now sprites can finally match the 252-gray that used to become full white.

The rgb_table is still being wrapped to a 6bit palette. I don't have time for figuring that one. I duped the bestfit_init, so it can still generate the weights it needs.

Game data version (GameDataVersion enum) should be raised, because palette unit range has changed. Project version may be left, I think, as palette was saved in 256-unit range there.

For a proper behavior, there should be a palette upgrade from 64-range to 256-range on game load in the engine (UpdateGameData) if loaded a game of lower version.

"Game State" save component version should be raised, for the same reasons, as palette is written there. And palette upgraded on loading an older save.

Seems a waste of time, since the format is essentially unchanged and we only need a full rebuild to get everything on track.
Savestates should be expected to be unsafe on a alpha build.

I am bothered by temporary palette in bestfit_color, because that function is run per pixel, meaning this temporary palette will also be recreated over and over again for each processed pixel. We need to find way to optimize this, such as create a fixed palette beforehand and pass into this function instead of original palette.

Sounds good, but in my opinion supporting 8bit mode in the way it's implemented is a waste of time, because shaders could do all these transformations in more convenient ways.

I suppose that's what the rgb_table is supposed to optimize. In fact there are several sections where the table overtakes the individual bestfit, if initialized.

I am also bothered by the potential use of rgb_map in fixup_loaded_bitmap, which I do not fully understand. But it looks like the engine only goes this path if a hi/true-color image is being converted to 8-bit image, which is unlikely to happen in practice.

I'm bothered by all the 8bit code. I hope to never have to touch it again.

There's also BestFitColor in PaletteUtilities.cs, which i ported to C# for remapping backgrounds. Likely it should have same fix?

I managed to fix both to full 0-255 color matching.

There's still this statement in the PR's description:

Fixed

ivan-mogilko · 2024-07-23T15:00:09Z

Seems a waste of time, since the format is essentially unchanged and we only need a full rebuild to get everything on track.

Format is changed, because you switch the palette units from 64-based to 256-based, this changes the meaning of data. Without fixups this will make loading older data incorrect.
You may ignore upgrading older data in the engine, but at least the version should be incremented to indicate the change in format. This will help in case someone will like to analyze game data created by different versions.

EDIT: Fine, I will raise it myself after merging this.

ivan-mogilko · 2024-07-23T16:03:29Z

Editor/AGS.Editor/Utils/PaletteUtilities.cs

@@ -233,20 +232,20 @@ private static void RemapPixels(byte[] pixels, Color[] oldPal, Color[] newPal, b
        }

        // 1.5k lookup table for color matching
-        private static uint[] ColDiffTable = new uint[3 * 128];
+        private static uint[] ColDiffTable = new uint[3 * 512];


There's a comment about "1.5k table", now it does not reflect the table size. Should be either fixed, or removed.

ivan-mogilko · 2024-07-23T16:04:21Z

libsrc/allegro/src/color.c

@@ -261,7 +261,7 @@ int geta(int c)


 /* 1.5k lookup table for color matching */
-static unsigned int col_diff[3*128]; 
+static unsigned int col_diff[3*512]; 


Also comment about 1.5k table, same as above.

ivan-mogilko · 2024-07-23T21:27:11Z

Tested the palette remap, it seems to work fine (results are equal or close to the existing version).

ivan-mogilko · 2024-08-16T05:03:29Z

Somehow I completely missed this, but this PR did 2 changes that were not related to the palette in any way, and instead changed regular color property calculation from 16-bit R5G6B5 to 32-bit R8G8B8, which refers rather to task #1980. But since these changes are not complemented by others, on their own they break some things in both Editor and Engine.

Noteably these are changes to ColorMapper and __my_setcolor function in AGS.Native.

For example, same "Color" property numbers now produce a different RGB, so loading existing project suddenly makes colored things look different. And because __my_setcolor was only modifed in AGS.Native, but not Engine, they will also look different between runtime and the preview in the Editor.

I've already reverted the __my_setcolor back during small recent refactor, now this function is shared, but ColorMapper still has this change. I started working on #1980 now, where this change will make sense, but I am tempted to revert it temporarily, because #1980 may take time, and any public ags4 release done now will contain this discrepancy.

ericoporto · 2024-08-16T07:24:35Z

Uhm, is this also related to #2352 somehow?

ivan-mogilko · 2024-08-16T16:34:10Z

Uhm, is this also related to #2352 somehow?

This may be the related place in code, but the change itself is not related, because it was done in ags4, while the mistake is found in 3.* branch.

AlanDrake added ags 4 related to the ags4 development context: 8-bit gfx related to 8-bit games with paletted gfx labels Mar 11, 2024

AlanDrake changed the base branch from master to ags4 March 11, 2024 09:28

AlanDrake force-pushed the palette255 branch 3 times, most recently from 628232d to 31cc90a Compare July 7, 2024 12:32

AlanDrake marked this pull request as ready for review July 7, 2024 12:49

ivan-mogilko reviewed Jul 22, 2024

View reviewed changes

AlanDrake added 2 commits July 23, 2024 14:21

Extend Allegro Palettes from 0-63 to 0-255

2d0150b

Editor: Remove old character import format

375a754

AlanDrake force-pushed the palette255 branch from 31cc90a to 375a754 Compare July 23, 2024 13:44

ivan-mogilko reviewed Jul 23, 2024

View reviewed changes

ivan-mogilko merged commit 8a209de into adventuregamestudio:ags4 Jul 23, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AGS4: Extend Allegro palette to 0-255 #2356

AGS4: Extend Allegro palette to 0-255 #2356

AlanDrake commented Mar 11, 2024 •

edited

Loading

ericoporto commented Jul 7, 2024

AlanDrake commented Jul 7, 2024

ivan-mogilko commented Jul 21, 2024 •

edited

Loading

ivan-mogilko Jul 22, 2024 •

edited

Loading

ivan-mogilko Jul 22, 2024

ivan-mogilko Jul 22, 2024

ivan-mogilko commented Jul 22, 2024 •

edited

Loading

AlanDrake commented Jul 23, 2024 •

edited

Loading

ivan-mogilko commented Jul 23, 2024 •

edited

Loading

ivan-mogilko Jul 23, 2024

ivan-mogilko Jul 23, 2024

ivan-mogilko commented Jul 23, 2024

ivan-mogilko commented Aug 16, 2024 •

edited

Loading

ericoporto commented Aug 16, 2024

ivan-mogilko commented Aug 16, 2024

AGS4: Extend Allegro palette to 0-255 #2356

AGS4: Extend Allegro palette to 0-255 #2356

Conversation

AlanDrake commented Mar 11, 2024 • edited Loading

ericoporto commented Jul 7, 2024

AlanDrake commented Jul 7, 2024

ivan-mogilko commented Jul 21, 2024 • edited Loading

ivan-mogilko Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

ivan-mogilko Jul 22, 2024

Choose a reason for hiding this comment

ivan-mogilko Jul 22, 2024

Choose a reason for hiding this comment

ivan-mogilko commented Jul 22, 2024 • edited Loading

AlanDrake commented Jul 23, 2024 • edited Loading

ivan-mogilko commented Jul 23, 2024 • edited Loading

ivan-mogilko Jul 23, 2024

Choose a reason for hiding this comment

ivan-mogilko Jul 23, 2024

Choose a reason for hiding this comment

ivan-mogilko commented Jul 23, 2024

ivan-mogilko commented Aug 16, 2024 • edited Loading

ericoporto commented Aug 16, 2024

ivan-mogilko commented Aug 16, 2024

AlanDrake commented Mar 11, 2024 •

edited

Loading

ivan-mogilko commented Jul 21, 2024 •

edited

Loading

ivan-mogilko Jul 22, 2024 •

edited

Loading

ivan-mogilko commented Jul 22, 2024 •

edited

Loading

AlanDrake commented Jul 23, 2024 •

edited

Loading

ivan-mogilko commented Jul 23, 2024 •

edited

Loading

ivan-mogilko commented Aug 16, 2024 •

edited

Loading