Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bad video with icculus.org quake2 + ref_softsdl #216

Open
sezero opened this issue Sep 21, 2022 · 10 comments
Open

bad video with icculus.org quake2 + ref_softsdl #216

sezero opened this issue Sep 21, 2022 · 10 comments
Assignees

Comments

@sezero
Copy link
Contributor

sezero commented Sep 21, 2022

Quake2 built from http://svn.icculus.org/quake2/trunk with only SDL options
enabled in Makefile and run with ./quake2 +set vid_ref softsdl gives funky
colors in video when run against sdl12-compat, like bad palette usage. Real
SDL1.2 is OK. (Note that the sdlquake binary coredumps, so use the quake2
binary.) Curiously the pcx screenshot taken with F12 key looks OK, therefore
the attached shot is taken using Gnome functionalities.

Screenshot

@sezero
Copy link
Contributor Author

sezero commented Sep 22, 2022

Changing SDL_SetPalette to SDL_SetColors fixes display, like:

Index: src/linux/rw_sdl.c
===================================================================
--- src/linux/rw_sdl.c	(revision 205)
+++ src/linux/rw_sdl.c	(working copy)
@@ -747,7 +747,7 @@ void SWimp_SetPalette( const unsigned ch
 		colors[i].b = palette[i*4+2];
 	}
 
-	SDL_SetPalette(surface, sdl_palettemode, colors, 0, 256);
+	SDL_SetColors(surface, colors, 0, 256);
 }
 #endif
 

SDL_SetColors itself calls SDL_SetPalette with SDL12_LOGPAL | SDL12_PHYSPAL:
https://github.com/libsdl-org/sdl12-compat/blob/main/src/SDL12_compat.c#L7068

On the other hand, rw_sdl.c sets sdl_palettemode to 0x1, i.e. SDL12_LOGPAL,
so, calling SDL_SetPalette there only calls first the SDL20_SetPaletteColors
with palette20, but not the second one with VideoPhysicalPalette20:
https://github.com/libsdl-org/sdl12-compat/blob/main/src/SDL12_compat.c#L7043

The result is an incompatibility with real SDL-1.2.

@sezero
Copy link
Contributor Author

sezero commented Sep 22, 2022

This is possibly a bug in rw_sdl.c -- can't tell now exactly, tired, will sleep.
@icculus: What can you see in there?

@slouken
Copy link
Collaborator

slouken commented Sep 22, 2022

This seems like it ought to work. What quake is doing is setting the palette on the surface, but only the logical palette, not the physical palette, because the display isn't 8-bit. Then presumably it does a blit from its internal surface to the screen, which should use the palette it just set.

@slouken slouken added this to the 1.2.60 milestone Sep 22, 2022
@icculus
Copy link
Collaborator

icculus commented Sep 22, 2022

I'm actually surprised we haven't seen more paletted games, that a bug like this has lived this long. I'll look at it.

@icculus
Copy link
Collaborator

icculus commented Sep 23, 2022

So this only uses SDL_LOGPAL because earlier they called...

vinfo = SDL_GetVideoInfo();
sdl_palettemode = (vinfo->vfmt->BitsPerPixel == 8) ? (SDL_PHYSPAL|SDL_LOGPAL) : SDL_LOGPAL;

And since the display isn't 8-bit, they don't try to set the physical palette.

But they call SDL_SetVideoMode(bpp=8), so we give them an 8-bit surface as requested.

So I'm thinking the problem here is that we try to simulate a physical palette at all, whereas SDL-1.2 is (probably) not offering a hardware palette on x11 (or maybe anywhere on modern machines).

So the solution is probably just to remove any code that references VideoPhysicalPalette20.

(indeed, commenting out this line in SDL_UpdateRects...

surface12->surface20->format->palette = VideoPhysicalPalette20;

...fixes quake2.)

@sezero
Copy link
Contributor Author

sezero commented Sep 23, 2022

Hmm. Doing that does seem to fix it. However, regardless of that change, changing q2 to use SDL_SetColors instead, results in lots of flicker when effects are applied to screen, e.g. when using quad or when taking damage, which most probably is q2's fault.

@icculus
Copy link
Collaborator

icculus commented Sep 23, 2022

changing q2 to use SDL_SetColors instead, results in lots of flicker when effects are applied to screen

That's probably this code in sdl12-compat's SDL_SetPalette, which gets called by SDL_SetColors with SDL_LOGPAL|SDL_PHYSPAL:

    if ((surface12 == VideoSurface12) && (flags & SDL12_PHYSPAL)) {
        SDL_UpdateRect(surface12, 0, 0, 0, 0);   /* force screen to reblit with new palette. */
    }

We basically end up uploading the surface twice...the theory is this will work if the game is not calling UpdateRects() but just changing the physical palette to cause some sort of animation effect.

This code should probably go away with the fake hardware palette. I'd have to see what happens on real SDL-1.2, it's possible this trick I described doesn't work there anyhow.

@icculus
Copy link
Collaborator

icculus commented Sep 23, 2022

Yeah, most places in SDL-1.2 wouldn't ever offer a hardware palette anyhow, so it's best to just dump that code, which I now have.

@sezero
Copy link
Contributor Author

sezero commented Sep 23, 2022

changing q2 to use SDL_SetColors instead, results in lots of flicker when effects are applied to screen

That's probably this code in sdl12-compat's SDL_SetPalette, which gets called by SDL_SetColors with SDL_LOGPAL|SDL_PHYSPAL:

    if ((surface12 == VideoSurface12) && (flags & SDL12_PHYSPAL)) {
        SDL_UpdateRect(surface12, 0, 0, 0, 0);   /* force screen to reblit with new palette. */
    }

We basically end up uploading the surface twice...the theory is this will work if the game is not calling UpdateRects() but just changing the physical palette to cause some sort of animation effect.

The bad thing is, the same happens with real SDL-1.2. sigh...

I had changed our private port of Sin to use SDL_SetColors, now I will revert it.

@icculus
Copy link
Collaborator

icculus commented Oct 14, 2022

Obviously this fix broke a bunch of other things, so I reverted it and need to investigate this more.

@icculus icculus reopened this Oct 14, 2022
@icculus icculus modified the milestones: 1.2.60, 1.2.64 Nov 19, 2022
@icculus icculus removed this from the 1.2.64 milestone May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants