Inelastic Screen: Ball 2 can escape reflecting border

[arouinfar]

For https://github.com/phetsims/QA/issues/524

I've only noticed this for one present on the Inelastic screen, and haven't seen it happen elsewhere in the sim.

Steps to reproduce:

1. Inelastic screen, criss-cross preset
2. Set switch to Slip
3. Press play
4. Ball 1 is stopped at the top wall, but Ball 2 escapes (and has a non-zero velocity!)

[brandonLi8]

Hmmm. I looked at 1.0.0-dev.31 and the bug didn't exist, so it must've been introduced in between 1.0.0-dev.31 and 1.0.0-dev.32

EDIT: I said it didn't exist in 1.0.0-dev.31, but I know see it does:

The ball stops in 1.0.0-dev.31, but has a non-zero velocity.

[brandonLi8]

I also found another bug in this same scenario, where you first allow the balls to collide in "stick", then switch to "slip" before the collision with the wall occurs. There seems to be an infinite loop somewhere.

[brandonLi8]

I said:

I also found another bug in this same scenario, where you first allow the balls to collide in "stick", then switch to "slip" before the collision with the wall occurs. There seems to be an infinite loop somewhere.

In the commit above, I fixed this issue by "Resetting" the "stuck together state" of sticky collisions when the stick-slip ABSwitch is switched. As a side effect, when Balls are rotating and it is turned to "slip", the balls stop rotating:

sample:


IMO, this behavior is more natural, so I'm leaving it for now.

[brandonLi8]

Back to the original bug. These types of bugs are a huge pain to debug, but I think I've been able to slow this one down and I have a some sense of what is going on. From async await/console.log debugging, here's what the collision engine does:

1. Right before the collision occurs, it detects both collisions successfully.
2. It handles only one of the collisions (ball 1 and the wall) and steps to that position.
3. It then re-detects the collision with ball2 and the wall. The desired behavior is that this is the only collision detect at this point. However, ball2 has a velocity pointing up and is directly tangential to ball1, meaning that the physics engine now thinks there is also a collision between ball1 and ball2.
4. Since it detected a false collision between ball1 and ball2, it handles that one, which results in no velocity changes. At this point, I'm not entirely sure why, but the collision engine now forgets about the collision with ball2 and the wall, and essentially it is never handled.

This issue is similar to #129, where collisions are being detected when balls are sort of tangentially moving past each other. The balance is trying to also allow collisions that happen at the same time.

I think I have a potential fix:

• Don't allow ball-ball collisions that happen in the next 0 seconds.
• When there are multiple collisions that happen in the same x seconds, handle each one of them at the soonest step call rather than relying on subsequent step calls to handle them.

I'll be testing this solution.

[brandonLi8]

I did the above approach in the commit above, which involved adding 3 utility methods to CollisionLabUtils. Overall, the result looks good. Multiple collisions at the same time are still supported, #129 looks to still be solved, and this scenario is also fixed.

@arouinfar please review in master. This could also incur some new bugs in the collision engine (I haven't seen any yet from my own testing).

EDIT: also lmk if what i said in https://github.com/phetsims/collision-lab/issues/132#issuecomment-667423844 is ok.

[arouinfar]

@brandonLi8 I'm not seeing any escaping balls in dev.33.

also lmk if what i said in #132 (comment) is ok. As a side effect, when Balls are rotating and it is turned to "slip", the balls stop rotating

This seems a bit odd to me because the collision occurred in the past. However, switching between Stick and Slip mid-experiment is not really all that productive. I think this in an acceptable solution to addressing the bug, so we can leave it as-is. I'll make a note in the Teacher Tips.

[brandonLi8]

Reopening.

I said:

I fixed this issue by "Resetting" the "stuck together state" of sticky collisions when the stick-slip ABSwitch is switched. As a side effect, when Balls are rotating and it is turned to "slip", the balls stop rotating:

to which @arouinfar said:

This seems a bit odd to me because the collision occurred in the past. However, switching between Stick and Slip mid-experiment is not really all that productive. I think this in an acceptable solution to addressing the bug, so we can leave it as-is. I'll make a note in the Teacher Tips.

With the changes in #126, the balls still rotate even if the ABSwitch is turned to "slip". I confirmed the original bugs don't happen anymore. I'm reopening to let @arouinfar know and I'll let her close again.

[jonathanolson]

I think I reproduced this? Constant size + stick + path, 4th screen.

[brandonLi8]

Tested the above setup and confirmed that the rotating ball cluster left the play area.

This bug is frustrating because I have code that checks for when balls are already touching the border in InelasticCollisionEngine:

    // Handle degenerate case where the cluster is already colliding with the border.
    if ( this.rotatingBallCluster.balls.some( ball => this.playArea.isBallTouchingSide( ball ) ) ) {
      return this.collisions.add( new Collision( this.rotatingBallCluster, this.playArea, elapsedTime ) );
    }

My suspicion is that the this.playArea.isBallTouchingSide is failing because the epsilon that is used is too small.

[brandonLi8]

I said:

My suspicion is that the this.playArea.isBallTouchingSide is failing because the epsilon that is used is too small.

I stand corrected.

I added this to detectBallClusterToBorderCollision locally.

    console.log( 'calling detectBallClusterToBorderCollision')
    // No-op if the PlayArea's border doesn't reflect or the cluster-to-border collision has already been detected or
    // if any of the Balls are outside of the PlayArea.
    if ( !this.playArea.reflectsBorder ||
         CollisionLabUtils.any( this.collisions, collision => collision.includes( this.rotatingBallCluster ) ) ||
         this.rotatingBallCluster.balls.some( ball => !this.playArea.fullyContainsBall( ball ) ) ) {
      return; /** do nothing **/
    }
    console.log( 'here',  this.rotatingBallCluster.balls.map( ball => this.playArea.isBallTouchingSide( ball ) ))

It turns out 'here' was never logged into the console., meaning the failing code is

    // No-op if the PlayArea's border doesn't reflect or the cluster-to-border collision has already been detected or
    // if any of the Balls are outside of the PlayArea.
    if ( !this.playArea.reflectsBorder ||
         CollisionLabUtils.any( this.collisions, collision => collision.includes( this.rotatingBallCluster ) ) ||
         this.rotatingBallCluster.balls.some( ball => !this.playArea.fullyContainsBall( ball ) ) ) {
      return; /** do nothing **/
    }

[brandonLi8]

Fixed in the commit above.

@jonathanolson please test using (add the following preset to InelasticPreset for testing).

  DEBUG: new InelasticPresetValue( [
    new BallState( new Vector2( -1.19, -0.45 ), new Vector2( 0.36, 0.61 ), 0.5 ),
    new BallState( new Vector2( -0.3, -0.22 ), new Vector2( 0.09, 0.75 ), 0.5 )
  ] )

[brandonLi8]

Reverted my changes. I think I have a better idea.

EDIT: reverted my revert. I'll like my original idea more.

[arouinfar]

With the changes in #126, the balls still rotate even if the ABSwitch is turned to "slip". I confirmed the original bugs don't happen anymore. I'm reopening to let @arouinfar know and I'll let her close again.

This looks great @brandonLi8.

Since it looks like there are changes for @jonathanolson to review, I'll leave the issue open.

[jonathanolson]

The changes here look good. The preset works nicely in this case. For other related issues, I'll dig into why those might be problematic independently.