Use PhetioGroup

[zepumph]

From #203 and phetsims/tandem#87

This may not take too long, and would be good cleanup. @jessegreenberg is there a timeline on taking RC shas for this sim? I think we should do this work afterwards.

[jessegreenberg]

Yep! We are hoping to grab RC shas by June 9.

[zepumph]

Unassigning until we work on PhET-iO in this sim.

[samreid]

The current pattern is now throwing errors on CT, so I decided to try it out. But I am stuck on a count mismatch on the destination frame. Here's my progress patch:

Index: js/photon-absorption/model/Molecule.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/photon-absorption/model/Molecule.js	(revision a19579b6e7c6bee878bb98a7e8d31926c4df2fc5)
+++ js/photon-absorption/model/Molecule.js	(date 1601999176766)
@@ -148,7 +148,7 @@
   } );
 
   // @public, set by PhotonAbsorptionModel
-  this.photonGroupTandem = null;
+  this.photonGroup = null;
 
   // @public (read-only) {Emitter} - emitter for when a photon is absorbed
   this.photonAbsorbedEmitter = new Emitter( { parameters: [ { valueType: Photon } ] } );
@@ -509,7 +509,7 @@
    * @param {number} wavelength - The photon to be emitted.
    **/
   emitPhoton: function( wavelength ) {
-    const photonToEmit = new Photon( wavelength, this.photonGroupTandem.createNextTandem() );
+    const photonToEmit = this.photonGroup.createNextElement( wavelength );
     const emissionAngle = phet.joist.random.nextDouble() * Math.PI * 2;
     photonToEmit.setVelocity( PHOTON_EMISSION_SPEED * Math.cos( emissionAngle ),
       ( PHOTON_EMISSION_SPEED * Math.sin( emissionAngle ) ) );
Index: js/photon-absorption/model/PhotonAbsorptionModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/photon-absorption/model/PhotonAbsorptionModel.js	(revision a19579b6e7c6bee878bb98a7e8d31926c4df2fc5)
+++ js/photon-absorption/model/PhotonAbsorptionModel.js	(date 1601998542458)
@@ -26,6 +26,7 @@
 import EnumerationIO from '../../../../phet-core/js/EnumerationIO.js';
 import inherit from '../../../../phet-core/js/inherit.js';
 import TimeSpeed from '../../../../scenery-phet/js/TimeSpeed.js';
+import PhetioGroup from '../../../../tandem/js/PhetioGroup.js';
 import PhetioObject from '../../../../tandem/js/PhetioObject.js';
 import IOType from '../../../../tandem/js/types/IOType.js';
 import NumberIO from '../../../../tandem/js/types/NumberIO.js';
@@ -63,7 +64,7 @@
 const EMITTER_OFF_EMISSION_PERIOD = Number.POSITIVE_INFINITY;
 
 // when stepping at "slow" speed, animate rate is reduced by this factor
-const SLOW_SPEED_FACTOR = 0.5;
+const SLOW_SPEED_FACTOR = 0.1;
 
 /**
  * Constructor for a photon absorption model.
@@ -79,7 +80,10 @@
   this.photonAbsorptionModel = tandem; // @private
 
   // @private
-  this.photonGroupTandem = tandem.createGroupTandem( 'photons' );
+  this.photonGroup = new PhetioGroup( ( tandem, wavelength ) => new Photon( wavelength, tandem ), [ WavelengthConstants.IR_WAVELENGTH ], {
+    phetioType: PhetioGroup.PhetioGroupIO( Photon.PhotonIO ),
+    tandem: tandem.createTandem( 'photonGroup' )
+  } );
 
   // @public - Property that indicating whether photons are being emitted from the photon emittter
   this.photonEmitterOnProperty = new BooleanProperty( false, {
@@ -250,6 +254,8 @@
       this.photonEmissionCountdownTimer -= dt;
       if ( this.photonEmissionCountdownTimer <= 0 ) {
         // Time to emit.
+
+        console.log(phet.preloads.phetio.queryParameters.frameTitle, 'timer emit photon');
         this.emitPhoton( Math.abs( this.photonEmissionCountdownTimer ) );
         this.photonEmissionCountdownTimer = this.photonEmissionPeriodTarget;
       }
@@ -339,7 +345,7 @@
    * frames.
    */
   emitPhoton: function( advanceAmount ) {
-    const photon = new Photon( this.photonWavelengthProperty.get(), this.photonGroupTandem.createNextTandem() );
+    const photon = this.photonGroup.createNextElement( this.photonWavelengthProperty.get() );
     photon.positionProperty.set( new Vector2( PHOTON_EMISSION_POSITION.x + PHOTON_VELOCITY * advanceAmount, PHOTON_EMISSION_POSITION.y ) );
     const emissionAngle = 0; // Straight to the right.
     photon.setVelocity( PHOTON_VELOCITY * Math.cos( emissionAngle ), PHOTON_VELOCITY * Math.sin( emissionAngle ) );
@@ -435,8 +441,8 @@
     this.targetMolecule = newMolecule;
     this.activeMolecules.add( newMolecule );
 
-    // Set the photonGroupTandem so that photons created by the molecule can be registered for PhET-iO
-    newMolecule.photonGroupTandem = this.photonGroupTandem;
+    // Set the photonGroup so that photons created by the molecule can be registered for PhET-iO
+    newMolecule.photonGroup = this.photonGroup;
 
     // Emit a new photon from this molecule after absorption.
     newMolecule.photonEmittedEmitter.addListener( function( photon ) {
Index: js/photon-absorption/model/Photon.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/photon-absorption/model/Photon.js	(revision a19579b6e7c6bee878bb98a7e8d31926c4df2fc5)
+++ js/photon-absorption/model/Photon.js	(date 1601998478172)
@@ -12,6 +12,7 @@
 import Vector2 from '../../../../dot/js/Vector2.js';
 import Vector2Property from '../../../../dot/js/Vector2Property.js';
 import inherit from '../../../../phet-core/js/inherit.js';
+import CouldNotYetDeserializeError from '../../../../tandem/js/CouldNotYetDeserializeError.js';
 import PhetioObject from '../../../../tandem/js/PhetioObject.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 import IOType from '../../../../tandem/js/types/IOType.js';
@@ -27,6 +28,8 @@
  */
 function Photon( wavelength, tandem ) {
 
+  console.log(phet.preloads.phetio.queryParameters.frameTitle, 'created photon');
+
   this.positionProperty = new Vector2Property( new Vector2( 0, 0 ), {
     tandem: tandem.createTandem( 'positionProperty' )
   } );
@@ -39,7 +42,8 @@
   // Photons in the play area are instrumented, those in the control panel (for icons) are not.
   PhetioObject.call( this, {
     tandem: tandem,
-    phetioType: Photon.PhotonIO
+    phetioType: Photon.PhotonIO,
+    phetioDynamicElement: true
   } );
 }
 
@@ -75,38 +79,27 @@
 } );
 
 Photon.PhotonIO = new IOType( 'PhotonIO', {
-  valueType: Photon,
-  toStateObject: photon => ( {
-    vx: NumberIO.toStateObject( photon.vx ),
-    vy: NumberIO.toStateObject( photon.vy ),
-    wavelength: NumberIO.toStateObject( photon.wavelength ),
-    phetioID: photon.tandem.phetioID
-  } ),
+    valueType: Photon,
+    toStateObject: photon => ( {
+      vx: NumberIO.toStateObject( photon.vx ),
+      vy: NumberIO.toStateObject( photon.vy ),
+      wavelength: NumberIO.toStateObject( photon.wavelength ),
+      phetioID: photon.tandem.phetioID
+    } ),
 
-  /**
-   * This is sometimes data-type and sometimes reference-type serialization, if the photon has already be created,
-   * then use it.
-   * @public
-   * @override
-   *
-   * @param {Object} stateObject
-   * @returns {Photon}
-   */
-  fromStateObject( stateObject ) {
-    let photon;
-    if ( phet.phetio.phetioEngine.hasPhetioObject( stateObject.phetioID ) ) {
-      photon = phet.phetio.phetioEngine.getPhetioObject( stateObject.phetioID );
-    }
-    else {
-      photon = new phet.moleculesAndLight.Photon( NumberIO.fromStateObject( stateObject.wavelength ),
-        Tandem.createFromPhetioID( stateObject.phetioID ) );
-    }
+    // TODO: Should this use ReferenceIO or other shared code? https://github.com/phetsims/tandem/issues/215
+    fromStateObject: stateObject => {
+      const phetioID = stateObject.phetioID;
+      if ( phet.phetio.phetioEngine.hasPhetioObject( phetioID ) ) {
+        return phet.phetio.phetioEngine.getPhetioObject( phetioID );
+      }
+      else {
+        throw new CouldNotYetDeserializeError();
+      }
+    },
 
-    photon.wavelength = NumberIO.fromStateObject( stateObject.wavelength );
-    photon.setVelocity( NumberIO.fromStateObject( stateObject.vx ), NumberIO.fromStateObject( stateObject.vy ) );
-
-    return photon;
+    stateToArgsForConstructor: stateObject => [ stateObject.wavelength ]
   }
-} );
+);
 
 export default Photon;

[samreid]

The main ingredient I was missing was to convert photon.dispose to photonGroup.disposeElement(photon). After adding that, things are working much better. @zepumph can you please review at your convenience?

[zepumph]

RE: #375, I'm a bit surprised to see that general registration assertion instead of a more specific one built into trying to add/remove elements during state, like https://github.com/phetsims/tandem/blob/607bb16eef2b0f1335a1e1f886ed3fa16637e07b/js/PhetioDynamicElementContainer.js#L320-L322. Do you think there could be a way to check on this centrally? Or perhaps not, because the bug is that you aren't going through PhetioGroup, and are calling the dynamic element dispose call directly.

As far as I could tell, PhotonAbsorbsionModel.photons was a central container for all photons. Could you please describe your reasoning in creating a photonGroup instead of converting photons to be the PhetioGroup? We did the conversion strategy early on in JT and CAF, and it worked well, though it was a fair bit of phet-io take over. I feel like @pixelzoom was a strong proponent of not doing this pattern in Natrual Selection, but I'm curious if you could speak to your rationale, so that we can continue to refine our pattern as a general project.

Otherwise things are looking really good.

[samreid]

Great idea @zepumph! I changed that in the commit. Can you please review that part?

Do you think there could be a way to check on this centrally? Or perhaps not, because the bug is that you aren't going through PhetioGroup, and are calling the dynamic element dispose call directly.

I wasn't clear on this, can you elaborate or brainstorm some possibilities?

[zepumph]

7103b2d looks really nice. I love the cleanup and uninstrumentation of the model.

I wasn't clear on this, can you elaborate or brainstorm some possibilities?

I have been trying to make the conversion to/use of PhetioGroup as simple as possible, but right now there are still some wrong things that can yield some awkward error messages. In this particular case, if you call Photon.dispose() directly instead of photonGroup.disposeElement(). I can't think of a way to make a nicer assertion for this, but wanted to brainstorm with you.

[samreid]

Sounds good, what if close this issue and open a tandem issue about the last point? Also, we may have limited options regarding Photon.dispose() vs photonGroup.disposeElement() if we code the member elements (like Photon) so that they are unaware they are group members.

[zepumph]

I think I would prefer to just close, and keep it in our minds for the next times we are using PhetioGroup and run into this.