Closed MignonBelongie closed 3 years ago
Thanks so much for preparing this and your much larger associated contributions in donmccurdy/three-to-cannon#27 😊 I think package-lock.json
should also be updated to reflect the changes to package.json
. This can be done with (reference)
npm install --save --force three-to-cannon@3.0.2
three.js has also become a peer dependency for three-to-cannon
since v2.1.0
which means that with the installed version (rather than linked package) we'll get test and build errors in APS. I think it should be sufficient to also just add three
as a peer dependency just like in https://github.com/donmccurdy/three-to-cannon/commit/d85ced9cc9ab677ff4bc50901aa7ad4c8805da32. And then install it locally to get package-lock.json
to update
npm install three@0.115.0
We will also need to update our distribution command to exclude three
from the bundle, again just as was done in three-to-cannon
treating three
as an external library that will be available globally for users. browserify
has a similar --external
option, and I think something like the following should be a sufficient replacement
browserify index.js -o dist/aframe-physics-system.js -x three
budo index.js:bundle.js -d examples --port 8000 -x three
However I don't think browserify
has the same --globals
option. Looking at some of the distributions for three-to-cannon
I'm not actually sure what --globals three=THREE
is doing in the microbundle
execution in the dist
command. I assumed it would just declare three=THREE
in the output bundle, but I don't see that. So I'm not sure right now what is the right thing to do here, do you have any thoughts on this?
Running the tests do work with these changes. I think technically the tests do include three
in the test bundles, which means it's not exactly the same as the distributed bundles, but I think the same is also true in three-to-cannon
where three
is not excluded. So I would be happy to leave the test commands as they are.
Really sorry, I made a mistake in the budo
dev
command, the correct one should be
budo index.js:bundle.js -d examples --port 8000 -- -x three
However running this actually fails with an error
Uncaught Error: Cannot find module 'three'
at o (_prelude.js:1)
at _prelude.js:1
at Object.6.cannon-es (three-to-cannon.js:1)
at o (_prelude.js:1)
at _prelude.js:1
at Object.10.../../../lib/CANNON-shape2mesh (body.js:2)
at o (_prelude.js:1)
at _prelude.js:1
at Object.1../src/components/ammo-constraint (index.js:5)
at o (_prelude.js:1)
when trying to view http://localhost:8000/spring.html
. So there is something wrong going on here, and it probably has something to do with the --globals
option. I'm happy to look into this further if that would be helpful? When Don's status is no longer on vacation maybe we could also ask for some advice on this, and what --globals
is doing in the microbundle
command.
I've been trying to figure this out. The problem, as I understand it, is not that aframe-physics-system.js doesn't work, but just that it contains all of three.js, and thus is huge. Is that right?
aframe.js defines THREE, and thus it is already defined when aframe-physics-system.js is loaded. Previously, that was good enough for three-to-cannon, which also used THREE. Now, instead, three-to-cannon imports from 'three', which is why I had to add 'three' as a dependency in package.json. To make things more complicated, aframe uses 'super-three', not 'three'. I don't think we can expect three-to-cannon to change to import from 'super-three' instead of 'three', and I don't even know if that would help, so, I think the question is how do we make browserify only include the parts of three that we need (instead of all 1.2 MB (600K for three.min.js)). Am I understanding the problem correctly?
I've been trying to figure this out. The problem, as I understand it, is not that aframe-physics-system.js doesn't work, but just that it contains all of three.js, and thus is huge. Is that right?
Yeh that's a much better way to describe it 😊
For the issue with super-three
vs three
, I don't think it matters because in the end we want to just say THREE
will be exposed globally, and both super-three
and three
do that. And super-three
only really makes additions to three
https://github.com/mrdoob/three.js/compare/dev...supermedium:dev, and so the THREE
global that super-three
exposes would have everything three-to-ammo
needs.
I think the question is how do we make browserify only include the parts of three that we need (instead of all 1.2 MB (600K for three.min.js)). Am I understanding the problem correctly?
Yeh I think that is the question, and really we should be able to exclude all of three
. There should be some mechanism for browserify
to effectively replace the require('three')
statement in three-to-cannon
with something that references the global THREE
. We could manually write something to do this, but I'm sure we're not the first project to have this problem, and it must already have a solution.
There should be some mechanism for
browserify
to effectively replace therequire('three')
statement inthree-to-cannon
with something that references the globalTHREE
. We could manually write something to do this, but I'm sure we're not the first project to have this problem, and it must already have a solution.
Now I understand why you were talking about --globals three=THREE
. I didn't know you could do what you're describing. I'll try to figure it out.
@Galadirith Thanks for the -g
suggestion in https://github.com/donmccurdy/three-to-cannon/issues/28#issuecomment-706487966
Does this look OK to you now?
That looks great, thanks so much for making those changes @MignonBelongie 😊 I'd be happy to suggest for you to merge this now.
Fixes #160. To see the corrected behavior, set debug: true in examples/cannon.html, and see that the wireframe for the cone now has the correct orientation.
See https://github.com/donmccurdy/three-to-cannon/pull/27 for the fix.