c-frame / physx

A-Frame physics using PhysX
MIT License
24 stars 4 forks source link

Deleting & creating physx-bodies directly from collision event callbacks appears to be unsafe. #12

Open diarmidmackenzie opened 1 year ago

diarmidmackenzie commented 1 year ago

Seen when moving Above-Paradowski mini-golf to A-Frame 1.4.1.

This application used a static "out of bounds" object to detect balls going out of bounds. On the contactbegin event, it deleted the old ball & created a new ball.

This led to some very problematic physics on the new ball, which was subjected to random additional forces. Repeating the cycle (i.e. when this 2nd ball went out of bounds) made the problem even worse with each iteration.

By deferring the delete / re-create logic until after we were outside the event handler (using setTimeout(.... , 0)) the problem went away.

We don't fuly understand what happened, but the working hypothesis is that:

If there need to be some restrictions on what updates can be made to the physics system during collision event callbacks, that's probably OK as a restriction of the physics engine, as long as we can clearly document what those restrictions are.

Goal for this issue is to come up with a simple repro scenario for the problem, establish what these unsafe operations are, and document the necessary restrictions.

Alternatively, there may be some explcitly documented restrictions on the PhysX API, in which case we just need to explain what those imply for use the A-Frame / JS APIs that we expose.

diarmidmackenzie commented 1 year ago

This is clearly covered in the docs here

SDK state changes are not permitted from an event callback

In full...

The simplest type of simulation callbacks are the events. Using callbacks the application can simply listen for events and react as required, provided the callbacks obey the rule that SDK state changes are forbidden. This restriction may be a bit surprising given that the SDK permits writes to an inactive back-buffer while the simulation is running. Event callbacks, however, are not called from within the simulation thread, but rather from inside fetchResults(). The key point here is that fetchResults() processes the buffered writes, meaning that writing to the SDK from an event callback can be a particularly fragile affair. To avoid this fragility it is necessary to impose the rule that SDK state changes are not permitted from an event callback.

diarmidmackenzie commented 1 year ago

Merged documentation PR to cover this: https://github.com/c-frame/physx/pull/16

I guess it would be great if the physx components took care of this restriction by delaying calls to the SDK until it's safe, rather than users having to add a delay themselves...

Can consider that as a future QoL improvement.