phetsims / collision-lab

"Collision Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Altering certain aspects of the sim with balls offscreen causes buggy behavior related to state #199

Closed loganbraywork closed 3 years ago

loganbraywork commented 3 years ago

Test device

Windows 10 Laptop

Operating System

Windows 10 1909

Browser

Firefox 84.0.2 (64-bit)

Problem description

For https://github.com/phetsims/QA/issues/599 Related to https://github.com/phetsims/collision-lab/issues/183

Turning the borders off causes a variety of buggy behavior when certain aspects of the sim are altered.

  1. When Velocity or Size is changed, which includes the use of the constant size checkbox, the balls are returned to the edge of the screen which @arouinfar confirmed was an unintended behavior.
  2. Noticed by @KatieWoe. When Elasticity is changed while the balls are off screen, stepping back only goes to the point where elasticity was changed, which is an off screen location, while the blue reset button returns the balls in a manner similar to the "Return Balls" button, which is inconsistent with its usual behavior of returning balls to the last altered state.

Steps to reproduce

To reproduce the velocity and size returning balls issues

  1. Uncheck the reflecting border box
  2. Unpause the simulation
  3. Allow balls to leave screen
  4. Check the constant size box, or alter the mass or velocity by clicking on the values at the bottom of the screen

To reproduce the elasticity and step back issue

  1. Uncheck the reflecting border box
  2. Unpause the simulation
  3. Allow balls to leave screen
  4. Alter elasticity
  5. Pause the sim and hold the step back button until grey
  6. Compare with the results of the blue reset button

Visuals A gif of the issues caused with changing velocity and mass while off screen 2021-01-21CollLabVelocityandSize A gif demonstrating the failure of step back to synchronize with the locations of the blue reset all button 2021-01-21CollLabStepbackinconsistent

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Collision Lab‬ URL: https://phet-dev.colorado.edu/html/collision-lab/1.1.0-rc.1/phet/collision-lab_en_phet.html Version: 1.1.0-rc.1 2021-01-20 01:05:28 UTC Features missing: touch User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:84.0) Gecko/20100101 Firefox/84.0 Language: en-US Window: 1920x966 Pixel Ratio: 1/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (Mozilla) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

KatieWoe commented 3 years ago

Addition: Adding balls seems to show similar buggy behavior. addingaball

brooklynlash commented 3 years ago

A few more things I noticed about this change of state with the reflecting wall: When a ball is halfway through the wall and the reflected wall is turned on, the ball still continues through. collisionwalls

When the balls are on the "stick" setting, they become one entity so if one ball goes through then the other ball goes through as well, even though it is fully behind the border at the time. collisionwalls2

KatieWoe commented 3 years ago

Behavior when stepping back past reflecting border. oddborderbehavior

jonathanolson commented 3 years ago

When Velocity or Size is changed, which includes the use of the constant size checkbox, the balls are returned to the edge of the screen which @arouinfar confirmed was an unintended behavior.

Does that mean I'll need to identify whether it was inside the play area BEFORE a user behavior? Presumably if a ball is inside of the play area (small, but on the edge of the play area) and the user increases the size, it would usually now be partially outside of the play area (and we bump it back in).

So... if a ball is outside the play area BEFORE editing, it should STAY outside? Does that sound correct @arouinfar?

jonathanolson commented 3 years ago

When Elasticity is changed while the balls are off screen, stepping back only goes to the point where elasticity was changed, which is an off screen location, while the blue reset button returns the balls in a manner similar to the "Return Balls" button, which is inconsistent with its usual behavior of returning balls to the last altered state.

This seems consistent with the stated goals of "don't ever save ball positions for the restart-balls button when one is outside the play area" and "don't let the user rewind past the point of an elasticity change". In that specific case, it seems impossible to find a single instant back in time where rewinding and restarting can use the same instant in time AND satisfy those conditions.

What should the desired behavior here be?

jonathanolson commented 3 years ago

Addition: Adding balls seems to show similar buggy behavior.

This is where the sim would otherwise save the state when the ball is added. However because one ball is outside the play area, this doesn't happen (as specified).

Perhaps the assumptions baked into the codebase aren't matching up with expectations?

jonathanolson commented 3 years ago

When a ball is halfway through the wall and the reflected wall is turned on, the ball still continues through.

This seems like by design, according to the simulation's code. Is there another behavior that is desired?

jonathanolson commented 3 years ago

When the balls are on the "stick" setting, they become one entity so if one ball goes through then the other ball goes through as well, even though it is fully behind the border at the time.

That definitely seems correct, and describes accurately how the code is handling it. This would be fairly tricky to change.

jonathanolson commented 3 years ago

@arouinfar let me know your thoughts on the above? Happy to discuss over a call if it would help.

arouinfar commented 3 years ago

Thanks @loganbraywork @KatieWoe @brooklynlash! You've documented some really interesting edge cases here. I've added my comments to each observation below, but the tl;dr is that off-screen balls are weird sometimes, but I don't think we need to make any changes.

When Velocity or Size is changed, which includes the use of the constant size checkbox, the balls are returned to the edge of the screen which @arouinfar confirmed was an unintended behavior.

Does that mean I'll need to identify whether it was inside the play area BEFORE a user behavior? Presumably if a ball is inside of the play area (small, but on the edge of the play area) and the user increases the size, it would usually now be partially outside of the play area (and we bump it back in).

So... if a ball is outside the play area BEFORE editing, it should STAY outside? Does that sound correct @arouinfar?

When @loganbraywork demonstrated this for me it looked really buggy, but after @jonathanolson's explanation I understand what the sim is doing now. Let's leave the behavior as-is. It's a bit of an edge case, anyway, and doesn't hurt anything.

When Elasticity is changed while the balls are off screen, stepping back only goes to the point where elasticity was changed, which is an off screen location, while the blue reset button returns the balls in a manner similar to the "Return Balls" button, which is inconsistent with its usual behavior of returning balls to the last altered state.

This seems consistent with the stated goals of "don't ever save ball positions for the restart-balls button when one is outside the play area" and "don't let the user rewind past the point of an elasticity change". In that specific case, it seems impossible to find a single instant back in time where rewinding and restarting can use the same instant in time AND satisfy those conditions.

What should the desired behavior here be?

I don't think we should be saving the state of off-screen things. Now that we've decided in #183 to disable the step back button for elasticity =/= 0 the latter goal of "don't let the user rewind past the point of an elasticity change" is moot. I don't think we need to do anything here @jonathanolson.

Addition: Adding balls seems to show similar buggy behavior.

This is where the sim would otherwise save the state when the ball is added. However because one ball is outside the play area, this doesn't happen (as specified).

We don't need to save the state of off-screen balls. I'll be sure to document this in the Teacher Tips.

When a ball is halfway through the wall and the reflected wall is turned on, the ball still continues through.

This seems like by design, according to the simulation's code. Is there another behavior that is desired?

Correct @jonathanolson this is by design and will be documented in the Teacher Tips.

When the balls are on the "stick" setting, they become one entity so if one ball goes through then the other ball goes through as well, even though it is fully behind the border at the time.

That definitely seems correct, and describes accurately how the code is handling it. This would be fairly tricky to change.

This is the correct behavior and consistent with the point above.