Point SDL2 port at upstream #15452

Daft-Freak · 2021-11-08T11:54:32Z

Everything should be there now. This is also an update from 2.0.10 to 2.0.17 git from this morning.

Required some minor test updates, looks like togglefullscreen has been broken for a while though.

sbc100 · 2021-11-08T16:23:51Z

Very exciting! I guess this means we can/should archive the https://github.com/emscripten-ports/SDL2 repository?

The codesize failures should go away with a rebase.

test_webgl_context_attributes looks like maybe a real failure?

sbc100 · 2021-11-08T16:19:48Z

tests/sdl2_glalphatest.c

@@ -110,7 +110,5 @@ int main(int argc, char* argv[]) {

  glDeleteTextures(1, &texture);

-  SDL_Quit();


Why remove these calls/

Otherwise all windows get destroyed and the tests fail because the canvas is gone (well, 0x0).

So have these tests been failing forever?

Can we perhaps land all these test fixes separately (first), since they seem independent of the SDL2 update?

I think these were passing with the old version, so something may have changed there. I can split the test fixes out though.

(The testfullscreen one has likely been failing for a while.)

I did slightly worry that we have a behavioral change here? My understanding is that switching to upstream SDL would essentially be a no-op but this seem to contradict that.

Can you explain a little more what you mean by "tests fail because the canvas is gone (well, 0x0)."? Is this test run as part of CI? i.e. would reverting this part of the change cause CI to fail?

I see, thanks for clarifying. Makes sense. Could you replace these with a comment maybe // Exiting without calling SDL_Quit so the test framework can the canvas data (or something like that.. I'm not sure I have the working corret.

Ah, some of the other tests already have something like that:

emscripten/tests/sdl2glshader.c

Line 169 in df034c9

// Don't quit - we need to reftest the canvas! SDL_Quit();

Must have been missed in these ones due to the missing SDL_Init and the bug in quit... will update

Maybe also a good idea to add a changlog entry for this? Both in general for the change to use latest upstream, and specifically for this noticeable issue.

Ooh, yeah +4 SDL releases is probably worth a changelog entry! I'll try to come up with something...

(note that the issue here is only noticeable if you don't call SDL_Init but do call SDL_Quit... otherwise it happened a few updates ago)

I see, thanks! Yeah, I guess the breaking change here is not that important to mention then. But the +4 versions is 😄 Any positive highlights that people might notice could also be worth mentioning briefly in the log.

sbc100 · 2021-11-08T16:20:35Z

tests/sdl_togglefullscreen.c

@@ -136,7 +136,7 @@ int main() {
  /* SDL 1 limits which events can be used here.
   * Click and mouseup don't work.
   */
-  if (emscripten_set_mouseup_callback(NULL, NULL, 1, mouseup) != EMSCRIPTEN_RESULT_SUCCESS) {
+  if (emscripten_set_mouseup_callback("#canvas", NULL, 1, mouseup) != EMSCRIPTEN_RESULT_SUCCESS) {
    printf("Couldn't to set mouse callback. Test failed.\n");


Add return 1; here?

sbc100 · 2021-11-08T16:21:44Z

tools/ports/sdl2.py

-TAG = 'version_24'
-HASH = '5a8181acdcce29cdda7e7a4cc876602740f5b9deebd366ecec71ae15c4bbf1f352da4dd0e3c5e0ba8160709dda0270566d64a6cd3892da894463ecf8502836aa'
-SUBDIR = 'SDL2-' + TAG
+TAG = '5b646cd19e36e02c658fcbf792721b5bdce8491f'


Is there some tag we could use here? I wonder if upstream could mind tagging a release so that we could point to something here?

Hmm, there will be a tag when 2.01.8 releases. Until then there isn't a released version with all the downstream changes. (Mostly missing timers without threads)

EDIT: might need to change this anyway, the config.h needs a fix.

OK, sgtm to use the raw hash for now then. Perhaps add a TODO to update to 2.01.8 once it is released?

Daft-Freak · 2021-11-08T17:20:38Z

Ah, I didn't see that that test had a SDL2 variant. I'll go see what's going on there...

Daft-Freak · 2021-11-08T19:17:11Z

Seemed to be some conflict between the GL context the test was creating and the one SDL was creating internally for its renderer (probably the fact that they end up being the same context). Since that test was only using GL and none of the SDL render APIs, I just removed the call to SDL_CreateRenderer.

Daft-Freak · 2021-12-01T12:19:24Z

2.0.18 has been released now, so updated to use the tag. Also rebased.

This destroys the window on newer SDL, causing the tests to fail as the rendered result is gone.

Seems some state from the renderer is causing the context created later to not draw anything. This test doesn't use the renderer API anyway, so remove it.

a20eac1d · 2022-01-03T06:53:53Z

@Daft-Freak I am very excited about this change. Do you have any update on when this can be merged and deployed?

sbc100

lgtm. Looks like the there is conflict that needs resolving but otherwise I think we can land.

a20eac1d · 2022-01-07T14:15:20Z

Thank you everybody for working on this!

@kripken Can we pull this change yet somehow, or do we have to wait for the next official release build to update our EMSDK?

sbc100 · 2022-01-07T14:42:05Z

You can try it now by installing tot (i.e. ./emsdk install tot).

See emscripten-core/emscripten#15452

Daft-Freak mentioned this pull request Nov 8, 2021

Emscripten port repo libsdl-org/SDL#4623

Closed

sbc100 reviewed Nov 8, 2021

View reviewed changes

Daft-Freak force-pushed the upstream-sdl branch from b0d2127 to e6928b1 Compare November 8, 2021 16:57

Daft-Freak force-pushed the upstream-sdl branch from 5d6ddcc to cb97752 Compare November 8, 2021 21:05

Daft-Freak force-pushed the upstream-sdl branch from 238471b to 5fd9b83 Compare December 1, 2021 11:07

kripken mentioned this pull request Dec 1, 2021

Create a version_25 tag + release so emscripten-core can be updated to use it emscripten-ports/SDL2#156

Closed

Daft-Freak added 5 commits December 2, 2021 11:44

Avoid calling SDL_Quit in SDL2 tests

f0f8377

This destroys the window on newer SDL, causing the tests to fail as the rendered result is gone.

Fix sdl_togglefullscreen for new event target behaviour

ebb5d39

Remove creating a renderer from test_webgl_context_attributes_sdl2

adbe679

Seems some state from the renderer is causing the context created later to not draw anything. This test doesn't use the renderer API anyway, so remove it.

Update SDL2 port to point at upstream

86efa76

changelog

7ea9bff

Daft-Freak force-pushed the upstream-sdl branch from d969171 to 7ea9bff Compare December 2, 2021 11:45

sbc100 reviewed Jan 3, 2022

View reviewed changes

Merge branch 'main' into upstream-sdl

587b066

kripken enabled auto-merge (squash) January 4, 2022 19:53

kripken approved these changes Jan 4, 2022

View reviewed changes

kripken merged commit e325125 into emscripten-core:main Jan 4, 2022

Daft-Freak deleted the upstream-sdl branch January 5, 2022 15:18

Daft-Freak mentioned this pull request Jan 12, 2022

Emscripten : error: undefined symbol: SDL_strtokr emscripten-ports/SDL2#159

Closed

ericoporto mentioned this pull request Jan 13, 2022

Fix missing SDL_strtokr.c #15985

Merged

carstene1ns added a commit to carstene1ns/easyrpg-player that referenced this pull request May 24, 2022

Fix EasyRPG#1899

33b8ff8

See emscripten-core/emscripten#15452

TheKK mentioned this pull request Jan 30, 2023

Request update to newer version emscripten-ports/SDL2_ttf#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point SDL2 port at upstream #15452

Point SDL2 port at upstream #15452

Daft-Freak commented Nov 8, 2021

sbc100 commented Nov 8, 2021

sbc100 Nov 8, 2021

Daft-Freak Nov 8, 2021

sbc100 Nov 8, 2021

Daft-Freak Nov 8, 2021

sbc100 Dec 1, 2021

sbc100 Dec 1, 2021

Daft-Freak Dec 1, 2021

kripken Dec 1, 2021

Daft-Freak Dec 1, 2021

kripken Dec 1, 2021

sbc100 Nov 8, 2021

Daft-Freak Nov 8, 2021

sbc100 Nov 8, 2021

Daft-Freak Nov 8, 2021 •

edited

sbc100 Nov 8, 2021 •

edited

Daft-Freak commented Nov 8, 2021

Daft-Freak commented Nov 8, 2021

Daft-Freak commented Dec 1, 2021

a20eac1d commented Jan 3, 2022

sbc100 left a comment

a20eac1d commented Jan 7, 2022

sbc100 commented Jan 7, 2022

		@@ -110,7 +110,5 @@ int main(int argc, char* argv[]) {

		glDeleteTextures(1, &texture);

		SDL_Quit();

Point SDL2 port at upstream #15452

Point SDL2 port at upstream #15452

Conversation

Daft-Freak commented Nov 8, 2021

sbc100 commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daft-Freak Nov 8, 2021 • edited

Choose a reason for hiding this comment

sbc100 Nov 8, 2021 • edited

Choose a reason for hiding this comment

Daft-Freak commented Nov 8, 2021

Daft-Freak commented Nov 8, 2021

Daft-Freak commented Dec 1, 2021

a20eac1d commented Jan 3, 2022

sbc100 left a comment

Choose a reason for hiding this comment

a20eac1d commented Jan 7, 2022

sbc100 commented Jan 7, 2022

Daft-Freak Nov 8, 2021 •

edited

sbc100 Nov 8, 2021 •

edited