Skip to content

Conversation

@NeroBurner
Copy link
Contributor

@NeroBurner NeroBurner commented Feb 24, 2022

Description

This PR uses new APIs added in emscripten-core/emscripten#9336 to improve compatibility with USE_PTHREADS=1.

Original PR: emscripten-ports/SDL2#127
By: @jakogut
Reviewed by: @Daft-Freak

This fix enables me to compile an executable using -s USE_PTHREADS=1 -s USE_SDL=2 and not get the following error in the chromuim debug console

Uncought TypeError: Cannot read properties of undefined (reading 'CreateImageData')

Uncought TypeError CreateImageData

With the current patch applied the InfiniSim Simulator works 🎉

InfiniSim wasm working

Existing Issue(s)

This PR uses new APIs added in [emscripten-core/emscripten#9336](emscripten-core/emscripten#9336)
to improve compatibility with USE_PTHREADS=1.

Original PR: emscripten-ports/SDL2#127
By: @jakogut
Reviewed by: Daft-Freak
@Daft-Freak
Copy link
Collaborator

I think some of the comments from the original PR still apply, though I still haven't really looked into emscripten's threading stuff...

Copy link
Collaborator

@Daft-Freak Daft-Freak left a comment

Choose a reason for hiding this comment

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

Minor cast warning and formatting comments. Also SDL_GetDisplayUsableBounds has some more EM_ASMs that were added after the original PR.

@NeroBurner
Copy link
Contributor Author

I hope I addressed the required changes. Can't follow about SDL_GetDisplayUsableBounds as I don't think I've touched that in this PR. Is there something I should update anyways? Just trying to "champion" these changes, as they solve an obstacle for me.

@slouken slouken requested a review from icculus February 26, 2022 20:42
@slouken slouken added this to the 2.0.22 milestone Feb 26, 2022
@uyjulian
Copy link
Contributor

uyjulian commented Mar 6, 2022

Yeah you should update SDL_GetDisplayUsableBounds to also proxy to main thread

@icculus
Copy link
Collaborator

icculus commented Mar 13, 2022

@Daft-Freak Do we want to merge this and deal with SDL_GetDisplayUsableBounds separately, or should one of us add it to this PR?

@Daft-Freak
Copy link
Collaborator

It's probably just EM_ASM_INT_V -> MAIN_THREAD_EM_ASM_INT there too, I haven't got around to trying it though. Okay with merging as-is or with the extra change if someone tests it.

@icculus icculus merged commit fe79eb2 into libsdl-org:main Mar 31, 2022
@icculus
Copy link
Collaborator

icculus commented Mar 31, 2022

I added the SDL_GetDisplayDisplayBounds stuff in ea7d530, so this is good to go now.

@NeroBurner
Copy link
Contributor Author

thanks a lot @icculus 🙇‍♂️

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.

5 participants