Sim doesn't launch with correct avg temperature if mass is changed on Diffusion Screen

[Nancy-Salpepi]

Test device MacBook Air M1 chip

Operating System 14.5

Browser Safari 17.5

Problem description For https://github.com/phetsims/qa/issues/1107, in the Studio and State wrappers on the Diffusion Screen changing the Mass of either particle type will result in an incorrect Tavg in the Data Panel when the sim is launched.

Steps to reproduce

1. In Studio go to the Diffusion screen
2. Open the Data Panel
3. Add 20 Red Particles
4. Change the Mass to 30
5. Press Test

Visuals

[pixelzoom]

Reproduced in main. Very weird. T_avg should be the same as the Initial Temperature setting. In the launched sim, if I change Initial Temperature to 250, then back to 300, then T_avg is correct. I'm guessing there's an ordering issue with how state is restored that is resulting in a different result.

T_avg is the value of averageTemperatureProperty in DiffusionData. It is updated by calling DiffusionData update when the model is stepped. Relevant code in DiffusionModel:

  protected override stepModelTime( dt: number ): void {
    ...
    this.updateData();
  }

  /**
   * Updates the Data displayed for the left and right sides of the container.
   */
  private updateData(): void {
    this.leftData.update();
    this.rightData.update();
  }

[pixelzoom]

In the above commits, I tightened up the API for DiffusionData, which is where averageTemperatureProperty lives and is computed. This doesn't fix the problem, but ensures that nothing outside of DiffusionData is changing averageTemperatureProperty. These changes were patched into all 3 sims.

[pixelzoom]

If I omit step 4 (Change the Mass to 30) then T_avg is correct.

If I add this to DiffusionModel, the problem goes away. But this should not be necessary, because stepModelTime calls updateData on every time step.

if ( assert && Tandem.PHET_IO_ENABLED ) {
  phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => this.updateData() );
}

[pixelzoom]

Inspecting totalKE computed by DiffusionData update... With the sim running in Studio, totalKE is 74844000. In the Standard wrapper, it's 69854400. Since KE = (1/2) * m * |v|^2 (see Particle getKineticEnergy) this suggests that the mass (m) or speed (|v|) is incorrect in the State wrapper.

[pixelzoom]

DiffusionParticle extends Particle, and adds mass and radius fields. While ParticleIO exists for serializing Particle, we do not currently have DiffusionParticleIO. So the mass and radius fields are not being serialized, and will be incorrect until something occurs in the Standard wrapper (or Downstream sim) to update mass and radius.

Verified with this addition to DiffusionData update. The Standard wrapper reports mass=28 while the control shows a mass of 30.

    else {
      assert && assert( totalKE !== 0, 'totalKE should not be zero when the container is not empty' );
+     console.log( `totalKE=${totalKE} mass=${this.particleSystem.particles2[ 0 ].mass}` );

So we need DiffusionParticleIO. Ugh... This will be a big change.

[pixelzoom]

Looking more closely at ParticleStateObject, I see that it does include mass and radius:

export type ParticleStateObject = {
  mass: number;
  radius: number;
  x: number;
  y: number;
  _previousX: number;
  _previousY: number;
  vx: number;
  vy: number;
};

The difference with DiffusionParticle is that mass and radius are mutable. So something is apparently not setting mass (and radius?) when restoring the state of DiffusionParticle instances.

[pixelzoom]

The problem is in DiffusionParticleSystem, where there's some overly-zealous uses of isSettingPhetioStateProperty that are preventing mass, speed, and radius from being set; see below. Removing these guards should fix the problem, with no need for DiffusionParticleIO.

   Multilink.multilink(
      [ this.particle2Settings.massProperty, this.particle2Settings.initialTemperatureProperty ],
      ( mass, initialTemperature ) => {
        if ( !isSettingPhetioStateProperty.value ) {
          updateMassAndSpeed( mass, initialTemperature, this.particles2 );
        }
      } );

    // Update radii of existing particles.
    this.particle1Settings.radiusProperty.link( radius => {
      if ( !isSettingPhetioStateProperty.value ) {
        updateRadius( radius, this.particles1, container.leftBounds, isPlayingProperty.value );
      }
    } );
    this.particle2Settings.radiusProperty.link( radius => {
      if ( !isSettingPhetioStateProperty.value ) {
        updateRadius( radius, this.particles2, container.rightBounds, isPlayingProperty.value );
      }
    } );

[pixelzoom]

The fix is https://github.com/phetsims/gas-properties/commit/470663f44ee3362a46958ad558d30fdb21aad002, the other commits are patches for the 3 sim branches.

@Nancy-Salpepi please test in main. Leave open for verification in 1.1.0-rc.2.

[Nancy-Salpepi]

In the State wrapper when I follow similar steps to https://github.com/phetsims/gas-properties/issues/284#issue-2409582562, the downstream sim will launch with the correct average temperature after pressing Set State Now. If I then go back to the upstream sim and change the Number of Particles or the Radius and press Set State Now a second time, the average temperature will no longer match.

Steps:

1. In the State Wrapper set state rate =0
2. In the upstream sim go to the Diffusion Screen
3. Add 20 blue particles
4. Increase the Mass to 30
5. Open the Data Panel
6. Press Set State Now -- average temperatures match
7. In the upstream sim change the radius to 135 (or add some more blue particles)
8. Press Set State Now -- average temperatures no longer match

[pixelzoom]

Reproduced in main using the steps in https://github.com/phetsims/gas-properties/issues/284#issuecomment-2233117334.

Noting that this workaround (first mentioned in https://github.com/phetsims/gas-properties/issues/284#issuecomment-2231790104) does NOT address this new problem.

    if ( assert && Tandem.PHET_IO_ENABLED ) {
      phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => this.updateData() );
    }

[pixelzoom]

I previously noted:

While ParticleIO exists for serializing Particle, we do not currently have DiffusionParticleIO.

ParticleIO does not exist; we have LightParticleIO and HeavyParticleIO, DiffusionParticle1IO and DiffusionParticle2IO. I suspect that there's a serialization problem with DiffusionParticle1IO and DiffusionParticle1IO.

[pixelzoom]

This was indeed a serialization bug in DiffusionParticle1IO and DiffusionParticle2IO. While mass and radius were serialized correctly, there were not deserialized correctly. The mass and radius values that were saved were not being used to create the particle instance during deserialization. For example, here's the fix for DiffusionParticle1:

  /**
   * Deserializes an instance of DiffusionParticle1.
   */
  private static fromStateObject( stateObject: DiffusionParticle1StateObject ): DiffusionParticle1 {
    return new DiffusionParticle1( {
      x: stateObject.x,
      y: stateObject.y,
      previousX: stateObject._previousX,
      previousY: stateObject._previousY,
      vx: stateObject.vx,
-     vy: stateObject.vy
+     vy: stateObject.vy,
+     mass: stateObject.mass,
+     radius: stateObject.radius
    } );
  }

The primary commit is https://github.com/phetsims/gas-properties/commit/cab2dc59131c0a73d0165862c7f631e6f8cd26dd, the others are related to patching release branches for the 3 sims.

I tested both the original problem (https://github.com/phetsims/gas-properties/issues/284#issue-2409582562) and the more recent problem (https://github.com/phetsims/gas-properties/issues/284#issuecomment-2233117334).

[pixelzoom]

@Nancy-Salpepi please test in main. Leave open for verification in 1.1.0-rc.2.

[Nancy-Salpepi]

This looks good now on main!

[pixelzoom]

Please verify for https://github.com/phetsims/qa/issues/1123 and https://github.com/phetsims/qa/issues/1125. (This issue is irrelevant for the Gases Intro sim.)

To verify: (1) Follow "Steps to reproduce" in https://github.com/phetsims/gas-properties/issues/284#issue-2409582562. (2) Follow "Steps to reproduce" in https://github.com/phetsims/gas-properties/issues/284#issuecomment-2233117334.

If everything looks OK, please close this issue.

[Nancy-Salpepi]

looks good in rc.2 for Gas Properties and Diffusion sims. Closing