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

Modularize emscripten build. #26

Merged
merged 1 commit into from
Jun 26, 2019
Merged

Conversation

prestomation
Copy link

By default, emscripten's generated javascript modules lives in the global object "Module'.

This makes it impossible to load multiple emscripten modules on the same page if they are all built using this default.

In attempt to avoid namespace collision, this change modularizes the emscripten build so the Module is instead called 'BASIS'. See the emscripten FAQ for more info: https://emscripten.org/docs/getting_started/FAQ.html

Many of the target applications for Basis are possibly using other emscripten modules, so this change will reduce friction of adding Basis(assuming any of their existing modules are not modularized)

This will require existing consumers of this package to change how they load Basis when they next upgrade their Basis build.

Testing:

The existing webgl/(texture|gltf) pages have been updated to use the new name and seem to operate as before

(Built/tested on OS X with emscripten sdk-1.38.29-64bit)

@zeux
Copy link
Contributor

zeux commented Jun 3, 2019

How was basis_transcoder.wasm rebuilt? It looks like in this build the settings from CMakeLists.txt from #7 aren't picked up, so the transcoder is back to being large.

@prestomation
Copy link
Author

Hmm, I'm not sure why this is.
I built using the instructions in the README(pretty much just emcmake ../ && make) and this change is off HEAD.

I noticed my version of emscripten is behind latest. I've upgraded to sdk-1.38.31-64bit but I'm not seeing any change to the output size.

Checking out origin/master and rebuilding also gives me the larger size so possibly something else is going on.

I'll continue investigating, thanks!

@zeux
Copy link
Contributor

zeux commented Jun 3, 2019

Ok this is my fault; my change in #7 didn't update CMakeLists correctly :( I'll make a patch for this.

@zeux
Copy link
Contributor

zeux commented Jun 3, 2019

Added #27, sorry about that!

@prestomation
Copy link
Author

No worries! I will rebase and update this PR once #27 is merged

@prestomation
Copy link
Author

This PR has been rebased with HEAD after #27's merge

@zeux
Copy link
Contributor

zeux commented Jun 12, 2019

Just want to make sure everything is fine with #27 - was the .wasm file rebuilt after rebase? GH seems to say that the .wasm file in this PR is still very large, but when I locally do this on a fresh Linux box:

$ git clone https://github.com/emscripten-core/emsdk
$ cd emsdk
$ ./emsdk install latest
$ ./emsdk activate latest
$ source ./emsdk_env.sh
$ cd ..
$ git clone https://github.com/prestomation/basis_universal
$ cd basis_universal/webgl/transcoder/build
$ emcmake cmake ..
$ make

I get this:

$ ls -la
-rw-rw-rw- 1 zeux zeux  61521 Jun 11 17:53 basis_transcoder.js
-rw-rw-rw- 1 zeux zeux 219388 Jun 11 17:53 basis_transcoder.wasm
$ cat basis_transcoder.js | head -n 2

var BASIS = (function() {

(the 200-ish KB .wasm file is what I'd expect, whereas this PR still has a ~750 KB file)

@prestomation
Copy link
Author

Sorry! I'm not used to checking in binaries. I had rebuilt, confirmed the size change,but did not checkin updated binaries. Will do so early tomorrow.

By default, emscripten's generated javascript modules lives in the global object "Module'.

This makes it impossible to load multiple emscripten modules on the same page.

In attempt to avoid namespace collision, this change modularizes the emscripten build so the Module is instead called 'BASIS'.

Many target applications for Basis are likely to already be using other emscripten modules.

Testing:

The existing webgl/(texture|gltf) pages have been updated to use the new name and seem to operate as before
@prestomation
Copy link
Author

third time is the charm: updated binary now included

@kenrussell
Copy link
Contributor

Could we please review and merge this? It's important for modularity of Basis Universal's WebAssembly build. Thanks!

@richgel999 richgel999 merged commit 087198f into BinomialLLC:master Jun 26, 2019
@richgel999
Copy link
Contributor

Got it, thanks!

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.

4 participants