Infinite loop when applying impulse to shape

[diarmidmackenzie]

Splitting this out as a separate issue as it's not straightforward

Further issue spotted in Cannon example. When clicking to apply an impulse in the compound example. Seems to be specific to this example...

aframe-physics-system.js:18053 Uncaught RangeError: Maximum call stack size exceeded at LocalDriver.applyBodyMethod (aframe-physics-system.js:18053:3) at body.applyImpulse [as applyImpulse] (aframe-physics-system.js:18916:16) at LocalDriver.applyBodyMethod (aframe-physics-system.js:18053:27) at body.applyImpulse [as applyImpulse] (aframe-physics-system.js:18916:16) at LocalDriver.applyBodyMethod (aframe-physics-system.js:18053:27) at body.applyImpulse [as applyImpulse] (aframe-physics-system.js:18916:16) at LocalDriver.applyBodyMethod (aframe-physics-system.js:18053:27) at body.applyImpulse [as applyImpulse] (aframe-physics-system.js:18916:16) at LocalDriver.applyBodyMethod (aframe-physics-system.js:18053:27) at body.applyImpulse [as __applyImpulse] (aframe-physics-system.js:18916:16)

[diarmidmackenzie]

What I've discovered so far...

When a body has a separate shape component (as per compount shapes), after the shape is initialized, the shape calls addShape() on the body, and the body sets the shouldUpdateBody flag on itself.

Then on the next tick, the _play() function gets called, which calls this.system.addBody(this.body);, which results in this code being called for a 2nd time (it was originally called when the body component was initialized)

if (this.data.driver === 'local') {
      body.__applyImpulse = body.applyImpulse;

The problem is that this code is thus being executed twice, and when this happens, __applyImpulse ends up pointing to itself, resulting in an infinite loop.

This doesn't happen when there is no separate shape component, as we don't get this double initialization.

What I haven't yet understood is why this problem did not occur in A-Frame 1.3.0.

[diarmidmackenzie]

The difference seems to be that when the body component is initialized, in initBody() with 1.4.0, this.isPlaying is set, whereas in 1.3.0 it isn't. That results in _play() being called on initialization, and then again as a result of the shouldUpdateBody flag being set.

isPlaying flag is managed by A-Frame, so it makes sense that an A-Frame change could change when this gets set. Not yet sure what changed, though...

[diarmidmackenzie]

Done a bit of digging in A-Frame but can't easily identify what has led to this change in ordering.

Ether way, aframe-physics-system should be more robust to this sequence.

Here's an example using A-Frame 1.3.0 that shows the same problem: https://glitch.com/edit/#!/infinite-loop-apply-impulse?path=index.html%3A30%3A0

So I'm not too bothered what the exact cause of the subtle change in ordering is - we should make aframe-physics-system robust to either order...

[diarmidmackenzie]

I atempted a fix for this by calling _pause() before calling _play() in tick processing when this.shouldUpdateBody is set, here

That does fix the problem in 1.4.0.

However things stop working at 1.3.0 with that fix.

In both this example, and in the glitch example linked above, we then hit this error:

aframe-physics-system.js:18054 Uncaught TypeError: Cannot read properties of undefined (reading 'apply')
    at LocalDriver.applyBodyMethod (aframe-physics-system.js:18054:27)
    at body.applyImpulse (aframe-physics-system.js:18917:16)
    at i.forcePushCannon (force-pushable.js:51:13)
    at HTMLElement.<anonymous> (a-node.js:263:16)
    at i.twoWayEmit (cursor.js:427:19)
    at i.onCursorUp (cursor.js:281:12)
    at HTMLCanvasElement.<anonymous> (bind.js:12:17)

We had a problem with the code to set up __applyImpulse being called twice in 1.4.0. Now we have a probem with it not being called at all in 1.3.0.

A further oddity - in the glitch example, if I replace the <a-sphere> with an <a-entity> with sphere geometry, this new problem goes away (that change has no impact on the original problem tracked in this issue) But if I do the same thing with the aframe-physics-system example, it doesn't...

Why is it not set up? Well it seems that wuith 1.3.0, on the initial call to the play function, isLoaded is not set to true for the <a-sphere> (even though it is set at this point for all the other entities with physics bodies).

(as an aside, don't confuse 2 flags: isLoaded and hasLoaded - hasLoaded is an aframe flag on thea-entity,isLoadedis aframe-physics-system state on thebody` component).

But even if this flag is not set up on the first call to play, surely it will be set up on the subsequent tick processing, which should lead to __applyImpulse being set up as required. Why is that not happening? More investigation needed.

[diarmidmackenzie]

OK, problem with the fix is fairly simple.

Just as the play() processing doesn't allow for the fact that play() might have already been called, so the pause() processing doesn't allow for the fact that play() might not have been called yet.

This bit of code is problematic when that's the case, as it clears out body.applyImpulse. completely. if body.__applyImpulse has not been set up.

      body.applyImpulse = body.__applyImpulse;
      delete body.__applyImpulse;

      body.applyForce = body.__applyForce;
      delete body.__applyForce;   

[diarmidmackenzie]

Stepping back, let's look at the overall flow here...

isLoaded is set on a body:

• when it's initialized if if already has a shape.
• or after shouldUpdateBody is set (in the next tick)

When play is called on the body, play processing is only done if isLoaded is set.

So what's curious (and probably the cause of the regression) is why play processing is being invoked 1st time around in 1.4.0, prior to the shape being added...

But also, we can make A-Frame physics system resilient by replacing this code:

      this.isLoaded = true;

      this._play();

with this:

      this.pause();
      this.isLoaded = true;
      this.play();

I've tested that fix with the example at 1.3.0 and 1.4.0, and with the glitch at 1.3.0 and 1.4.0, and it seems good.

[diarmidmackenzie]

So what's curious (and probably the cause of the regression) is why play processing is being invoked 1st time around in 1.4.0, prior to the shape being added...

OK, this is happening due to these lines:

    // If component wasn't initialized when play() was called, finish up.
    if (this.isPlaying) {
      this._play();
    }

The isPlaying flag is managed by A-Frame, and it's clearly possible for this to be set when the isLoading flag is not set. But if we go ahead and call _play() when isLoading is not set, that's likely t cause problems.

Therefore an alternative fix is probably just to modify the code above to this:

    // If component wasn't initialized when play() was called, finish up.
    if (this.isPlaying) {
      this.play();
    }

(then _play() will only be invoked if isLoaded is set).

[diarmidmackenzie]

Turns out that fix doesn't work, because this.play() is not invoked directly, but via the the A-Frame wrapPlay() method, which then doesn't invoke the components play() because that wrapper has code that de-duplicates calls to play()

But this fix does work:

if (this.isPlaying) {
      if (this.isLoaded) this._play();
    }

Or rather, this fixes the original problem described in this issue. But it doesn't fix the glitch, because the glitch had engineered a different route to duplicate calls to play(), which is not addressed by this alternative fix.