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

Emscripten application #300

Merged
merged 35 commits into from Jun 5, 2019

Conversation

Projects
3 participants
@Squareys
Copy link
Contributor

commented Dec 15, 2018

Hi @mosra !

This is the pullrequest for the light-weight emscripten HTML5 API based application hackathon project.

Cheers, Jonathan

TODO

@Squareys Squareys force-pushed the Squareys:emscripten-application branch 5 times, most recently from 4b89b04 to a976270 Dec 15, 2018

@mosra mosra referenced this pull request Dec 29, 2018

Open

Port Sdl2Application to Android #186

0 of 7 tasks complete

@mosra mosra referenced this pull request Mar 7, 2019

Open

Compilation time, CI time and executable size improvements #293

33 of 72 tasks complete

@Squareys Squareys force-pushed the Squareys:emscripten-application branch 2 times, most recently from 55dfa4f to 2d9d36d Apr 10, 2019

@Squareys Squareys force-pushed the Squareys:emscripten-application branch from edafc8d to 5d91571 Apr 15, 2019

@Squareys

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@mosra Is it sensible to really port all of the examples? Should I keep a macro platform switch to demonstrate that Sdl2Application is compatibly replaceable with EmscriptenApplication?

@mosra

This comment has been minimized.

Copy link
Owner

commented May 5, 2019

Should I keep a macro platform switch to demonstrate that Sdl2Application is compatibly replaceable with EmscriptenApplication?

The EmscriptenApplication should be the sensible default for Emscripten, but Sdl2Application is still the sensible default basically everywhere else. So, yes there should be a platform switch between EmscriptenApplication, Sdl2Application (and AndroidApplication) because the ports branch still needs to compile on all platforms, but there's no need for having an option to use SDL2 on Emscripten.

@Squareys

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@mosra This is ready for review! If there is any bigger tasks I completely overlooked, send those over first so I can work on them in parallel to the rest of the review 🙂

@Squareys Squareys referenced this pull request May 7, 2019

Closed

Use Emscripten application #10

@mosra
Copy link
Owner

left a comment

Finally got to review this, sorry about the delay. Should be mostly minor things, no drastic design-changing requests from me ;) Also I think it isn't missing any large thing, apart from the touch events which I have to do for all the apps anyway.

Thank you!

Show resolved Hide resolved src/Magnum/Platform/CMakeLists.txt Outdated
Show resolved Hide resolved src/Magnum/Platform/CMakeLists.txt Outdated
Show resolved Hide resolved package/ci/travis.yml Outdated
Show resolved Hide resolved src/Magnum/Platform/CMakeLists.txt Outdated
Show resolved Hide resolved src/Magnum/Platform/Test/EmscriptenApplicationTest.cpp Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.cpp Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated

@mosra mosra referenced this pull request May 21, 2019

Open

2019.07 release #340

4 of 14 tasks complete

@Squareys Squareys changed the title [WIP] Emscripten application Emscripten application May 22, 2019

@Squareys Squareys force-pushed the Squareys:emscripten-application branch from 586d281 to 7014d16 May 23, 2019

@Squareys

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@mosra This is also ready for the next review pass, then I'll squash the remaining commits down into one.

@mosra
Copy link
Owner

left a comment

I was about to merge, but the EM_JS thing scared me off -- and you can save one include in the header :)

@@ -103,7 +103,7 @@ option(WITH_SHADERS "Build Shaders library" ON)
cmake_dependent_option(WITH_TEXT "Build Text library" ON "NOT WITH_FONTCONVERTER;NOT WITH_MAGNUMFONT;NOT WITH_MAGNUMFONTCONVERTER" ON)
cmake_dependent_option(WITH_TEXTURETOOLS "Build TextureTools library" ON "NOT WITH_TEXT;NOT WITH_DISTANCEFIELDCONVERTER" ON)
cmake_dependent_option(WITH_TRADE "Build Trade library" ON "NOT WITH_MESHTOOLS;NOT WITH_PRIMITIVES;NOT WITH_IMAGECONVERTER;NOT WITH_ANYIMAGEIMPORTER;NOT WITH_ANYIMAGECONVERTER;NOT WITH_ANYSCENEIMPORTER;NOT WITH_OBJIMPORTER;NOT WITH_TGAIMAGECONVERTER;NOT WITH_TGAIMPORTER" ON)
cmake_dependent_option(WITH_GL "Build GL library" ON "NOT WITH_SHADERS;NOT WITH_GL_INFO;NOT WITH_ANDROIDAPPLICATION;NOT WITH_WINDOWLESSIOSAPPLICATION;NOT WITH_CGLCONTEXT;NOT WITH_GLXAPPLICATION;NOT WITH_GLXCONTEXT;NOT WITH_XEGLAPPLICATION;NOT WITH_WINDOWLESSWGLAPPLICATION;NOT WITH_WGLCONTEXT;NOT WITH_WINDOWLESSWINDOWSEGLAPPLICATION;NOT WITH_DISTANCEFIELDCONVERTER" ON)
cmake_dependent_option(WITH_GL "Build GL library" ON "NOT WITH_SHADERS;NOT WITH_GL_INFO;NOT WITH_ANDROIDAPPLICATION;NOT WITH_WINDOWLESSIOSAPPLICATION;NOT WITH_CGLCONTEXT;NOT WITH_GLXAPPLICATION;NOT WITH_GLXCONTEXT;NOT WITH_XEGLAPPLICATION;NOT WITH_WINDOWLESSWGLAPPLICATION;NOT WITH_WGLCONTEXT;NOT WITH_WINDOWLESSWINDOWSEGLAPPLICATION;NOT WITH_EMSCRIPTENAPPLICATION;NOT WITH_DISTANCEFIELDCONVERTER" ON)

This comment has been minimized.

Copy link
@mosra

mosra May 25, 2019

Owner

The app has #ifndef MAGNUM_TARGET_GL and should thus theoretically work when WITH_GL is disabled, no? The SDL and GLFW apps are not here either.

To be extra sure, can you verify it builds when WITH_GL is disabled?

This comment has been minimized.

Copy link
@Squareys

Squareys May 25, 2019

Author Contributor

Was a very good idea, found quite a couple of issues with the GL-less build.
I think the SDL has compile issues without GL, though 🤔 That appears to be the combination of emscripten and non-GL. That would not make sense for Sdl2Application to support, though, right?

This comment has been minimized.

Copy link
@mosra

mosra May 26, 2019

Owner

Thanks!

SDL should compile w/o GL mainly on desktop and mobile (Vulkan), I don't care much if it breaks on Emscripten tho, since it was superseded by your implementation now :) For EmscriptenApplication we should be able to turn off GL either for WebGPU or 3rd party renderers.

Show resolved Hide resolved doc/platforms-html5.dox Outdated
@@ -351,7 +351,7 @@ Module.doNotCaptureKeyboard = true;
@endcode

The above is implicitly set for windowless apps, because these don't have any
event loop.
event loop, and is not supported by @ref Platform::EmscriptenApplication.

This comment has been minimized.

Copy link
@mosra

mosra May 25, 2019

Owner

I would instead mention that there's WindowlessEglApplication that can be used, instead of saying this is not supported by EmscriptenApp.

Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.cpp Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.cpp Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
Show resolved Hide resolved src/Magnum/Platform/EmscriptenApplication.h Outdated
/* The resize event is not fired on window resize, so poll for the canvas
size here. But only if the window was requested to be resizable, to
avoid resizing the canvas when the user doesn't want that. Related
issue: https://github.com/kripken/emscripten/issues/1731 */

This comment has been minimized.

Copy link
@mosra

mosra May 25, 2019

Owner

This AFAIK holds for emscripten_set_resize_callback() as well, right? Maybe update the comment to make it clear it's not just a copypasted assumption from Sdl2App.

This comment has been minimized.

Copy link
@Squareys

Squareys May 25, 2019

Author Contributor

Good question, I think it may actually fire even.

This comment has been minimized.

Copy link
@Squareys

Squareys May 25, 2019

Author Contributor

Yes, it holds, see the note in the emscripten documentation

This comment has been minimized.

Copy link
@mosra

mosra May 26, 2019

Owner

Hmm. That led me to think we could attach the callback to the window instead of this ugly polling. Will try this locally.

@mosra mosra added this to the 2019.0b milestone May 25, 2019

@mosra mosra added this to TODO in Platforms via automation May 25, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 25, 2019

Codecov Report

Merging #300 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #300      +/-   ##
=========================================
+ Coverage   71.39%   71.4%   +0.01%     
=========================================
  Files         349     349              
  Lines       17598   17595       -3     
=========================================
  Hits        12564   12564              
+ Misses       5034    5031       -3
Impacted Files Coverage Δ
src/Magnum/GL/TimeQuery.h 25% <ø> (ø) ⬆️
src/Magnum/Animation/Easing.h 100% <100%> (ø) ⬆️
src/Magnum/GL/Context.cpp 65.58% <0%> (+0.26%) ⬆️
src/Magnum/Shaders/Phong.cpp 77.86% <0%> (+1.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b11873b...733d417. Read the comment docs.

mosra and others added some commits May 26, 2019

Platform: expose raw event structures in EmscriptenApplication.
Also store them as references and not pointers.
Platform: this is all wrong anyway and mainLoopIteration() is useless.
The exec() should return int to be API-compatible with other
implementations.
Platform: rework DPI awareness and resize event in EmscriptenApp.
By mistake I thought it's the same as in Emscripten's SDL, but there
Emscripten does some emulation to ensure windowSize() ==
framebufferSize().

For the resize event it's possible to hook into the window resize
callback instead of polling for the size every frame, That's much more
efficient.
Platform: Fix UTF8 text input in EmscriptenApplication
Signed-off-by: Squareys <squareys@googlemail.com>
Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>
dox: Link to building-doc from developer guide
Signed-off-by: Squareys <squareys@googlemail.com>
Platform: light-weight main loop for EmscriptenApplication
Signed-off-by: Squareys <squareys@googlemail.com>

@mosra mosra force-pushed the Squareys:emscripten-application branch from c6f242e to 733d417 Jun 5, 2019

@mosra

mosra approved these changes Jun 5, 2019

@mosra mosra merged commit 733d417 into mosra:master Jun 5, 2019

4 checks passed

codecov/patch 100% of diff hit (target 71.39%)
Details
codecov/project 71.4% (+0.01%) compared to b11873b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Platforms automation moved this from TODO to Done Jun 5, 2019

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 5, 2019

:shipit: 🎉

@Squareys

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@mosra Thanks for merging! And especially thanks for all the great cleanup work! 🎉

@Squareys Squareys deleted the Squareys:emscripten-application branch Jun 5, 2019

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.