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

Move builds from browserify to webpack #22

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

diarmidmackenzie
Copy link
Member

Needed it we are to move to three-to-ammo 1.0.0, as it uses ES6-style imports, which are unsupported by browserify.

(Could in theory patch up with babel, but better long-term to move to webpack).

Note, this also solves:
n5ro#201

(and maybe other problems reported compiling with webpack)
n5ro#151
n5ro#88

@diarmidmackenzie diarmidmackenzie marked this pull request as draft December 22, 2022 11:31
@diarmidmackenzie
Copy link
Member Author

This seems to have broken the Cannon worker driver completely - moving back to draft for now...

@diarmidmackenzie
Copy link
Member Author

diarmidmackenzie commented Dec 22, 2022

Error with worker driver is this:

image

2bd4a668-9c3a-4f13-b997-1b1713785598:20 Uncaught TypeError: Cannot read properties of undefined (reading 'call')
    at __nested_webpack_require_154__ (2bd4a668-9c3a-4f13-b997-1b1713785598:20:31)
    at webpackBootstrapFunc (2bd4a668-9c3a-4f13-b997-1b1713785598:72:11)
    at 2bd4a668-9c3a-4f13-b997-1b1713785598:74:3
__nested_webpack_require_154__ @ 2bd4a668-9c3a-4f13-b997-1b1713785598:20
webpackBootstrapFunc @ 2bd4a668-9c3a-4f13-b997-1b1713785598:72
(anonymous) @ 2bd4a668-9c3a-4f13-b997-1b1713785598:74

And physics doesn't work (many examples didn't work anyway with worker driver, but the sandbox example did work, and isn't working now).

Copy link
Member

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a quick look at the PR. If you're blocked, I'll look into it.

"mocha": "^6.2.3",
"patch-package": "^6.5.0",
"replace": "^1.2.0",
"sinon": "^2.4.1",
"sinon-chai": "^2.14.0",
"three": ">=0.125.0",
"uglify-es": "^3.3.9"
"uglify-es": "^3.3.9",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the uglify-es dependency.
webpack is using terser to minify.

@@ -17,9 +19,6 @@
"postversion": "git push && git push --tags && npm publish",
"postinstall": "patch-package"
},
"browserify-shim": {
"three": "global:THREE"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to define three as external if webpack is using three-to-ammo as ES6 module that itself import from three. See the webpack config in aframe-inspector https://github.com/aframevr/aframe-inspector/blob/e8798c73b6abd51566107e21a007f6fe90fbb5ad/webpack.config.js#L21-L24

@diarmidmackenzie
Copy link
Member Author

Problems with this PR are with cannon worker driver only. Comparing the built files, it looks like a big chunk of code is being omitted in the new builds, but I didn't understand why yet.

I'm not sure how much effort it's worth expending to maintain cannon worker driver. Lots of the function already doesn't work, and I don't see much need for a 4th physics option on top of cannon, ammo and PhysX.

I think a better way forwards is probably to deprecate cannon worker driver, but quite a bit of work to do so cleanly.

@kylebakerio
Copy link
Member

what work is needed to deprecate the cannon worker driver?

@diarmidmackenzie
Copy link
Member Author

what work is needed to deprecate the cannon worker driver?

Docs updates; remove various bits of code & tidy up; remove examples; test to ensure non-deprecated function is not regressed.

Worth noting that I am curious about whether an A-Frame physics system built on PHY might be a better solution, particularly in terms of maintainability.

That would give us 3 drivers (Oimo, Ammo.js and PhysX), with both worker drivers and direct drivers available, and an actively developed foundation to build on. My intention is to check out the feasibility of that option as a priority, before investing further in either of aframe-physics-system, or aframe-physx.

@diarmidmackenzie
Copy link
Member Author

Need to watch out for this effect, of course :-)
https://xkcd.com/927/

@kylebakerio
Copy link
Member

PHY looks very interesting; however, single maintainer, and looks like it was untouched fo 2 years? I hadn't heard of it, but from the README and demo it certainly looks promising. Would of course be good to leverage the work of another dev in the space.

Even if just for "full support of physx" (is that for real???), getting the extra oimo support as well--definitely should be looked into. Very cool find.

@kylebakerio
Copy link
Member

Need to watch out for this effect, of course :-) https://xkcd.com/927/

I knew exactly which xkcd this was going to be without clicking, lol.

@vincentfretin
Copy link
Member

@kylebakerio You may have overlooked the commits? There were commits the last two weeks. There is support for Physx 5.1. The maintainer shared the news the 3rd february 2023 in this tweet that I shared in the #physics channel on slack.

@kylebakerio
Copy link
Member

Not sure if my english was unclear, but my tone was of excited surprise and appreciation, not so much doubt--but some sense of 'need to confirm this, but that is amazing news if so'.

@diarmidmackenzie
Copy link
Member Author

I've spent a bit of time looking at PHY, and although there is some potential there, I don't see a way forwards there that I'm keen to pursue at the moment...

PHY is more of a game engine, than a physics engine, with a 3 layer architecture

  • Main.js, which is responsible for everything you see here takes care of orchestrating a wide range of different demos, engines etc.
  • Motor (aka Phy) is the main game engine. It provides the main game engine API, which the demos use to set things up, and takes care of fan-out to the appropriate physics engine + also the rendering of the objects (using Object3Ds or Instanced Meshes)
  • There are then wrappers for the various physics engines (PhysX, Ammo, Oimo etc.) that Motor can use to drive the actual physics. All the physics engines preset the same clean, standardized API to Motor, either via direct function calls, or running as workers.

The API between Motor & the Engines is fairly simple & clean. It's not documented, but you can see how it is used by searching across the codebase for:

  • root.post( for messages from the Motor to the Engine
  • engine.post( for messages from the Engine to the Motor

This API provides support for rigid bodies (including compound shapes), joints + drivers, plus more. I didn't see how collision events work (only the Rapier engine seemed to have code to generate collision events), but I may_ have overlooked something.

How could we make use of any of this in A-Frame?

It looks to me like Motor wraps together too many concerns to fit into A-Frame's ECS architecture as a physics engine. I wouldn't want an A-Frame physics engine to take responsibility for setting up Meshes, Materials etc. I want to handle that separately using standard pre-existing A-Frame components.

I think it would be possible to create a set of A-Frame components that take the place of Motor, sitting on top of the Motor-Engine API, and provide a standardized component-based way to set up any of the PHY Engines.

Potentially this could give us a physics engine for A-Frame that provides a standard set of components, and can easily be switched between any of the physics engines that PHY supports (at this time, that's Oimo, Ammo & Physx).

The PHY Motor-Engine interface is not documented or versioned. There's no guarantee it changes over time, although it's likely to be quite stable. The individual physics Engines are built to per-engine JS modules in the build folder of the repo.

I think there's the potential to build a strong, flexible physics system for A-Frame on top of the PHY Engines. But it also looks like quite a lot of work just to get to parity with where we are with PhysX, Ammo & Cannon today.

I'm also unconvinced that the A-Frame community needs multiple physics engines available. Excellent, well-documented support for one really good Physics Engine is probably sufficient.

The question remains in my mind, whether that should be based on PhysX or Ammo. My personal inclination at this time is to lean into PhysX, to try to bring it up to feature parity with Ammo/aframe-physics-engine. However that's probably not going to happen fast, so in the meantime, tidying up aframe-physics engine to remove the deprecated Cannon Worker option (as per the topic of this issue) is probably a worthwhile thing to do.

@kylebakerio
Copy link
Member

Thanks for looking into that.

Is it possible to use PHY as a reference for building out our own interface for PhysX?

@diarmidmackenzie
Copy link
Member Author

We already have PhysX here:

https://github.com/c-frame/physx

PHY may be a useful reference for filling some gaps, e.g. compound shapes.

@kylebakerio
Copy link
Member

Sure, filling in the gaps is what I mean; from the description I had the impression that their implementation of a PhysX to JS wrapper was much more complete.

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