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

Initial SDL2 port #241

Merged
merged 12 commits into from May 6, 2024
Merged

Initial SDL2 port #241

merged 12 commits into from May 6, 2024

Conversation

icculus
Copy link
Contributor

@icculus icculus commented Apr 30, 2024

This just gets the basic SDL2 work in place. The resolution changes, etc, are coming in a separate PR.

These are split across several commits because that's how it was in my fork, but I could squash this down to a single unit of work and force-push this PR if that's preferable.

Note that SDL2 has no CD-ROM subsystem, so this removes physical disc support from ddio_lnx/lnxcdrom.cpp. If that's a serious problem in 2024, we can talk about what to do about it; likely we can get away with something quick and dirty if this is even necessary anymore.

This has gone as far as "it compiles and works on my machine," so please test it! I'm marking this as a draft PR until GitHub Actions gives the thumbs up.

@Arcnor Arcnor mentioned this pull request Apr 30, 2024
13 tasks
@GravisZro
Copy link
Contributor

I would prefer this not be merged as my particular target platform only has SDL 1.2 and not SDL2. A preferred solution would be to make it optional.

@icculus
Copy link
Contributor Author

icculus commented Apr 30, 2024

particular target platform

What platform is this? Dreamcast?

@GravisZro
Copy link
Contributor

GravisZro commented Apr 30, 2024

What platform is this? Dreamcast?

Yes... but there are also other old platforms in the same boat. Also, I'm aware DC has OpenGL support I could use but that means dealing with everything else separately. I would like to see this patch be modified to be an option to use SDL2 rather than displacing SDL1.2 entirely.

@winterheart
Copy link
Collaborator

Our major platforms are Linux, macOS and Windows (actual platforms for most of users). We cannot support both SDL 1.2 and SDL 2 at same time. SDL 1.2 nearly reached EOL support and lacks of features like hardware acceleration support and advanced rendering support, that we count on in future development.

@JeodC
Copy link
Member

JeodC commented Apr 30, 2024

SDL2 is currently in the pipeline as we work on getting windows up to parity with Ryan's 2020 Linux and mac ports. If you want to keep SDL1.2, you should create a fork of this project and branch it.

@icculus icculus force-pushed the ryan-sdl2-port branch 3 times, most recently from ea32991 to 815628b Compare April 30, 2024 16:57
@jcoby
Copy link
Contributor

jcoby commented Apr 30, 2024

This causes keyboard controls to act weird. pitch and yaw start out fast but quickly ramp down to a crawl. roll is limited to about 45°. might be related to removal of SDL_EnableKeyRepeat?

renderer/HardwareOpenGL.cpp Outdated Show resolved Hide resolved
Currently Windows gets this from vcpkg. Mac will get it from Homebrew,
and the GitHub linux builders will just install it with apt-get.

This might not be the perfect solution (having to install Homebrew is a pain,
GitHub Actions has an ancient SDL2), but it gets all the common platforms
running for now without much fuss.
@icculus
Copy link
Contributor Author

icculus commented May 2, 2024

Rebased against the latest in main; it's one thing to merge PRs instead of rebase them, but merging main to an unmerged PR seems like a bridge too far, y'all. :)

@icculus
Copy link
Contributor Author

icculus commented May 2, 2024

This causes keyboard controls to act weird.

Whoops, there's a later commit that fixed this I haven't moved over yet. I'll verify today and get that pushed to this PR.

Copy link
Contributor

@Jayman2000 Jayman2000 left a comment

Choose a reason for hiding this comment

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

I tested out this pull request on NixOS 23.11. It worked very well. Here are my notes:

renderer/HardwareOpenGL.cpp Outdated Show resolved Hide resolved
Descent3/lnxmain.cpp Show resolved Hide resolved
This doesn't fix the audio gaps, just the static introduced in the SDL2 port.

SDL2 does not initialize the audio callback's buffer, unlike SDL 1.2, under
the assumption the callback is going to fully write it anyhow. But since
the movie player wants to mix against the current contents of the buffer,
we need to explicitly initialize it to silence first.
Fixes keyboard input during gameplay.
@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

This causes keyboard controls to act weird. pitch and yaw start out fast but quickly ramp down to a crawl. roll is limited to about 45°. might be related to removal of SDL_EnableKeyRepeat?

This is fixed in 851f475 (and yes, it was a key-repeat thing, good call!).

@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

When I run the version of the game that’s in this PR, cutscene audio still doesn’t work, but it’s much worse.

This is fixed in 65b1a7d. (This was because I had abandoned the MVE code in my fork, so this never got fixed before.)

@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

Dependencies in README.md need to be updated. They still say to install SDL 1.2.

Resolved in bd3a596, thanks!

@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

I can’t bind controls to the scroll wheel.

Fixed in 8033513 ...this also adds support for mouse buttons beyond left/middle/right, but this is untested, as I don't have a mouse that supports it.

@JeodC
Copy link
Member

JeodC commented May 5, 2024

I can’t bind controls to the scroll wheel.

Fixed in 8033513 ...this also adds support for mouse buttons beyond left/middle/right, but this is untested, as I don't have a mouse that supports it.

I do, I'll give this a try today and let you know....assuming I can try on windows.

@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

I do, I'll give this a try today and let you know....assuming I can try on windows.

Eventually you can, but right now the SDL2 work only builds on Linux and macOS; Windows still talks directly to the Win32 API and DirectX instead.

@icculus icculus marked this pull request as ready for review May 5, 2024 15:51
@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

Okay, I think I've resolved all the concerns for this PR; there will be more SDL2 work in the future, including moving it to Windows, and other patches on top of this, like all the video resolution improvements, but this seems like a reasonable baseline, if people are happy with it.

@JeodC JeodC requested review from jcoby, winterheart and Lgt2x May 5, 2024 15:53
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.

Only thing that I've noticed - app runs in windowed mode, but that ok.
Tested on Linux 64bit.

@icculus
Copy link
Contributor Author

icculus commented May 5, 2024

Oh yeah, I changed it back to default to fullscreen right before shipping. It's a one-line change, I'll switch it shortly.

@icculus
Copy link
Contributor Author

icculus commented May 6, 2024

Fullscreen mode defaults to on as of 6d837e2.

Copy link
Collaborator

@Lgt2x Lgt2x left a comment

Choose a reason for hiding this comment

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

LGTM, tested on Linux x64. We can merge as it is and solve further problems from here

@@ -115,6 +115,7 @@ CLnxVideoModes::~CLnxVideoModes() {
// enumerate all the possible video modes
bool CLnxVideoModes::Init(void) // Display *dpy,int screen)
{
#if 0 // with SDL2, we'll just use FULLSCREEN_DESKTOP and never change the physical vidmode
Copy link
Collaborator

Choose a reason for hiding this comment

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

to be deleted sometime

@Lgt2x Lgt2x merged commit 0e69bf4 into DescentDevelopers:main May 6, 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.

None yet

8 participants