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

Use SDL2 on Windows #457

Merged
merged 18 commits into from
Jun 29, 2024
Merged

Use SDL2 on Windows #457

merged 18 commits into from
Jun 29, 2024

Conversation

Lgt2x
Copy link
Member

@Lgt2x Lgt2x commented Jun 23, 2024

This PR makes Windows use SDL2 instead of DirectX, to be on par with the other supported platforms. As a consequence, now the Windows executable:

  • Is fully 64-bit compatible
  • Does not require Win95 compatibility mode or administrator permissions
  • Uses a file in %AppData% to store configuration instead of the registry

Windows 32bit has been dropped because of compatibility issues. Note that Linux and OSX 32bit are not built either.

The internal editor gave me a some trouble: this MFC app relies a lot on DirectX tools and interacts with the game through it, and is not a solved problem as of this PR. Making it use the SDL2 game properly is going to require additional work.
One solution I tried was to keep using directX for the game on the editor. This way, we would build both DirectInput and SDL2 Input for the ddio module in the form of 2 separate static libraries, but as many modules links to it, that would mean many modules have both a SDL2 and a DirectX version, which would be cumbersome. The solution to get a successful build here is a hybrid one, building the game used in the editor with SDL2, and the rest is still using DirectX. It compiles, but has runtime problems (the editor was broken before this PR anyway)

Future work:

  • Make the editor work with SDL2, and remove all remaining DirectX source files
  • Rename linux module and classes, now built by all platforms
  • Make Windows use SDL Opengl dynamic load

Depends on #453 and #456 / #449

@tophyr tophyr mentioned this pull request Jun 24, 2024
13 tasks
@JeodC JeodC mentioned this pull request Jun 24, 2024
9 tasks
@JeodC
Copy link
Collaborator

JeodC commented Jun 24, 2024

You probably know, but in windows x64 there is no audio in the main menu. I didn't test further into levels. Mve audio played fine.

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 24, 2024

yep, I did not look into it yet

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 24, 2024

rebased on master, nothing obvious comes up to fix the sound, any help welcome, I'll try again later

@pzychotic
Copy link
Contributor

rebased on master, nothing obvious comes up to fix the sound, any help welcome, I'll try again later

I tested it on Windows and had no audio at all, except for movies.

I tracked it down to a change in how the SDL audio device is getting created for movies and the rest of the game.

The callstack for movies is:

Descent3.exe!open_audio_device(const char * devname, int iscapture, const SDL_AudioSpec * desired, SDL_AudioSpec * obtained, int allowed_changes, int min_id) Line 1314
Descent3.exe!SDL_OpenAudio_REAL(SDL_AudioSpec * desired, SDL_AudioSpec * obtained) Line 1593
Descent3.exe!SDL_OpenAudio(SDL_AudioSpec * a, SDL_AudioSpec * b) Line 112
Descent3.exe!StartupSoundSystem(LnxSoundDevice * dev) Line 516
Descent3.exe!LnxSound_CreateSoundBuffer(LnxSoundDevice * dev, SysSoundBufferDesc * lbdesc, LnxSoundBuffer * * lsndb) Line 168
Descent3.exe!LnxSound_CreateSoundBuffer(LnxSoundDevice * dev, SysSoundBufferDesc * lbdesc, LnxSoundBuffer * * lsndb) Line 101
Descent3.exe!'anonymous namespace'::MovieSoundDevice::CreateSoundBuffer(SysSoundBufferDesc * lbdesc, ISysSoundBuffer * * lsndb) Line 225
Descent3.exe!sndConfigure(unsigned int rate, unsigned int buflen, unsigned int stereo, unsigned int frequency, unsigned int bits16, unsigned int comp16) Line 362
Descent3.exe!MVE_rmStepMovie() Line 1215
Descent3.exe!mve_PlayMovie(const char * pMovieName, oeApplication * pApp) Line 355
Descent3.exe!PlayMovie(const char * moviename) Line 113

going through sndConfigure() and calling open_audio_device() with allowed_changes = 0

While the callstack for the menu and other things looks like this:

Descent3.exe!open_audio_device(const char * devname, int iscapture, const SDL_AudioSpec * desired, SDL_AudioSpec * obtained, int allowed_changes, int min_id) Line 1314
Descent3.exe!SDL_OpenAudioDevice_REAL(const char * device, int iscapture, const SDL_AudioSpec * desired, SDL_AudioSpec * obtained, int allowed_changes) Line 1611
Descent3.exe!SDL_OpenAudioDevice(const char * a, int b, const SDL_AudioSpec * c, SDL_AudioSpec * d, int e) Line 115
Descent3.exe!lnxsound::InitSoundLib(char mixer_type, oeApplication * sos, unsigned char max_sounds_played) Line 103
Descent3.exe!hlsSystem::InitSoundLib(oeApplication * sos, char mixer_type, char quality, bool f_kill_sound_list) Line 536
Descent3.exe!InitD3Systems1(bool editor) Line 1901
Descent3.exe!Descent3() Line 464

going through lnxsound::InitSoundLib() and calling open_audio_device() with allowed_changes = SDL_AUDIO_ALLOW_ANY_CHANGE

If I change the SDL_AUDIO_ALLOW_ANY_CHANGE to 0 in

sound_device = SDL_OpenAudioDevice(nullptr, 0, &spec, nullptr, SDL_AUDIO_ALLOW_ANY_CHANGE);

it works for me.

This change seems to boil down to SDL_audio.c Line 1478 where build_stream=SDL_TRUE now and SDL_NewAudioStream() gets called.

I have no clue about any consequences this might have for other platforms or in general...

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 25, 2024

@pzychotic you rock, thanks for investigating! you've saved me at least a couple hours of sleep tonight :) I'll give your fix a try later and report back

@JeodC
Copy link
Collaborator

JeodC commented Jun 25, 2024

@pzychotic you rock, thanks for investigating! you've saved me at least a couple hours of sleep tonight :) I'll give your fix a try later and report back

This should, in theory, work on all platforms, but definitely test it and verify. According to documentation for SDL_OpenAudioDevice, setting the flags to 0 effectively restricts SDL from changing the audio behavior when the hardware device is unable to offer a specific feature.

I'm not at all certain what this change would mean for embedded systems like android that don't have similar audio hardware and drivers like the three core platforms. Maybe add a fallback to where if SDL fails to init audio, go back to the SDL_AUDIO_ALLOW_ANY_CHANGE flag and try again. That sort of conditional should be easy enough to add.

@JeodC
Copy link
Collaborator

JeodC commented Jun 25, 2024

So at sdlsound.cpp do this, maybe.

  sound_device = SDL_OpenAudioDevice(nullptr, 0, &spec, nullptr, 0);
  if (sound_device == 0) {
    strcpy(m_error_text, SDL_GetError());
    // Retry
    sound_device = SDL_OpenAudioDevice(nullptr, 0, &spec, nullptr, SDL_AUDIO_ALLOW_ANY_CHANGE);
    if (sound_device == 0) {
      strcpy(m_error_text, SDL_GetError());
    }
    return false;
  }

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 25, 2024

Sound fix tested and approved on Windows! This PR is now open for review (@winterheart @Arcnor ?)

This should, in theory, work on all platforms, but definitely test it and verify. According to documentation for SDL_OpenAudioDevice, setting the flags to 0 effectively restricts SDL from changing the audio behavior when the hardware device is unable to offer a specific feature.

Let's not preemptively fix a bug we don't have yet and we can't test. First, find a case where this is proven to be problematic

@tophyr tophyr mentioned this pull request Jun 26, 2024
13 tasks
vcpkg.json Outdated Show resolved Hide resolved
@Jayman2000
Copy link
Contributor

Jayman2000 commented Jun 26, 2024

I just noticed something. This PR’s description says:

This PR makes Windows use SDL2 instead of DirectX, to be on par with the other supported platforms. As a consequence, now the Windows executable:

  • Is fully 64-bit compatible

[…]

Windows 32bit has been dropped because of compatibility issues.

At the same time, this PR’s README says:

Building - Windows

[…]

  1. Open a “x86 Native Tools Command Prompt” and run:

    git clone https://github.com/DescentDevelopers/Descent3
    cd Descent3
    cmake --preset win32 -D ENABLE_LOGGER=[ON|OFF]

I think that the “x86 Native Tools Command Prompt” and the --preset win32 parts need to be updated.

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 28, 2024

I think that the “x86 Native Tools Command Prompt” and the --preset win32 parts need to be updated.

Readme updated

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 28, 2024

huh is updated builtin-baseline too recent for Github-actions?

@tophyr tophyr mentioned this pull request Jun 28, 2024
13 tasks
@winterheart winterheart self-requested a review June 28, 2024 22:37
{
"name": "sdl2",
"version>=": "2.30.3"
}
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an override section is needed to specify an exact version, otherwise the builtin-baseline and version>= (note >=) fields together just designate a minimum version

Suggested change
]
],
"overrides": [
{
"name": "sdl2",
"version": "2.30.1"
}
]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the tip, I added the override

@Lgt2x Lgt2x force-pushed the win-sdl2 branch 2 times, most recently from d2ca9f9 to e94a13d Compare June 28, 2024 23:17
@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 28, 2024

rebased (correctly) on master

@winterheart
Copy link
Collaborator

Project compiles, but cannot start due failing to find required OpenGL symbols on dlopen. See Lgt2x#7 for proposed fix. Didn't test deeper, but after applying fix critical parts of game runs fine. Tested video playback, main screen UI, starting training mission and Retribution.

There only one non-critical regression found: all sound and briefings goes in German language. I don't know why, seems that happens because I'm using non-English locale on Windows and game incorrectly detects it as German.

@Lgt2x
Copy link
Member Author

Lgt2x commented Jun 29, 2024

Project compiles, but cannot start due failing to find required OpenGL symbols on dlopen. See Lgt2x#7 for proposed fix. Didn't test deeper, but after applying fix critical parts of game runs fine. Tested video playback, main screen UI, starting training mission and Retribution.

There only one non-critical regression found: all sound and briefings goes in German language. I don't know why, seems that happens because I'm using non-English locale on Windows and game incorrectly detects it as German.

Du sprichst nicht Deutsch? :D More seriously locale detection looks broken, I suspect this is also the case on Linux. I want to tackle this in a future PR. I created #477 to report it.

Your fix looks correct, I tested & cherry-picked it. It makes more sense to use the SDL loading functions for OpenGL just like Linux does. I don't know why it worked for me and others, maybe system version differences.

@Lgt2x Lgt2x force-pushed the win-sdl2 branch 2 times, most recently from babc895 to 7b7ffde Compare June 29, 2024 16:53
@winterheart
Copy link
Collaborator

Well, that was worked earlier for me too, I don't know why is was broken for this time...

On loading GL symbols we are using SDL_GL_GetProcAddress(), which only valid if we previously loaded OpenGL library with SDL_GL_LoadLibrary(). We cannot use mod_LoadModule() here, otherwise finding symbols will fail.
Copy link
Collaborator

@winterheart winterheart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Windows and Linux, both runs fine. Ready to merge.

@winterheart winterheart merged commit e68f270 into DescentDevelopers:main Jun 29, 2024
10 checks passed
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

Successfully merging this pull request may close these issues.

6 participants