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

[WIP] Test emscripten build for WebGL deployment #53

Closed
wants to merge 23 commits into from

Conversation

msavva
Copy link
Collaborator

@msavva msavva commented May 28, 2019

Motivation and Context

Preliminary testing of habitat-sim compiling through emscripten for WebGL deployment. Currently, just a "pile of hacks" to get viewer utility binary working only with pre-loaded data package containing test scenes.

Before landing will need to:

  • Figure out proper handling of dynamic asset loading (example dynamic data package loading here)
  • Remove hack ignoring texSize setting (see here)
  • Also compile with headless contexts so that things other than viewer utility work
  • Lots of other stuff ...

How Has This Been Tested

  1. Download and install emscripten SDK from https://emscripten.org/
  2. Make sure to source emscripten env variables before building (i.e. source ~/code/emsdk/emsdk_env.sh or similar)
  3. Run ./build_js.sh
  4. Follow instructions at end of build to launch server and open viewer in browser tab

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 28, 2019
Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

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

You're moving fast! :) Added a bunch of notes that might be (hopefully) helpful.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/esp/core/logging.h Show resolved Hide resolved
src/esp/gfx/GenericShader.cpp Show resolved Hide resolved
@@ -160,8 +162,11 @@ GenericShader& GenericShader::bindTexture(Magnum::GL::Texture2D& texture) {

texture.bind(TextureLayer);

// TODO this is a hack and terrible! Properly set texSize for WebGL builds
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess having bindTexture(Texture2D& texture, int size) would be a solution, right? Unfortunately until ES 3.1 there's no way to query texture image sizes.

src/utils/datatool/CMakeLists.txt Outdated Show resolved Hide resolved
# If not WebGL build, also include offscreen render-reliant code
# TODO: this should be built as well (with emscripten windowless contexts)
if(NOT BUILD_WEBGL)
list(APPEND gfx_SOURCES Simulator.cpp WindowlessContext.cpp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Magnum builtin WindowlessEglApplication works as-is on Emscripten, but I'm not sure about your modified version -- the extension APIs most definitely aren't defined there. I can take a look at this once I get to integrating your WindowlessContext modifications back into Magnum.

build_js.sh Outdated Show resolved Hide resolved
build_js.sh Outdated Show resolved Hide resolved
src/utils/viewer/viewer.html Outdated Show resolved Hide resolved
@msavva msavva mentioned this pull request May 30, 2019
11 tasks
@msavva msavva force-pushed the decouple-util-deps branch from e1e0ebc to b8a417d Compare May 30, 2019 03:22
@mathfac
Copy link
Contributor

mathfac commented Jun 1, 2019

If this PR will be rebased on latest master we will get proper CI testing.

@msavva msavva force-pushed the decouple-util-deps branch from bee06d2 to 0a41312 Compare June 8, 2019 02:26
-DBUILD_DATATOOL=OFF \
-DBUILD_PTEX_SUPPORT=OFF \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_PREFIX_PATH="$EMSCRIPTEN" \
Copy link
Contributor

Choose a reason for hiding this comment

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

The $EMSCRIPTEN variable should be explicitly set to the prefix where emscripten was built otherwise it picks up the wrong compiler.

Copy link
Collaborator Author

@msavva msavva Jul 2, 2019

Choose a reason for hiding this comment

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

Thanks @abhiskk for testing out this PR and support in my absence. Regarding, the $EMSCRIPTEN variable: it is defined by sourcing the installed emscripten distribution's emsdk_env.sh file so it is in general a good idea to leave that not explicitly set in the script here. Perhaps it would be good to include in the script a source /path/to/emsdk_env.sh statement though and ask people to modify as necessary.

@mosra
Copy link
Collaborator

mosra commented Aug 8, 2019

Since #133 got merged, this can be closed, right? Or is here something left that didn't get into #133 (JS bindings code)?

@msavva
Copy link
Collaborator Author

msavva commented Aug 8, 2019

Since #133 got merged, this can be closed, right? Or is here something left that didn't get into #133 (JS bindings code)?

Yes, it should be getting closed fairly soon. There are indeed a few JS bindings here that will make their way in as a separate PR.

@msavva
Copy link
Collaborator Author

msavva commented Aug 22, 2019

Functionality in this PR has been cleanly merged in separate PRs (mostly #133 and #166). Closing this PR.

@msavva msavva closed this Aug 22, 2019
@msavva msavva deleted the decouple-util-deps branch August 22, 2019 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants