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

Add support for Ammo.js physics. #208

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

diarmidmackenzie
Copy link
Member

I have run npm run test:machinima, and this patch passes all machinima tests: no

"npm install" fails with a bunch of errors, so I've not been able to build locally or run these tests.

npm ERR! Error while executing:
npm ERR! C:\Program Files\Git\cmd\git.EXE ls-remote -h -t ssh://git@github.com/donmccurdy/cannon.js.git
npm ERR!
npm ERR! Host key verification failed.
npm ERR! fatal: Could not read from remote repository.
npm ERR!
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR!
npm ERR! exited with error code: 128

@wmurphyrd
Copy link
Member

Took some digging, but I tracked this down to an issue with n5ro/aframe-physics-system#202. Got it running again by pointing to github instead of npm for it.

Would you mind pulling the latest aframe-super-hands-component into you branch here? Some minor merge conflicts from an updated version of the linter.

I'd also really like to add some unit tests around this so we can make ammo support a reliable ongoing feature. Let me know if you can add those tests or else I could probably fill in

@diarmidmackenzie
Copy link
Member Author

Thanks for looking at this - a little busy at the moment, but hope to look at this again within the next couple of weeks.

@diarmidmackenzie
Copy link
Member Author

diarmidmackenzie commented May 3, 2022

Hi, sorry for the delay, I finally got a chance to look at the test side of things...

  • I merged with the latest code.
  • got unit tests running. This exposed a bug in my code, which I fixed.

I can't get the machinima tests to run - I just get 100s of errors, starting like this...

C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component>npm run test:machinima

> super-hands@3.0.3 test:machinima C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component
> karma start ./machinima_tests/karma.conf.js

03 05 2022 10:06:12.139:INFO [framework.browserify]: registering rebuild (autoWatch=true)
03 05 2022 10:06:16.806:ERROR [framework.browserify]: bundle error
03 05 2022 10:06:16.807:ERROR [framework.browserify]: Error: Can't walk dependency graph: Cannot find module '../../constants' from 'C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component\node_modules\aframe\dist\aframe-master.js'
    required by C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component\node_modules\aframe\dist\aframe-master.js
03 05 2022 10:06:16.809:WARN [karma]: No captured browser, open http://localhost:9876/
03 05 2022 10:06:16.841:INFO [karma-server]: Karma v6.3.16 server started at http://localhost:9876/
03 05 2022 10:06:16.842:INFO [launcher]: Launching browsers Firefox_NoVR, Chrome with concurrency unlimited
03 05 2022 10:06:16.849:ERROR [framework.browserify]: bundle error
03 05 2022 10:06:16.849:ERROR [framework.browserify]: Error: Can't walk dependency graph: Cannot find module '../../utils' from 'C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component\node_modules\aframe\dist\aframe-master.js'
    required by C:\Users\ASUS\Documents\GitHub\AframeCommunity\aframe-super-hands-component\node_modules\aframe\dist\aframe-master.js
03 05 2022 10:06:16.850:INFO [framework.browserify]: bundle updated

I tried adding lines like this to karma.conf to avoid some of the bundling rrors, but still couldn't get it to run cleanly.

    // avoid errors when attempting to process pre-bundled file
    noParse: [
      path.resolve('./node_modules/aframe-physics-system/dist/aframe-physics-system.js'),
      path.resolve('./node_modules/aframe/dist/aframe-master.js'),
      path.resolve('./node_modules/aframe-extras/dist/aframe-extras.misc.js')
    ],

Not sure if I'm doing something wrong. Do the machinima tests work for you at the moment?

In terms of physics tests in the existing tests, it looks like there is very little in the non-machinima tests that uses physics at all.

Only one test (stretchable-physics) calls the entityFactory helper with usePhysics set to true.

  • I could duplicate this for Ammo.js, but it wouldn't give much coverage. Is that worthwhile?
  • I could also add more unit tests that use physics, but maybe it's better to test physics in the machinima tests?

In the machinima tests I see

  • one test 'Physics grab' that uses physics.html
  • one test 'Physics worker driver' that uses physics-worker.html

I can see how I could add some tests here to cover Ammo.js, but I'm blocked on the issues above running machinima.

Any pointers on the above would be much appreciated.

@wmurphyrd
Copy link
Member

Hey @diarmidmackenzie - sorry about machinima tests, those are dead until I (or someone) revives aframe-motion-capture-components. Only unit tests are relevant for now (I've since removed the PR template note about them)

Re testing,

I'd like to do another version of this suite for "grabbable with physics" that uses ammo instead
https://github.com/c-frame/aframe-super-hands-component/blob/master/tests/reaction-components/grabbable.test.js#L109
Don't need to repeat all the tests as some behavior won't change, just:

  • constraint registered on grab (making sure it respects the specified constraint name)
  • constraint removed on release

Also the same for stretchable.
https://github.com/c-frame/aframe-super-hands-component/blob/master/tests/reaction-components/stretchable.test.js#L96
Just the one test should cover it:

  • box bodies update with scaling

I might pick up these testing tasks when I have a spare minute, so let me know if you start on them

For examples, these have the now-broken mozillareality CDN link for ammo - do you have the new one?

@vincentfretin
Copy link
Member

See c-frame/aframe-physics-system#30 for the new ammo url.

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.

3 participants