FIXME cleanup

[icculus]

There are a bunch of FIXME()s that it'd be nice to get to: even if we don't get to fix them, it'd be nice to have some of them only trigger when they're more likely to indicate an actual problem with the running application. Now that FIXMEs are disabled by default, it's not as confusing as it could be, though.

Originally posted by @sulix in #53 (comment)

[icculus]

Putting this in the 1.2.50 milestone, but once we've made a pass over all the FIXMEs (many of them extremely aggressive wishlist stuff that may never get changed), we should close this bug, even if most of the FIXMEs remain.

[sezero]

PR #145 is in. Anything left?

[sulix]

There are still a whole bunch of FIXMEs left. I don't think we need to get all of them, but PR #145 only covered the few easy ones I got to over the weekend.

Here are my notes on the others. I don't expect to have time to get to these soon, and I think many of them we probably can ignore for now. Any thoughts?

Feature Requests:

• FIXME("Should we offer scaling for windowed modes, too?"); — Probably not for version 1
• FIXME("currently ignores SDL_WINDOWID, which we could use with SDL_CreateWindowFrom ...?"); — Definitely should support this at some point, but probably could be after v1.
• FIXME("handle SDL_ANYFORMAT"); — This seems like an optimisation, rather than something that'd break apps?
• FIXME("support SDL_INIT_EVENTTHREAD where it makes sense?"); — Didn't look into this, but from "Thread" in the name, is probably nasty.
• FIXME("there is never a parachute in SDL2, should we catch segfaults ourselves?"); — Not in v1, I think.

FIXME("Does this need to call Init12Video?");

• In SDL_VideoInit()
• Someone probably should

FIXME("In 1.2, this only fails (-1) if you haven't SDL_Init()'d.");

• In SDL_WaitEvent()
• It'd be trivial to make this fail if SDL_Init() isn't called, but I pity the application which relies on it.

Keyboard mapping gaps:

• We probably should look into fixing these, but I think some of them are nastily platform×layout specific, so probably need people with the appropriate setups to find the mappings and test.
• FIXME("nothing maps to SDLK12_COMPOSE, SDLK12_BREAK, or SDLK12_EURO ...?");
• FIXME("nothing maps to SDLK12_BREAK, or SDLK12_EURO ...?");
• FIXME("map some of the SDLK12_WORLD keys");

FIXME("we should probably drop a lot of these events.");

• For resize events when there's no window or we're in fullscreen desktop
• I think these are already all being dropped, albeit we probably should "return 1" here instead of "break"?

FIXME("bgr instead of rgb?");

• There are a bunch of these scattered about. I'm not 100% sure what these are referring to, but if they've broken anything so far, I haven't noticed. It'd be nice to remove these if they aren't necessary, since there are a lot of them…

FIXME("Should we force a compatibility context here?");

• This is about OpenGL contexts.
• I feel we probably shouldn't, and I think most platforms default to a compatibility context anyway. I'd guess SDL 1.2's behaviour would be just to use the platform default.

FIXME("setup screen saver");

• There's some code for enabling/disabling the screensaver based on the SDL_VIDEO_ALLOW_SCREENSAVER environment variable that's commented out.
• Seems like it's not needed, though, as SDL2 already disables the screensaver by default, and respects that hint.
• Can we just delete this, then?

FIXME("don't do anything if the window's dimensions, etc haven't changed.");
FIXME("we need to preserve VideoSurface12 (but not its pixels), I think...");

• This refers to calling SDL_SetVideoMode() repeatedly with the same resolution, etc.
• I think we already match SDL 1.2's visible behaviour here, but do end up doing a lot on needless work.
• Probably something we can leave to a later version.

Dest Alpha Stuff:

• A bunch of FIXMEs here
• SaveDestAlpha: FIXME("This should only save the dst rect in use"); — I think this is an optimisation: we're saving and restoring more than we need to.
• FIXME("Unhandled dest alpha"); — We only support a couple of different pixel formats.
• FIXME("this could be SIMD'd"); — Optimisation opportunity.

FIXME("is it legal to display multiple yuv overlays?");

• Even if it is, I don't know of anything using it.
• Probably can slip past v1.

FIXME("Support non-default delay and interval for Key Repeat");

• I'm not aware of anything that really depends on specific key repeat delay/intervals, so think this could probably wait.
• (It's a bit of a pain to implement too)
• There are also some issues with how Key Repeat would interact with TEXTINPUT events if the repeat rate doesn't match the system default. I suspect there are a few subtle dragons here.

FIXME("wrap surface");

• Something in SDL_SaveBMP_RW — not sure what this means.

Bunch of CDDA issues:

• I think these are nominally still tracked in cdrom code #32
• There seems to be a bit of work still here to make this robust, but it's a relatively rarely used feature which needs to be manually enabled by the user.
• FIXME("Is subsystem init reference counted in SDL 1.2?");
• FIXME("Can we do something more robust than this for directory enumeration?");
• FIXME("Maybe just mark it stopped?");
• FIXME("if this fails, we need to deal with app callback or cd-rom no longer working");
• FIXME("deal with failure in here");

FIXME("Respect 1.2 environment variables for defining format here.");

• This comes from SDL_OpenAudio()
• I wasn't able to find any such environment varibles in 1.2, so am not sure what this is referring to?
• Either delete the FIXME(), or punt it to a later version?

Misc OpenAudio() FIXMEs:

• Largely related to the cdrom stuff above
• FIXME("Cleanup from failures in here");
• FIXME("Close audio device if nothing else was using it");

[sezero]

And 4601def added two new FIXMEs. Heh he :)

[sezero]

There is also an /* !!! IMPLEMENT_ME X11_KeyToUnicode ? */ note at the top...

[icculus]

IIRC, there was a Loki game (I forget which), which uses X11_KeyToUnicode, which was an internal function in the X11 backend. At the time, it was non-trivial to mark non-static symbols in shared libraries as private, so Loki used one of them they shouldn't have.

[icculus]

...I want to say it was Heavy Gear II, but I could be wrong.

[icculus]

Actually, it appears to be the Torque Engine having this issue in more modern times (although perhaps they inherited this from Tribes 2?)...this means Space Pirates and Zombies, on Steam, will need this symbol exported, if nothing else.

We should open a bug for this.

[icculus]

FIXME("there is never a parachute in SDL2, should we catch segfaults ourselves?"); — Not in v1, I think.

So the primary reason for the parachute was to reset the video mode if an app crashes, so you didn't end up with a 320x200 desktop or whatever on X11...but this is not an issue with FULLSCREEN_DESKTOP, so the only case where we could hit this now is if the user has forced off OpenGL Scaling in a GL app.

(Actually, this might have been an XVidMode issue we don't get with XRandR...?)

[icculus]

FIXME("Respect 1.2 environment variables for defining format here.");

• This comes from SDL_OpenAudio()
• I wasn't able to find any such environment varibles in 1.2, so am not sure what this is referring to?
• Either delete the FIXME(), or punt it to a later version?

It's SDL_AUDIO_FREQUENCY, SDL_AUDIO_FORMAT, SDL_AUDIO_CHANNELS, and SDL_AUDIO_SAMPLES, in 1.2's SDL_OpenAudio implementation.

I don't think these are super-important, as only an end-user would set them, and it's not something you hear a lot about in real life.

[icculus]

Just stealing @sulix's comment to turn it into a checklist...

Feature Requests:

• FIXME("Should we offer scaling for windowed modes, too?"); — Probably not for version 1
• FIXME("currently ignores SDL_WINDOWID, which we could use with SDL_CreateWindowFrom ...?"); — Definitely should support this at some point, but probably could be after v1.
• FIXME("handle SDL_ANYFORMAT"); — This seems like an optimisation, rather than something that'd break apps?
• FIXME("support SDL_INIT_EVENTTHREAD where it makes sense?"); — Didn't look into this, but from "Thread" in the name, is probably nasty.
• FIXME("there is never a parachute in SDL2, should we catch segfaults ourselves?"); — Not in v1, I think.
• FIXME("Does this need to call Init12Video?"); in SDL_VideoInit()...Someone probably should
• FIXME("In 1.2, this only fails (-1) if you haven't SDL_Init()'d."); in SDL_WaitEvent()...It'd be trivial to make this fail if SDL_Init() isn't called, but I pity the application which relies on it.

Keyboard mapping gaps:

We probably should look into fixing these, but I think some of them are nastily platform×layout specific, so probably need people with the appropriate setups to find the mappings and test.

• FIXME("nothing maps to SDLK12_COMPOSE, SDLK12_BREAK, or SDLK12_EURO ...?");
• FIXME("nothing maps to SDLK12_BREAK, or SDLK12_EURO ...?");
• FIXME("map some of the SDLK12_WORLD keys");
• FIXME("we should probably drop a lot of these events."); for resize events when there's no window or we're in fullscreen desktop...I think these are already all being dropped, albeit we probably should "return 1" here instead of "break"?
• FIXME("Support non-default delay and interval for Key Repeat"); ...I'm not aware of anything that really depends on specific key repeat delay/intervals, so think this could probably wait. (It's a bit of a pain to implement too.) There are also some issues with how Key Repeat would interact with TEXTINPUT events if the repeat rate doesn't match the system default. I suspect there are a few subtle dragons here.

Various video stuff

• FIXME("bgr instead of rgb?"); ...there are a bunch of these scattered about. I'm not 100% sure what these are referring to, but if they've broken anything so far, I haven't noticed. It'd be nice to remove these if they aren't necessary, since there are a lot of them...
• FIXME("Should we force a compatibility context here?"); ...this is about OpenGL contexts. I feel we probably shouldn't, and I think most platforms default to a compatibility context anyway. I'd guess SDL 1.2's behaviour would be just to use the platform default.
• FIXME("setup screen saver"); ...There's some code for enabling/disabling the screensaver based on the SDL_VIDEO_ALLOW_SCREENSAVER environment variable that's commented out. Seems like it's not needed, though, as SDL2 already disables the screensaver by default, and respects that hint. Can we just delete this, then?
• FIXME("don't do anything if the window's dimensions, etc haven't changed.");
• FIXME("we need to preserve VideoSurface12 (but not its pixels), I think..."); ...this refers to calling SDL_SetVideoMode() repeatedly with the same resolution, etc. I think we already match SDL 1.2's visible behaviour here, but do end up doing a lot on needless work. Probably something we can leave to a later version.

Dest Alpha Stuff:

A bunch of FIXMEs here

• SaveDestAlpha: FIXME("This should only save the dst rect in use"); — I think this is an optimisation: we're saving and restoring more than we need to.
• FIXME("Unhandled dest alpha"); — We only support a couple of different pixel formats.
• FIXME("this could be SIMD'd"); — Optimisation opportunity.
• FIXME("is it legal to display multiple yuv overlays?"); ...Even if it is, I don't know of anything using it. Probably can slip past v1.
• FIXME("wrap surface"); ... Something in SDL_SaveBMP_RW — not sure what this means.

CDDA issues:

I think these are nominally still tracked in #32

There seems to be a bit of work still here to make this robust, but it's a relatively rarely used feature which needs to be manually enabled by the user.

• FIXME("Is subsystem init reference counted in SDL 1.2?");
• FIXME("Can we do something more robust than this for directory enumeration?");
• FIXME("Maybe just mark it stopped?");
• FIXME("if this fails, we need to deal with app callback or cd-rom no longer working");
• FIXME("deal with failure in here");

Misc OpenAudio() FIXMEs:

Largely related to the cdrom stuff above

• FIXME("Respect 1.2 environment variables for defining format here."); ...this comes from SDL_OpenAudio(). I wasn't able to find any such environment varibles in 1.2, so am not sure what this is referring to? Either delete the FIXME(), or punt it to a later version?
• FIXME("Cleanup from failures in here");
• FIXME("Close audio device if nothing else was using it");

[icculus]

FIXME("bgr instead of rgb?"); ...there are a bunch of these scattered about.

I was wondering if it would have been better for modern GPUs to use BGR color order instead of RGB. This was true in the early 2000's, but it honestly probably doesn't matter in modern times, especially for this particular workload. We should probably just remove these and not think more about it.

[icculus]

I'm inclined to say the rest of these shouldn't be changed for 1.2.50, as a lot are wishlist stuff and corner cases, and it's not worth destabilizing things just for the sake of removing them.

[icculus]

I'm putting this in the 1.2.54 milestone, but let's all agree that some of these should be fixed for completeness's sake and some we should just agree will probably live as FIXMEs forever.

[icculus]

FIXME("currently ignores SDL_WINDOWID, which we could use with SDL_CreateWindowFrom ...?"); — Definitely should support this at some point, but probably could be after v1.

If anyone is bored and wants to run with it, there's an extremely basic patch and test case sitting in #192 now.

[icculus]

FIXME("there is never a parachute in SDL2, should we catch segfaults ourselves?"); — Not in v1, I think.

@slouken SDL_INIT_PARACHUTE was in 1.2 mostly to reset the video mode in case of a segfault, right? Now that we're free of XVidMode, we probably don't need this on modern systems (and SDL2 doesn't offer this at all anyhow). Do you agree, or was there some case where this was still important? Trying to decide if I should wire up a signal handler in sdl12-compat or just remove the FIXME.

[slouken]

FIXME("there is never a parachute in SDL2, should we catch segfaults ourselves?"); — Not in v1, I think.

@slouken SDL_INIT_PARACHUTE was in 1.2 mostly to reset the video mode in case of a segfault, right? Now that we're free of XVidMode, we probably don't need this on modern systems (and SDL2 doesn't offer this at all anyhow). Do you agree, or was there some case where this was still important? Trying to decide if I should wire up a signal handler in sdl12-compat or just remove the FIXME.

Yep, we might still need it for KMSDRM, but otherwise we shouldn't need it anymore.

[icculus]

Okay, I implemented logical scaling for windowed modes in bf38589. Here's my 4K monitor on x11 with testsprite at normal size, at 1.5x, and 2.0x.

Or, if you prefer OpenGL apps, here's UT2004:

This will refuse to scale for resizable windows (under the assumption that they know how to handle arbitrary sizes already).

I mention this here instead of quietly clicking the checkbox because this feels like it might have corner cases I haven't considered. @sulix, any opinions on this one (including "this is a terrible idea, back this change out")?

[sulix]

Neat! I can't think of any particular corner-cases which could cause problems. There are maybe a few extra features / interactions we could add / change (do we always advertise HIGHDPI support now? do we make this add scaled "fullscreen" modes?, would it make sense to support this for resizable windows as well, in case of small text on high-DPI screens?) but I think the actual implementation should be pretty robust. Fractional scaling can get hairy, but since this only computes the final size of the window once, it should be as close to perfect as that can get, as far as I can tell.

I played around with a few different programs, and it seems to work well for me on all of them so far.

[icculus]

(I put the SDL_WINDOWID patch in, which is probably going to generate a bug report later when it doesn't work exactly right, but it's better than nothing.)

[icculus]

So making key repeat work like SDL 1.2 expects is surprisingly little code. I set out to add a timer that deals with lists of currently-pressed keys, but in practice 1.2 is less complex than that.

However, there's one little issue here:

sdl12-compat/src/SDL12_compat.c

Lines 2363 to 2396 in ebfcc6e

	case SDL_TEXTINPUT:
	{
	char *text = event20->text.text;
	int codePoint = 0;
	while ((codePoint = DecodeUTF8Char(&text)) != 0)
	{
	if (codePoint > 0xFFFF) {
	/* We need to send a UTF-16 surrogate pair. */
	Uint16 firstChar = ((codePoint - 0x10000) >> 10) + 0xD800;
	Uint16 secondChar = ((codePoint - 0x10000) & 0x3FF) + 0xDC00;
	event12.type = SDL12_KEYDOWN;
	event12.key.state = SDL12_PRESSED;
	event12.key.keysym.scancode = 0;
	event12.key.keysym.sym = SDLK12_UNKNOWN;
	event12.key.keysym.unicode = firstChar;
	if (!FlushPendingKeydownEvent(firstChar))
	PushEventIfNotFiltered(&event12);
	event12.key.keysym.unicode = secondChar;
	PushEventIfNotFiltered(&event12);
	}
	else
	{
	if (!FlushPendingKeydownEvent(codePoint)) {
	event12.type = SDL12_KEYDOWN;
	event12.key.state = SDL12_PRESSED;
	event12.key.keysym.scancode = 0;
	event12.key.keysym.sym = SDLK12_UNKNOWN;
	event12.key.keysym.unicode = codePoint;
	PushEventIfNotFiltered(&event12);
	}
	}
	}
	}
	return 1;

Basically SDL2 key repeat (or rather, I guess, operating system key repeat?) will keep generating TEXTINPUT events as you hold down most keys, and if sdl12-compat can't match the repeated input to a pending KEYDOWN event, it just pushes a new 1.2 KEYDOWN event.

The end result of this is that we end up with two competing key repeat mechanisms, and one that can't be controlled by SDL_EnableKeyRepeat.

How crucial is this code? I assume the careful tapdance to match TEXTINPUT to KEYDOWN events is extremely delicate, but if this is just here as a "maybe we missed it, send a keydown just in case" sort of thing, then can we remove the code that generates an event if FlushPendingKeydownEvent returns false?

[icculus]

There are also some issues with how Key Repeat would interact with TEXTINPUT events if the repeat rate doesn't match the system default. I suspect there are a few subtle dragons here.

(and of course, @sulix warned about this in the original TODO list. :) )

[icculus]

I took a flamethrower to it.

We can revisit (and revert!) if this proves to be a problem, but this code looked like it was trying to do more-correct Unicode support, and SDL 1.2 did not do anything correct at all about it.

[icculus]

FIXME("we should probably drop a lot of these events.");

This got fixed at some point, checking it off the list.

[icculus]

Holy moly, all the FIXMEs are gone. :O

[slouken]

Woo! Congrats! :)