phetsims / keplers-laws

"Kepler's Laws" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 1 forks source link

Testing standard wrapper while playing results in the default orbit #215

Closed AgustinVallejo closed 10 months ago

AgustinVallejo commented 11 months ago

To reproduce, create any orbit, press play, and while playing test the standard wrapper. The resulting orbit is the default one and if doing to in Second Law Screen, now the areas are all messed up.

pixelzoom commented 11 months ago

We confirmed that this is NOT a problem in MSS.

pixelzoom commented 11 months ago

I spent ~30 minutes on this and did not make any progress. I tried using if ( !isSettingPhetioStateProperty.value ) to short-circuit SolarSystemCommonModel restart and loadBodyInfo, because it feels like that type of state problem.

Very mystifying...

AgustinVallejo commented 10 months ago

I found something related even weirder. When doing

    this.planet.positionProperty.link( position => {
      console.log( position.x, position.y );

I found this printed out when the standard wrapper was tested with the sim PAUSED!

image

What I find strange is that at some point it gets to its custom state, but seemingly goes back to (2,0) and never logs the correct value again. And yet, the body seems at the correct state.

AgustinVallejo commented 10 months ago

Seems like in MSS this also happens, although the default value only gets printed once instead of three times:

image

AgustinVallejo commented 10 months ago

Gotcha! I'm 99% sure this is due to this part in the engine.run() method:

// line 268
    const newPosition = this.createPolar( this.nu, this.w );

// inside the method line 452
    return EllipticalOrbitEngine.staticCreatePolar( this.a, this.e, nu, w );

All these are using uninstrumented numerical values which probably have in mind the default values. I'm going to see if I can update the engine before initially running here.

AgustinVallejo commented 10 months ago

The above push fixes this! There's still a small fix here: When testing the standard wrapper while playing, the sim forgets it's playing the orbital sound. Should be straightforward.

pixelzoom commented 10 months ago

@AgustinVallejo Rather than adding an entire additional if statement, which is probably getting executed when it does not need to be, this solutions feels a little cleaner, and addresses isPlayingProperty specifically. I have not pushed this, feel free to apply.

Subject: [PATCH] update
---
Index: js/common/model/KeplersLawsModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/KeplersLawsModel.ts b/js/common/model/KeplersLawsModel.ts
--- a/js/common/model/KeplersLawsModel.ts   (revision 79fd6a69f2ba45f7958257b45486c92bdd78a665)
+++ b/js/common/model/KeplersLawsModel.ts   (date 1702485667507)
@@ -249,11 +249,7 @@
         this.planet.velocityProperty
       ],
       () => {
-        if ( !this.isPlayingProperty.value && !this.steppingForward && !this.engine.isMutatingVelocity ) {
-          this.engine.update( this.bodies );
-        }
-        if ( isSettingPhetioStateProperty.value ) {
-          // Update the engine properties when initializing with PhET-iO
+        if ( ( !this.isPlayingProperty.value || isSettingPhetioStateProperty.value ) && !this.steppingForward && !this.engine.isMutatingVelocity ) {
           this.engine.update( this.bodies );
         }
       } );
pixelzoom commented 10 months ago

@AgustinVallejo said:

There's still a small fix here: When testing the standard wrapper while playing, the sim forgets it's playing the orbital sound. Should be straightforward.

We discussed with @jbphet. This is the expected behavior, due to the browser's AudioContext policy. The AudioContext (sound) will not run until the user interacts with the browser window.