Various of fixes for the HeadlessMode and more

[rouming]

This PR addresses various crashes in HeadlessMode and lost keystrokes caused by the game menu or incorrect event polling. These issues were observed during AI agent training, and commits targeting them are part of the RFC #7974 PR. I decided to split the original RFC on several PRs because it is unnecessarily complex and has an unclear status. This PR includes the following commits for easier review:

• Ensured keystrokes are not dropped in game menu or during progress events:
• ShowProgress now preserves non-custom SDL events, ensuring no events are lost
• Fixed crashes in HeadlessMode by avoiding SDL or audio access
• Resolved a GCC warning in logging utility by ensuring a return value in all cases

[rouming]

It seems my SDL event processing fix needs some massagging for SDL1. I'm looking into the SDL1.2 SDL_PeepEvents() implementation, which doesn't support an event type range but instead uses a 32-bit mask, and I am confused about one thing: the code clearly states that you can only have 8 custom events (here), but interfac.h defines 13 events. According to the mask check here you will never get any event picked up from the queue due to the 32-bit overflow. Therefore, all custom events starting from WM_DIABRETOWN should not be ever processed for the SDL1.2, but apparently this is not true, right? What am I missing here?

[glebm]

but apparently this is not true, right? What am I missing here?

Probably that there is no actual limitation as it's just a 32-bit integer in the end of the day, and we don't use SDL_PeepEvents anywhere currently. The masked query for SDL_PeepEvents won't work because SDL_EVENTMASK will overflow.

Let's reduce the number of custom events by putting the actual event identifier into event.user.code? This will reduce the number of custom events to just 1.

[rouming]

but apparently this is not true, right? What am I missing here?

Probably that there is no actual limitation as it's just a 32-bit integer in the end of the day, and we don't use SLD_PeepEvents anywhere currently. The masked query for SDL_PeepEvents won't work because SDL_EVENTMASK will overflow.

SDL_PollEvents calls SDL_PeepEvents with a mask set to SDL_ALLEVENTS (which is (unsigned)-1) (here), so current code should be affected. Therefore not clear how that works for events with types >= 32.

Let's reduce the number of custom events by putting the actual event identifier into event.user.code? This will reduce the number of custom events to just 1.

Right, that what I thought should be used for sdl 1.2, but there is no such code in DevilutionX, thus my confusion.

I'll try to make a few tests in order to prove / disprove that events with types >= 32 can be missed on the sdl 1.2 before going deeper in these changes.

[rouming]

Oh, it's even worse, than just missing events. C17 standard (I assume same for all others, including CPP) says the following for bit shift:

6.5.7 Bitwise shift operators

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

And here we are "lucky" that at least on x86 shl instruction (left shift) behaves as cnt mod n, where cnt is number of bits to shift and n is a width of the operand. For example from here

Also, Intel's manual[1] states that the results are undefined when cnt is greater than the operand size, but at least for 32- and 64-bit data sizes it has been observed that shift operations are performed by (cnt mod n), with n being the data size.

So on x86 regardless of bits count the value is wrapped:

#include <assert.h>

unsigned shift(unsigned bits)
{
	return 1<<bits;
}

int main(int argc, char *argv[])
{
	assert(shift(33) == (1<<(33 % 32)));
	return 0;
}

But on other platforms this is undefined and if 0 is returned (as I expected from the very beginning), then we get missing events on SDL 1.2

[StephenCWills]

It might help explain this. https://github.com/diasurgical/DevilutionX/blob/e57a2d7d54108973bc55249f4c4ccc44e475d03e/Source/interfac.cpp#L43-L48

We should just fix it as glebm described, and then I can test on 3DS.

[rouming]

It might help explain this.

https://github.com/diasurgical/DevilutionX/blob/e57a2d7d54108973bc55249f4c4ccc44e475d03e/Source/interfac.cpp#L43-L48

Great there is an issue which can be explained by this. I was scratching my head, thinking it couldn't go unnoticed.

We should just fix it as glebm described, and then I can test on 3DS.

Awesome. I will rebase then on top of your PR.

[rouming]

@StephenCWills @glebm thanks folks for helping me resolve the SDL1.2 issue. Now with only one type of custom event SDL_PeepEvents() logic becomes simpler.

Am I correct that there is no full ctest run on the github for the USE_SDL1 case?

[glebm]

There isn't one but it'd be good to add it to the existing SDL1 Linux workflow

[rouming]

Hey folks, just a quick reminder to review this PR when you get a chance! Thanks.

[rouming]

Anything else to address from my side or this can be merged?

[glebm]

Looks good to me, just waiting for another maintainer to also have a look and merge! /cc @StephenCWills @AJenbo

[rouming]

Thanks @glebm for reviewing that.

[StephenCWills]

If the conclusion is that the AI can't work properly without forcing us to pass all loading screen events to the GameEventHandler, then I guess we are truly at an impasse.