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

Add initial port of SDL3 #23630

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Add initial port of SDL3 #23630

merged 1 commit into from
Feb 11, 2025

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Feb 8, 2025

I just got a couple of test working, but everything seems good so far.

Fixes: #23608

@sbc100 sbc100 marked this pull request as draft February 8, 2025 20:59
@sbc100 sbc100 mentioned this pull request Feb 8, 2025
@sbc100 sbc100 force-pushed the sdl3_port branch 2 times, most recently from 86d9a5f to 0a999bd Compare February 10, 2025 00:46
@sbc100 sbc100 changed the title [WIP] Add port of SDL3 Add initial port of SDL3 Feb 10, 2025
@sbc100 sbc100 marked this pull request as ready for review February 10, 2025 00:48
@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 10, 2025

CC @slouken @icculus

@sbc100 sbc100 requested a review from kripken February 10, 2025 00:48
tools/ports/sdl3.py Outdated Show resolved Hide resolved
'video/offscreen/*.c',
'audio/disk/*.c',
'loadso/dlopen/*.c',
# Dummy backends, do we need to support these?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@slouken, sorry for another dumb question, but do you think its worth including the dummy backends? I guess it will increase code size, but maybe allow SDL3 to be used in more cases/places?

Copy link

Choose a reason for hiding this comment

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

I'm not sure how useful they are for emscripten, maybe if someone needs to disable audio or something like that? @icculus, thoughts?

@sbc100 sbc100 enabled auto-merge (squash) February 10, 2025 19:53
test/browser/test_sdl3_canvas_write.c Outdated Show resolved Hide resolved
# symbols in the library (because LINKABLE includes the entire library).
self.emcc_args.append('-Wno-experimental')
self.emcc(test_file('browser/test_sdl3_misc.c'), ['-sLINKABLE', '-sUSE_SDL=3'], output_filename='a.out.js')
self.emcc(test_file('browser/test_sdl3_misc.c'), ['-sLINKABLE', '--use-port=sdl3'], output_filename='a.out.js')
Copy link
Member

Choose a reason for hiding this comment

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

Why test both ways of including SDL3 here? isn't that just a linker UI feature (not related to linkable/undefined symbols)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, its just two ways to link the same library. Testing that both work. This a direct copy of the SDL2 test right above.

tools/ports/sdl3.py Outdated Show resolved Hide resolved
tools/ports/sdl3.py Outdated Show resolved Hide resolved
@kripken
Copy link
Member

kripken commented Feb 10, 2025

lgtm otherwise.

So far I haven't got any browser tests working.

Fixes: emscripten-core#23608
@sbc100 sbc100 disabled auto-merge February 11, 2025 18:14
@sbc100 sbc100 merged commit 2306e68 into emscripten-core:main Feb 11, 2025
29 checks passed
@sbc100 sbc100 deleted the sdl3_port branch February 11, 2025 18:14
@ypujante
Copy link
Contributor

@sbc100 I guess I am late, but I wonder why did you choose to do -sUSE_SDL=3 (which I thought was a syntax we were trying to deprecate), instead of the new --use-port=sdl3?

@ypujante
Copy link
Contributor

@sbc100 I was also in the process of doing the sdl3 port... so here is my version

sdl3.py.zip

The way I did is "scientific":

I ran the emcmake / cmake build.

I took the output of the run which generated the exact list of files that are compiled (file_list). And the exact config file generated as well build_config_h

Hope this helps

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 12, 2025

@sbc100 I guess I am late, but I wonder why did you choose to do -sUSE_SDL=3 (which I thought was a syntax we were trying to deprecate), instead of the new --use-port=sdl3?

Both syntax's are supported. Since the USE_SDL setting already existed this change doesn't introduce a new setting name so it seems logical to allow -sUSE_SDL=3

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 12, 2025

@sbc100 I was also in the process of doing the sdl3 port... so here is my version

sdl3.py.zip

The way I did is "scientific":

I ran the emcmake / cmake build.

I took the output of the run which generated the exact list of files that are compiled (file_list). And the exact config file generated as well build_config_h

This port also uses the SDL_build_config.h file generated by cmake (see tools/ports/sdl3/SDL_build_config.h).

The one modification I made to it was to allow a single SDL_build_config.h to work with both threaded and non-threaded builds.

@ypujante
Copy link
Contributor

@sbc100 I guess you can do whatever you want but I personally don't like the addition of the sdl3/SDL_build_config.h in the ports folder. I feel like if I had proposed such a change it would have been rejected.

So far all ports have been self-contained and I feel it is better that way.

I also demonstrated in the sdl3.py file I just posted in my previous message that you can simply include the .h inside the port file (and BTW other ports do it as well)

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 12, 2025

@sbc100 I guess you can do whatever you want but I personally don't like the addition of the sdl3/SDL_build_config.h in the ports folder. I feel like if I had proposed such a change it would have been rejected.

So far all ports have been self-contained and I feel it is better that way.

I also demonstrated in the sdl3.py file I just posted in my previous message that you can simply include the .h inside the port file (and BTW other ports do it as well)

Indeed, it could be argued that self-contained headers are better. However I've always found a it rather odd to have large amounts of C/C++ header code inside of python strings. I've always been hoping to find a way to avoid that, and this is my first attempt. I was thinking of following up with the extraction of such headers into their own files for other ports. I get that for external ports its kind of nice to represent them as a single file, so there are arguments both ways.

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 12, 2025

I created a PR to inline the header for now: #23660

I may try to un-inline all of these headers in the future, but I agree that being consistent is good for now.

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.

Built-in SDL3 port
4 participants