-
Notifications
You must be signed in to change notification settings - Fork 559
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
Comments
Realized the problem is with Emscripten, not Ammo. |
@nruhe how did you end up fixing this problem for your use case? |
@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? |
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. |
- 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
- 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
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 |
- 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 - three-trackballcontrols@0.0.3 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
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
The text was updated successfully, but these errors were encountered: