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

Projects
None yet
3 participants
@Squareys
Copy link
Contributor

commented Sep 26, 2017

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

This comment has been minimized.

Copy link

commented Sep 26, 2017

Coverage Status

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

@Squareys Squareys force-pushed the Squareys:emscripten-sdl2 branch from bcbd3d2 to 467dd8d Oct 4, 2017

@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 Squareys:emscripten-sdl2 branch 3 times, most recently from 80204ac to b2e7091 Oct 4, 2017

@coveralls

This comment has been minimized.

Copy link

commented Oct 4, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

commented Oct 4, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

commented Oct 5, 2017

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")

This comment has been minimized.

Copy link
@mosra

mosra Oct 5, 2017

Owner

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

modules: Add -s USE_SDL=2 to linker flags in FindMagnum.cmake
Signed-off-by: Squareys <squareys@googlemail.com>

@mosra mosra force-pushed the Squareys:emscripten-sdl2 branch from b2e7091 to 92b4707 Oct 5, 2017

@@ -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) {

This comment has been minimized.

Copy link
@mosra

mosra Oct 5, 2017

Owner

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.

This comment has been minimized.

Copy link
@Squareys

Squareys Oct 6, 2017

Author Contributor

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

@coveralls

This comment has been minimized.

Copy link

commented Oct 5, 2017

Coverage Status

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

Platform: Move emscripten port of Sdl2Application to SDL2
Signed-off-by: Squareys <squareys@googlemail.com>

@Squareys Squareys force-pushed the Squareys:emscripten-sdl2 branch from 92b4707 to 5fb3f43 Oct 6, 2017

@coveralls

This comment has been minimized.

Copy link

commented Oct 6, 2017

Coverage Status

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

@mosra mosra merged commit 5fb3f43 into mosra:master Oct 9, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.549%
Details
@Squareys

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

Thanks! 👍

@mosra

This comment has been minimized.

Copy link
Owner

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
You can’t perform that action at this time.