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

Unable to step back inelastic collisions looks buggy #183

Closed KatieWoe closed 3 years ago

KatieWoe commented 4 years ago

Test device Lovelace Operating System ChromeOS Browser Chrome Problem description For https://github.com/phetsims/QA/issues/562 If the collisions in the sim are inelastic you are unable to use the step back button. @jonathanolson said on Slack:

// The step-backward button is only enabled when the sim is paused, the elasticity is 100%, and the total elapsed // time isn't 0. There isn't any support to provide a custom enabledProperty to step-buttons. So, we use a // a workaround. See https://github.com/phetsims/scenery-phet/issues/606 and // https://github.com/phetsims/collision-lab/issues/66. DerivedProperty never disposed since // CollisionLabTimeControlNode persists for the lifetime of simulation.

This makes sense, however in practice it comes across as inconsistent behavior. It also is particularly noticeable on the Inelastic screen, where activating the step back button seems to be impossible from what I can tell. I also noticed that if collisions are inelastic, the sim plays, and you change them to elastic, you can use the step back buttons to go back in time in a manner that it didn't behave, if that makes sense (not sure how to describe this.

Not sure what a solution would look like, but I thought it was worth bringing up. Especially on the Inelastic screen.

Visuals inelasticcantgoback

KatieWoe commented 4 years ago

This is an example of the step back button showing a sort of alternate history. alternatehistory

jonathanolson commented 4 years ago

@arouinfar, thoughts on how to proceed? It seems like we'd be gaining energy (which is why I'd guess it might not be enabled). Also should we in general avoid cases of alternate history?

arouinfar commented 4 years ago

I'm pretty sure the reason the step back button is disabled if the elasticity is less than 100% is because the physics will be inaccurate. We should remove the button from the Inelastic screen because it will never be enabled.

Honestly, I'm okay with the alternative history when stepping back so long as the physics is accurate. Right now, it looks like momentum is not conserved when stepping back. For example:

  1. Explore 2D, default configuration, Kinetic Energy on, Values on, elasticity 50%
  2. Press play
  3. After the first collision, pause the sim and take note of the KE and momentum.
  4. Set elasticity to 100%
  5. Step back until the balls collide. The KE is conserved, but the momentum increases.
jonathanolson commented 3 years ago

I'd like more details on the momentum increasing. It seems more like the momentum vectors of each ball change so that the sum of their individual magnitudes is more, while the net momentum's magnitude stays the same in this case?

In the Momenta Diagram, I see the total momentum vector stay the same also.

veillette commented 3 years ago

In principle, the total momenta should stay the same if the collision reversed. However, the kinetic energy should increased if the elasticity is set to anything to less than one.

veillette commented 3 years ago

After using the simulation in class, I concur with @KatieWoe that it makes the simulation difficult to operate.

A typical use of the simulation is to set up two ball to a particular initial states (masses, positions and velocities) and then observe the results from the collision.

If one did not pause the simulation after the first collision, then the balls have undoubtedly collided with the walls, another balls, etc, and it is too late for them to collect any data. I have seen a few students making that mistake repeatedly. Because there is no step back button, they cannot wind back the clock, and therefore they have to go back to the tedious process of setting the positions and velocities again.

I understand that by rewinding the clock, the kinetic energy becomes unbounded, but one suggestion could be to develop a heuristic argument that once the speed of a ball exceeds a particular threshold say 10m/s, then the rewind button would be disabled. In practice, it would mean that for most cases, it would give the students plenty of rewind "time", but would prevent playback problems associated when the speeds become too high.

As @arouinfar mentioned, the approach described above cannot work for the inelastic screen, so the rewind button should be disabled.

jonathanolson commented 3 years ago

Design meeting: For elasticity > 0, step back works until the "reset point", elasticity = 0 it will be disabled. Remove fully from the inelastic screen.

arouinfar commented 3 years ago

Thanks for your feedback @veillette!

We think the step back is a valuable control, but it's not particularly useful if the state (e.g. mass, elasticity) has changed since it is no longer representative of events that actually happened in the past. Changing the number of balls, mass, velocity, position, or elasticity saves a new state which can be restored by the reset button under the collision window. We decided to enable the step back button until the user reaches this saved state (so long as elasticity > 0). This way, users can step back until the point where the state was changed and they won't encounter a false history.

jonathanolson commented 3 years ago

This is implemented (and also included the implementation of backward stepping with elasticity, oops). @arouinfar can you give it a try?

arouinfar commented 3 years ago

The step back behavior looks great in master, but I realized that elasticity is not part of the state, so I opened #190.

KatieWoe commented 3 years ago

In 1.1.0 dev 13, adding or taking away a ball does not seem to restart the clock. addingballsrestart I also noticed that just entering the text box to change mass, position, and velocity resets the clock, even if you didn't make any change. This may be the best option however. enteringboxrestarts

KatieWoe commented 3 years ago

Talked to @arouinfar and I understand the expected behavior a bit better now. I noticed another oddity on how it is handled when a ball is taken away or added. If it was added, pressing the blue reset button will take you back to when the ball was added at 0, even though that point wasn't set at 0 before pressing the blue button. If you take a ball away, the blue button will take you back to the wrong reset position, the one that would be set before a ball was removed. The ball will still be gone. addrefresh takeawayrefresh

KatieWoe commented 3 years ago

Another odd feature: When a change is made with one ball outside the play area due to a lack of reflecting barrier (such as on the first screen), the step back button takes you back to the change, but the blue restart button takes you back to the last state where both balls were there. oddblueresetoffscreen

jonathanolson commented 3 years ago

When a change is made with one ball outside the play area due to a lack of reflecting barrier (such as on the first screen), the step back button takes you back to the change, but the blue restart button takes you back to the last state where both balls were there.

This is in the code explicitly, there's a check for whether balls are all in the play area.

KatieWoe commented 3 years ago

Should the state be changed on the 4th screen when tick and slip are switched? There's no step back button, so it seems to matter less.

KatieWoe commented 3 years ago

I also notice that it doesn't seem to change when you check the constant size checkbox, which can cause further errors.

arouinfar commented 3 years ago

I noticed another oddity on how it is handled when a ball is taken away or added. If it was added, pressing the blue reset button will take you back to when the ball was added at 0, even though that point wasn't set at 0 before pressing the blue button. If you take a ball away, the blue button will take you back to the wrong reset position, the one that would be set before a ball was removed. The ball will still be gone.

This is a great find @KatieWoe, and seems inconsistent with how we handle other model-changing parameters (e.g. mass).

Should the state be changed on the 4th screen when tick and slip are switched? There's no step back button, so it seems to matter less.

I don't think it's necessary. I think there was some discussion about this previously, but I couldn't find the issue.

I also notice that it doesn't seem to change when you check the constant size checkbox, which can cause further errors.

@KatieWoe if you haven't already, please open issues for the bugs you've seen related to Constant Size.

@jonathanolson I'd be fine with resetting the timer/overriding the blue reset button when constant size is checked if that's easiest.

KatieWoe commented 3 years ago

https://github.com/phetsims/collision-lab/issues/193 made for constant size issue.

jonathanolson commented 3 years ago

Are there any changes recommended in this issue (besides the pulled-out constant size issue?)

KatieWoe commented 3 years ago

The number of balls changing didn't seem to be fully handled https://github.com/phetsims/collision-lab/issues/183#issuecomment-756429346.

jonathanolson commented 3 years ago

I believe the above commit should handle that, can you verify?

arouinfar commented 3 years ago

Looks good in master @jonathanolson

KatieWoe commented 3 years ago

Looks dood on master

KatieWoe commented 3 years ago

Leaving open for now due to https://github.com/phetsims/collision-lab/issues/199, unless @jonathanolson thinks they are separate enough and this can be closed.

KatieWoe commented 3 years ago

Actually, I am seeing buggy behavior for slightly inelastic collisions being stepped back, where the rewinding behavior does not match what it was going forward. Recorded on Explore 1D. For https://github.com/phetsims/QA/issues/599 backstepwrong Also reproduced on 2D screen

KatieWoe commented 3 years ago

While attempting to reproduce on 2D screen, the sim froze with this console error building up. @jonathanolson is this a separate issue?

frozenscreen
KatieWoe commented 3 years ago

Example of very odd behavior on 2d screen oddbackstep

arouinfar commented 3 years ago

@jonathanolson seems like the solution might be to go back to disabling the step back button for elasticity =/= 0.

jonathanolson commented 3 years ago

I'm hitting this behavior also randomly.

Screen Shot 2021-02-01 at 6 22 22 PM Screen Shot 2021-02-01 at 6 22 28 PM

I believe it's due to numerical imprecision that gets magnified significantly for every collision (and glancing collisions magnify it also).

So, I'm inclined to say we should disable the step-back for inelastic collisions. @arouinfar thoughts?

arouinfar commented 3 years ago

So, I'm inclined to say we should disable the step-back for inelastic collisions. @arouinfar thoughts?

Sounds good!

jonathanolson commented 3 years ago

Implemented, can you verify?

arouinfar commented 3 years ago

@jonathanolson so sorry for the delay. Looks good in master!

KatieWoe commented 3 years ago

Did some exploring and this does look ok in rc.3. The step back button is disabled for inelastic collisions and I wasn't able to reproduce this while stepping back elastic collisions so far. I did sometimes see differing behavior when stepping back, mentioned in https://github.com/phetsims/collision-lab/issues/205#issuecomment-778412430. Since that was deemed acceptable I think this can be closed. Will reopen if something comes up. gobackisoff