c-frame / aframe-physics-system

community-maintained fork of n5ro's aframe-physics-system
https://c-frame.github.io/aframe-physics-system/
MIT License
43 stars 11 forks source link

Ammo wrapper performance improvements #15

Closed diarmidmackenzie closed 1 year ago

diarmidmackenzie commented 2 years ago

Previous perf testing had shown that aframe-physics-system Ammo driver "wrapper" performance is significantly worse than the equivalent "wrapper" performance for PhysX. https://c-frame.github.io/physx/examples/pinboard/ammo-vs-physx.html

This is the performance of the javascript code to sync to/from the physics engine, rather than the physics engine itself.

Recent testing with Cannon driver in aframe-physics-system (see PR #14) has shown that "wrapper" performance is similar to PhysX, so it's just aframe-physics-system Ammo driver that has the poor performance.

I've done some profiling to analyze why thhis is. So far 2 major issues stand out.

  1. Ammo synchronizes Static objects to physics on every tick. That is unecessary as static objects aren't supposed to move when using the Ammo driver.

  2. In the perf benchmark, balls get re-created when they drop off the bottom of the pinboard and are re-added at the top. This is particularly expensive because of the balls are set up with ammo-shape="fit:all"

To compute the ammo shape, the entire mesh geometry is analyzed point by point, which takes a lot of work. Even though this is only done for a few balls each second, it's still enough to add substantial overall cost.

Various fix options for the above:

1.

    • Update the example to explicitly encode the desired radius of the sphere, rather than using fit:all, together with explanation why.
    • Add to the documentation for ammo-shape to emphasize the performance issues with fit:all and in particular emphasize that it should not be used for entities that are frequently spawned.
    • Option: We could also extend three-to-ammo to explicitly notice where sphere and box geometries are used, and take short-cuts in fitting to these (PhysX does this), though it's unnecessary if we encourage manual fitting anyway.
    • Option: could we cache ammo shapes computed for a given geometry, so they don't need to be re-computed? My understanding is that the geometry system ensures that only a signle geometry is created when the same geometry is used multiple times. So perhaps some caching of previously calculated results is possible?

Having made prototype versions of these fixes, I ran some further profiling, and couldn't spot any obvious further gains. However "wrapper" timings for Cannon still seem to be quite a bit faster than for Ammo, even though Cannon also syncs all static entities to Physics (because it doesn't distinguigh static from kinematic).

That suggests there are potentially more gains available for Ammo "wrapper" perf, but haven't yet identified them.

On my PC, fixes 1 & 2 above seem to be enough to get Ammo wrapper perf from ~3msecs to ~1.5msecs, vs Cannon "wrapper" perf < 1msec.

kylebakerio commented 2 years ago

great finds. Perhaps moving a static object should require updating it to a dynamic object, and then back to a static object? does the code allow for that kind of switching? This is an idea as an alternative to a custom function that needs to be called when a static object is moved, which while could perhaps be easier to implement, would be a bit fuzzier dx overall imo. (Forgive my ignorance, haven't worked with ammo myself, so just thinking in principle here--if this doesn't make sense / isn't possible, ignore me.)

could we cache ammo shapes computed for a given geometry, so they don't need to be re-computed? My understanding is that the geometry system ensures that only a signle geometry is created when the same geometry is used multiple times. So perhaps some caching of previously calculated results is possible?

I'm sure that would be a big gain in certain scenarios, but caching is a dangerous tool that usually creates bugs somewhere when not used very carefully. Could put it behind a feature flag to start, though, and encourage flipping it on in a performance checklist, and solicit bug reports as necessary, to start.

kylebakerio commented 2 years ago

I am definitely interested in the improvement possible to the cannon driver if we do the same fix there; should the fix be upstream to cannon, or can we do it in the wrapper, or is it a situation where both are possible?

diarmidmackenzie commented 2 years ago

Perhaps moving a static object should require updating it to a dynamic object, and then back to a static object? does the code allow for that kind of switching?

Better would be to update to a kinematic object, and then back to a static object.

I think there are 2 different cases.

All that said, my experience with switching body types with the ammo driver is that it's a little bug-prone (that experience is with dynamic <--> kinematic, so may or may not be applicable...).

I am definitely interested in the improvement possible to the cannon driver if we do the same fix there; should the fix be upstream to cannon, or can we do it in the wrapper, or is it a situation where both are possible?

Cannon doesn't distinguish between kinematic & static objects. So you could say that all static objects in Cannon are kinematic - hence we should update them every frame.

We could introduce a "kinematic" concept at the wrapper level in Cannon, where the sole difference between static & kinematic would be that kinematic objects would have their position sync'd to the physics engine every frame, whereas static objects wouldn't.

I'd suggest a new issue to track that potential improvement. That said, I'm not sure there's that much value as the before/after phases are already much cheaper relative to the engine with Cannon than they are with Ammo.

diarmidmackenzie commented 1 year ago

This is done.