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

Environment Detection Causing Errors In Browser #109

Closed
nruhe opened this issue Jun 14, 2015 · 5 comments
Closed

Environment Detection Causing Errors In Browser #109

nruhe opened this issue Jun 14, 2015 · 5 comments

Comments

@nruhe
Copy link

nruhe commented Jun 14, 2015

Hello.

I'm trying to utilize this library alongside ES6 modules, via SystemJS. The problem is that the Ammo library seems to be doing environment detection based on whether CommonJS support is defined. Browsers can support CommonJS style formatting, so I'm running into a problem with Ammo trying to request a file at "/path" and "/fs".

The library shouldn't assume FS and Path modules are available just because CommonJS syntax is available. I'd appreciate it if you wouldn't mind fixing this when you get the chance. If I get frustrated enough in the meantime, I'll probably make a pull request, though I think I've got a work around.

Thanks!

-Nick

@nruhe nruhe closed this as completed Jun 14, 2015
@nruhe
Copy link
Author

nruhe commented Jun 14, 2015

Realized the problem is with Emscripten, not Ammo.

@agilgur5
Copy link

agilgur5 commented May 4, 2016

@nruhe how did you end up fixing this problem for your use case?

@agilgur5
Copy link

agilgur5 commented May 18, 2016

@kripken is there a way to use MEMFS instead of NODEFS (the problem seems to be that the Emscripten build is using NODEFS due to it detecting CommonJS listed above)?

Is there an argument I can pass to Ammo to do that? Or would I have to edit the Ammo script and/or recompile a different way with Emscripten so it always uses MEMFS?

@kripken
Copy link
Owner

kripken commented May 18, 2016

Emscripten programs will not use NODEFS automatically - it must be explicitly set up. So ammo.js must be using MEMFS.

Something else must be going wrong, it would be good to reduce this to a testcase and file on the emscripten bug tracker.

agilgur5 added a commit to agilgur5/physijs-webpack that referenced this issue Sep 16, 2018
- this is what I was having trouble with when I first created this repo
  - c.f. kripken/ammo.js#109 (comment) ,
    webpack/webpack#7352 ,
    and emscripten-core/emscripten#6542
- can't just upgrade to a new ammo as that might very well break
  physijs, so just make the Node environment check false
  - in this case I changed `var qa="object"===typeof process,` to
    `var qa=false,` in the minified code
    - could also remove the `if(qa){`... parts now, though that'll
      probably get re-minified too and nbd
agilgur5 added a commit to agilgur5/physijs-webpack that referenced this issue Sep 16, 2018
- this is what I was having trouble with when I first created this repo
  - c.f. kripken/ammo.js#109 (comment) ,
    webpack/webpack#7352 ,
    and emscripten-core/emscripten#6542
- can't just upgrade to a new ammo as that might very well break
  physijs, so just make the Node environment check false
  - in this case I changed `var qa="object"===typeof process,` to
    `var qa=false,` in the minified code
  - and also remove thed `if(qa){`...`}` parts
    - this was the part that had `require` statements, so now webpack
      etc should be able to parse it without a problem
      - there was a `require("fs")` and a `require("path")` in there
  - alternatively, could build and replace stuff with webpack, but I'd
    need to provide that anyway for auto-config
    - downside is it might no longer work on Node, but not the target
      audience so w/e
@agilgur5
Copy link

agilgur5 commented Sep 16, 2018

This issue was apparently related to emscripten-core/emscripten#6542. Newer versions could use the environment logic in emscripten-core/emscripten#6565 (if they don't already)

For anyone coming here using an older version of Ammo and having trouble with Webpack, Browserify, etc, webpack/webpack#7352 has some relevant details. Also see my commits that reference this issue above for an alternative method that just has you delete the parts of the code that have requires (they won't be used in a browser environment anyway) and the check for a Node env. There might be a better alternative than both of those by messing around with some more Webpack settings or newer versions of Webpack

agilgur5 added a commit to VerifiableRobotics/LTLMoPWeb3D that referenced this issue Oct 3, 2018
- use physijs-webpack, a fork I created a few years ago and finally
  took some time to debug and get working
  - use via github since someone took the NPM name a few years ago and
    referenced my repo in it even though it never worked until today...
    - maybe will update once I get that resolved
  - now we don't have to vendor in all those scripts anymore and don't
    need to manually configure Physijs either!
    - remove the vendored physijs and three scripts
  - had to debug and modify ammo to get that version working with
    webpack / bundlers in general
    - newer versions of Emscripten can target specific envs
    - c.f. kripken/ammo.js#109 (comment) ,
      webpack/webpack#7352 ,
      and emscripten-core/emscripten#6542
  - and then made the worker config more flexible
  - add worker-loader as a devDep per physijs-webpack instructions
    - fix publicPath location as this is now actually used
      - the worker is loaded based on the publicPath

- add three-window-resize and three-trackballcontrols as deps as well
  - since three's examples/js/ folder doesn't quite work with bundlers
  - c.f. mrdoob/three.js#9562
  - maybe if I made some modifications, updated to newer Three
    revision, and used imports-loader it might work 🤷 TBD

- upgrade physijs to latest and Three from r60 to r73
  - latest physijs uses r73, so remain consistent
  - also physijs-webpack has a peerDep for that specific version
  - [email protected] also requires r73 as dep
- had to refactor a bit due to upgrade
  - WebGLRenderer() -> WebGLRenderer({alpha: true})
    - the canvas now defaults to black without this, which was extremely
      disorienting
  - shadowMapEnabled -> shadowMap.enabled
  - CubeGeometry -> BoxGeometry
  - quaternion._euler -> rotation
    - I probably could've just used this before, couldn't have I...?

.
.

- TODO: improve webpack perf (CPU + Memory) and build speed
- this change slows down initial build times quite a bit (~20s), since
  Three, Physijs, and Ammo are all parsed by Webpack now
  - will want to update webpack to get a dev-server (wepback-serve)
    running
    - webpack itself is faster in later versions as well
    - and perhaps add HardSource for caching otherwise or split out
      vendored stuff into a DLL
    - probably can't update until decaffeinate'd since I believe the
      loaders used here are no longer maintained :/
      - and then would need to figure out the literate programming
        and its sourcemaps
  - will want to output a separate vendor bundle per best practice
    - may also want to output HTML via webpack too while at it
      - the templates don't do any templating so would be nbd
      - and would allow for hashing and therefore cache-busting
        - no need to manually clean cache then
  - and prod build / uglification is even _slower_ (~80s)
    - may want to exclude some files from Uglify
      - i.e. Ammo and use three.min so it can be excluded too
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

No branches or pull requests

3 participants