libretro-scummvm has a fix for cursor palette corruption in myst/11th hour

[i30817]

https://github.com/libretro/scummvm/issues/194#issue-1150056614

However, @diablodiab , there is another problem in 11th hour at least (maybe because that cursor is animated) where the transparency is not happening.

Also the fonts in the screenshot i took:

https://user-images.githubusercontent.com/757900/155861160-eb42f40d-18b6-421d-895e-d369034b4719.png

from the demo: https://archive.org/download/11th_Hour_demo/11th.iso

[50J0uRNeR]

I submitted a patch to the libretro/scummvm project that fixes an issue with 32bit cursors, however this project uses a newer version of Scummvm which has changed how it uses 32bit cursors and the KeyColor. In the older version the alpha channel values seemed to be random and unusable, so all engines appear to simply use the ColorKey. This newer version of Scummvm seems to utilize the alpha channel of 32bit cursors and use a unused value for the ColorKey (with 11th Hour at least). Here's a patch that still uses the ColorKey but also blends the cursor to the screen Surface using the alpha channel for 32bit cursors. It also fixes an issue with the blitting functions masking 0xFFFFFFFF pixels (I have no idea why they are doing this?):

--- orig/backends/platform/libretro/libretro_os.cpp     2022-03-03 21:58:05.676528342 -0500
+++ patched/backends/platform/libretro/libretro_os.cpp  2022-03-05 03:36:50.643823827 -0500
@@ -127,7 +127,7 @@
          uint8 r, g, b;

          const uint8_t val = in[j];
-         if(val != 0xFFFFFFFF)
+         //if(val != 0xFFFFFFFF)
          {
             if(aIn.format.bytesPerPixel == 1)
             {
@@ -162,8 +162,8 @@

          uint8 r, g, b;

-         const uint32_t val = in[j];
-         if(val != 0xFFFFFFFF)
+         //const uint32_t val = in[j];
+         //if(val != 0xFFFFFFFF)
          {
             aIn.format.colorToRGB(in[j], r, g, b);
             out[j] = aOut.format.RGBToColor(r, g, b);
@@ -189,8 +189,8 @@

          uint8 r, g, b;

-         const uint16_t val = in[j];
-         if(val != 0xFFFFFFFF)
+         //const uint16_t val = in[j];
+         //if(val != 0xFFFFFFFF)
          {
             aIn.format.colorToRGB(in[j], r, g, b);
             out[j] = aOut.format.RGBToColor(r, g, b);
@@ -256,6 +256,45 @@
    }
 }

+static void blit_uint32_uint16(Graphics::Surface& aOut, const Graphics::Surface& aIn, int aX, int aY, const RetroPalette& aColors, uint32 aKeyColor)
+{
+   for(int i = 0; i < aIn.h; i ++)
+   {
+      if((i + aY) < 0 || (i + aY) >= aOut.h)
+         continue;
+
+      uint32_t* const in = (uint32_t*)aIn.pixels + (i * aIn.w);
+      uint16_t* const out = (uint16_t*)aOut.pixels + ((i + aY) * aOut.w);
+
+      for(int j = 0; j < aIn.w; j ++)
+      {
+         if((j + aX) < 0 || (j + aX) >= aOut.w)
+            continue;
+
+         uint8 in_a, in_r, in_g, in_b;
+         uint8 out_r, out_g, out_b;
+         uint32_t blend_r, blend_g, blend_b;
+
+         const uint32_t val = in[j];
+         if(val != aKeyColor)
+         {
+            aIn.format.colorToARGB(in[j], in_a, in_r, in_g, in_b);
+
+            if(in_a)
+            {
+               aOut.format.colorToRGB(out[j + aX], out_r, out_g, out_b);
+
+               blend_r = ((in_r * in_a) + (out_r * (255 - in_a))) / 255;
+               blend_g = ((in_g * in_a) + (out_g * (255 - in_a))) / 255;
+               blend_b = ((in_b * in_a) + (out_b * (255 - in_a))) / 255;
+
+               out[j + aX] = aOut.format.RGBToColor(blend_r, blend_g, blend_b);
+            }
+         }
+      }
+   }
+}
+
 static INLINE void copyRectToSurface(uint8_t *pixels, int out_pitch, const uint8_t *src, int pitch, int x, int y, int w, int h, int out_bpp)
 {
    uint8_t *dst = pixels + y * out_pitch + x * out_bpp;
@@ -520,10 +559,19 @@
             const int x = _mouseX - _mouseHotspotX;
             const int y = _mouseY - _mouseHotspotY;

-            if(_mouseImage.format.bytesPerPixel == 1)
-               blit_uint8_uint16(_screen, _mouseImage, x, y, _mousePaletteEnabled ? _mousePalette : _gamePalette, _mouseKeyColor);
-            else
-               blit_uint16_uint16(_screen, _mouseImage, x, y, _mousePaletteEnabled ? _mousePalette : _gamePalette, _mouseKeyColor);
+            switch(_mouseImage.format.bytesPerPixel)
+            {
+               case 1:
+               case 3:
+                  blit_uint8_uint16(_screen, _mouseImage, x, y, _mousePaletteEnabled ? _mousePalette : _gamePalette, _mouseKeyColor);
+                  break;
+               case 2:
+                  blit_uint16_uint16(_screen, _mouseImage, x, y, _mousePaletteEnabled ? _mousePalette : _gamePalette, _mouseKeyColor);
+                  break;
+               case 4:
+                  blit_uint32_uint16(_screen, _mouseImage, x, y, _mousePaletteEnabled ? _mousePalette : _gamePalette, _mouseKeyColor);
+                  break;
+            }
          }
       }

I have also added analog controller support to the retro_input_descriptor struct. Here's the patch:

--- orig/backends/platform/libretro/libretro.cpp        2022-03-03 21:58:05.656528443 -0500
+++ patched/backends/platform/libretro/libretro.cpp     2022-03-03 23:06:42.533996415 -0500
@@ -356,6 +356,10 @@
       { 0, RETRO_DEVICE_JOYPAD, 0, RETRO_DEVICE_ID_JOYPAD_SELECT, "F1" },
       { 0, RETRO_DEVICE_MOUSE,  0, RETRO_DEVICE_ID_MOUSE_LEFT,    "Left click" },
       { 0, RETRO_DEVICE_MOUSE,  0, RETRO_DEVICE_ID_MOUSE_RIGHT,   "Right click" },
+      { 0, RETRO_DEVICE_ANALOG, RETRO_DEVICE_INDEX_ANALOG_LEFT,   RETRO_DEVICE_ID_ANALOG_X, "Left Analog X" },
+      { 0, RETRO_DEVICE_ANALOG, RETRO_DEVICE_INDEX_ANALOG_LEFT,   RETRO_DEVICE_ID_ANALOG_Y, "Left Analog Y" },
+      { 0, RETRO_DEVICE_ANALOG, RETRO_DEVICE_INDEX_ANALOG_RIGHT,  RETRO_DEVICE_ID_ANALOG_X, "Right Analog X" },
+      { 0, RETRO_DEVICE_ANALOG, RETRO_DEVICE_INDEX_ANALOG_RIGHT,  RETRO_DEVICE_ID_ANALOG_Y, "Right Analog Y" },
       { 0 },
   };

I can't be sure these patches will work for all games. Since there's no way to know whether a game is using the keycolor or the alpha channel, it is at least possible that a game will set a valid keycolor but return 0 for the alpha making the cursor invisible. At least for now I can say this works for Myst, Riven, 7th Guest and 11th Hour.

[i30817]

The first patch has two errors. The first is on: ../../../../backends/platform/libretro/libretro_os.cpp:259:141: error: expected ‘,’ or ‘...’ before ‘>’ token

Which appears to be > instead of ')'

The other is a uninitialized variable from apparently deleting a argument but still using it:

../../../../backends/platform/libretro/libretro_os.cpp:279:20: error: ‘aKeyColor’ was not declared in this scope 279 | if(val != aKeyColor)

I think it's just a copy paste error on the variable name.

[i30817]

Everything works besides.

BTW, the masking of 0xFFFFFFFF caused the font problems in 11th hour.

[i30817]

Do you want to do a PR @50J0uRNeR , or wait for @diablodiab to apply this manually? A PR would get you credit in your account in github, which could be useful if you ever want to submit it as part of your curriculum vitae.

[50J0uRNeR]

Sorry, it was because of how I pasted it into the browser. I've fixed the original post.

Thanks @i30817 but I'm not concerned about credit. I'd rather wait and see if we get more discussion and testing here. I'm really concerned about removing the 0xFFFFFFFF masking code and whether someone else knows why that was there. I'm also hoping someone else knows a way to determine whether an engine is using alpha or the keycolor (right now I'm using both if it is a 32bit cursor). This patch needs to be tested on a wider variety of games.

[i30817]

From git blame, it kind of appears to have been a refactor by twinaphex during a conversion from C++ to C. Look here:

https://github.com/libretro/scummvm/commit/65c1a0e4224cae1a0430c3f6d827924c0921ea7c

On that commit a C++ template 'blit' function is turned into c. 0xFFFFFFFF was a argument to some calls of blit used in const Graphics::Surface& getScreen().

Looking at the file before that commit when it was named 'os.cpp' : https://github.com/libretro/scummvm/blob/803007081abfcde8689b973a8d79fd31bb180ece/backends/platform/libretro/os.cpp

'blit' with the argument was used before drawing the mouse, and passed with argument, and the argument was dun dun dun uint32 aKeyColor

Now, why did the keycolor have to be WHITE at that time and for the image except the cursor, i have no idea, so a new git blame on that file is probably needed.

[i30817]

that use of blit is introduced in https://github.com/libretro/scummvm/commit/b5f781a1f5565a39561a3636950705407a446d3e if i'm not mistaken, and that says nothing except ' LIBRETRO: In game graphics work. (Poorly but it's a start) ' as a commit message, and it's, like, the 3rd commit.

It appears to be a filter where opaque white is not subject to palette translation or write to the out surface, so nothing appears to have changed much since the start. Obviously, it's probably wrong, but i have no clue why it was done either, from the history.

Maybe it was a misguided micro-optimization (don't copy white if the out surface starts with white?)

If it ever worked like that it obviously isn't anymore from the 11th hour font bug.

[50J0uRNeR]

That all makes sense, basically it was just overlooked or an old offscreen buffer optimization (I thought the same thing when I first saw it).

I was confused as to why the function was even called a "blit" function if it did not have a color key or mask pointer parameter. I was also confused as to why the older engine was bothering to pass a 32bit cursor without a usable alpha channel (the newer groovie engine now uses a proper alpha channel). It also doesn't make sense to mask onto the screen buffer as it causes "House of Mirrors" artifacts, masking should be done on the offscreen buffer first.

As for the cursor alpha value, I'm not sure every engine will pass a proper alpha value for 32bit cursors. As long as ALL engines either pass a usable alpha channel OR a usable color key with all alpha values at 0xFF, then my code will always work.

This patch needs to be tested on a LOT more games than I have!

[i30817]

Would you suggest that cursors are good for checking the code for immediate artifacts?

[50J0uRNeR]

Yes, any game that has a messed up 32bit cursor will be a sign that something went wrong with my patch. I did not touch the 8bit or 16bit cursor code though, so those will continue to work the same.

[i30817]

Seems to all work, i tried the first scene of many games, focusing on the windows games i had.

That said, i only found 1 other game that had that 'blue cursor' game this fixed (and it fixed it), Starship Titanic.

The longest journey is warning for some reason 'software renderer does not support modded assets' but it starts. However it's, on, you guessed, software mode, and it's much slower and cpu intensive than upstream.

I noticed that the core is generally more cpu intensive, maybe it can't use hardware rendering at all.

edit: and that tlj 'no opengl' thing is strange, because the 2.5.1 release of scummvm enabled tinygl on that engine ('Stark'). This makes it suspicious i can't choose opengl/tinygl renderer on the options of either the game or general options on the core, even if the files for tinygl are included and built by calling 'make'.

[diablodiab]

I'm not able to look into this in the next few weeks, but if I remember correctly, the 0xFFFFFFFF masking code is used by the AGS engine. If you are regression testing the patch above, try to test the two AGS versions of Kings Quest 3. I think the transparency might be used for menubar at the top of the interface in those games.

[i30817]

Which version of king's quest ags, the AGD interactive one, or the infamous adventures?

I took this image in that game and i don't appear to see any defects? It hides and shows as normal when you hover the top line.

Going to the the infamous adventures version next, although i have to download it and install first.

edit: same thing.

[i30817]

Another maybe strange maybe normal error. The free game 'the kite' on steam which is a wintermute game, starts with a black screen on opengl here but can start with vulkan.

The standalone self compiled scummvm crashes but the installed scummvm runs so i have no clue about what's the difference there except i know (from the crashes) it's a opengl shader in the wintermute engine failing (the self compiled says it can't find it). So it might be a problem between the keyboard and chair here, but working on vulkan but not on opengl might be something.

There are also a bunch of 3d wintermute games that don't start but i think that's normal (well, a 'release' from the repository installed scummvm warns that the wintermute 3d is not supported yet instead of exiting to debugger, but i believe that's just because it's a release).

[i30817]

ping, any chance of this patch being added?

There was a problem i saw with the Heroine's Quest intro (the portrait of thrivaldi appears when it shouldn't overlapped with the npc, happens just after thrivaldi does the 'fist shaking' animation midway through the intro), but that happens in the original too (just verified by calling make on the upstream).

And this bug: https://bugs.scummvm.org/ticket/12998 also in upstream, so irrelevant.

Note that both of these problems are fixed on the upstream of upstream (ags) so maybe they'll go away.

[diablodiab]

I've added both patches now

[i30817]

fixed