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

SDL2 Support when building for Emscripten #218

Merged
merged 2 commits into from Oct 9, 2017

Conversation

Squareys
Copy link
Contributor

Hi @mosra !

As mentioned, I am in the midst of moving the Sdl2Application to SDL2 for Emscripten (which removes alot of preprocessor branches).

Cheers, Jonathan

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling bcbd3d2 on Squareys:emscripten-sdl2 into db56be5 on mosra:master.

@Squareys Squareys changed the title [WIP] SDL2 Support when building for Emscripten SDL2 Support when building for Emscripten Oct 4, 2017
@Squareys Squareys force-pushed the emscripten-sdl2 branch 3 times, most recently from 80204ac to b2e7091 Compare October 4, 2017 16:57
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling 1cbcc93 on Squareys:emscripten-sdl2 into 68766b1 on mosra:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling b2e7091 on Squareys:emscripten-sdl2 into 68766b1 on mosra:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling b2e7091 on Squareys:emscripten-sdl2 into 68766b1 on mosra:master.

@@ -534,6 +534,10 @@ foreach(_component ${Magnum_FIND_COMPONENTS})
find_package(SDL2)
set_property(TARGET Magnum::${_component} APPEND PROPERTY
INTERFACE_LINK_LIBRARIES SDL2::SDL2)
if(CORRADE_TARGET_EMSCRIPTEN)
# TODO: give me INTERFACE_LINK_OPTIONS or something, please
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -s USE_SDL=2")
Copy link
Owner

@mosra mosra Oct 5, 2017

Choose a reason for hiding this comment

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

I need to verify that this doesn't bloat Emscripten apps that are not using SDL2. Apart from that, I have no other comment. Thanks for the contribution!

EDIT: see below

Signed-off-by: Squareys <squareys@googlemail.com>
@@ -100,8 +100,6 @@ bool Sdl2Application::tryCreateContext(const Configuration& configuration) {
SDL_GL_SetAttribute(SDL_GL_FRAMEBUFFER_SRGB_CAPABLE, configuration.isSRGBCapable());
#endif

/** @todo Remove when Emscripten has proper SDL2 support */
#ifndef CORRADE_TARGET_EMSCRIPTEN
/* Set context version, if user-specified */
if(configuration.version() != Version::None) {
Copy link
Owner

@mosra mosra Oct 5, 2017

Choose a reason for hiding this comment

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

This line doesn't compile for me:

../src/Magnum/Platform/Sdl2Application.cpp:104:22: error: no member named 'version' in 'Magnum::Platform::Sdl2Application::Configuration'
    if(configuration.version() != Version::None) {

I force-pushed a rebased version on your branch, waiting for the CI to say what's up. Can you look into it after?

Also, while at it, there's also MouseEvent::clickCount() that's disabled on Emscripten currently (shouldn't be anymore if using SDL2) and the isTextInputActive() function has a probably-no-longer-necessary Emscripten workaround in it (you have to verify that the linker doesn't fail).

Sorry for the premature "no other comment" comment, then. These are things that can be seen only after I pull locally, not from the diff :D

EDIT: there's no point in configuring a version on WebGL, so I would propose having it #defined-out the same as previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thanks for the review! I will find some time today to finish that up 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling 92b4707 on Squareys:emscripten-sdl2 into 68766b1 on mosra:master.

Signed-off-by: Squareys <squareys@googlemail.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.549% when pulling 5fb3f43 on Squareys:emscripten-sdl2 into 68766b1 on mosra:master.

@Squareys
Copy link
Contributor Author

Squareys commented Oct 9, 2017

Thanks! 👍

@mosra
Copy link
Owner

mosra commented Oct 9, 2017

Just for the record: reverted back to the previous state in 6ae21f2, because the size increase even in WASM build was over half a megabyte (i.e., size of the triangle example went from 600 kB to 1.2 MB), which I don't find acceptable.

Will revisit this later, if needed.

@mosra mosra added the scrapped label Feb 15, 2018
@mosra mosra added this to the 2018.02 milestone Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants