Instrument EnergyChunks as dynamic elements (PhetioGroup)

[zepumph]

After discussion with @jbphet last Thursday, I feel like I am well briefed on the structure of the EnergyChunks and how I want to begin investigation to add PhET-iO dynamic element support to them.

Brief notes from our meeting:

https://github.com/phetsims/energy-forms-and-changes/issues/306
EnergySystemElement.preloadEnergyChunks overrides
energyChunksVisibleProperty calls preloadEnergyChunks
EnergyChunkPathMover.nextPoint will need to be serialized

care about time, and KP wants an estimate 

 PhetioGroup for:
  * EnergyChunk
  * EnergyChunkNode
  * EnergyChunkMover

[zepumph]

Currently we are only trying to add support for this to the second screen. It wasn't totally clear if this is needed for the first screen. It may be the case that it is easy to add to the first once the second is in good shape.

[zepumph]

In the last 2.5 hours I made progress on this issue.

Work done so far:

1. Isolated the systems model to a single source/converter/user (biker/generator/beakerHeater.
2. Instrumented EnergyChunk and EnergyChunkMover as dynamic elements with state methods
3. Created a single PhetioGroup for each in the systems model, and passed those to each EnergySystemElement
4. EnergyChunks were already well supported in an ObservableArray, I just instrumented that.
5. I converted all native arrays of path movers to instrumented ObservableArrays.
6. I spent about 40 minutes working our tweaks/bugs in that implementation so far.

Currently the state wrapper will set state on the startup screen on a sim, but breaks when adding EnergyChunks

Up next:

1. Continue to work through assert errors in the state wrapper.
2. add this same pattern to the rest of the system elements once it is working with these three.

Patch so far

```diff Index: js/common/model/EnergyChunk.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/common/model/EnergyChunk.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/common/model/EnergyChunk.js (date 1599517930588) @@ -8,43 +8,98 @@ import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js'; import NumberProperty from '../../../../axon/js/NumberProperty.js'; +import PropertyIO from '../../../../axon/js/PropertyIO.js'; import Vector2 from '../../../../dot/js/Vector2.js'; import Vector2Property from '../../../../dot/js/Vector2Property.js'; +import merge from '../../../../phet-core/js/merge.js'; +import PhetioObject from '../../../../tandem/js/PhetioObject.js'; +import Tandem from '../../../../tandem/js/Tandem.js'; +import BooleanIO from '../../../../tandem/js/types/BooleanIO.js'; +import ObjectIO from '../../../../tandem/js/types/ObjectIO.js'; +import ReferenceIO from '../../../../tandem/js/types/ReferenceIO.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import EnergyType from './EnergyType.js'; // static data let instanceCount = 0; // counter for creating unique IDs -class EnergyChunk { +class EnergyChunk extends PhetioObject { /** + * TODO: better way to handle defaults for initial values for instrumented sub-Properties * @param {EnergyType} initialEnergyType * @param {Vector2} initialPosition * @param {Vector2} initialVelocity * @param {BooleanProperty} visibleProperty + * @param {Object} [options] */ - constructor( initialEnergyType, initialPosition, initialVelocity, visibleProperty ) { + constructor( initialEnergyType, initialPosition, initialVelocity, visibleProperty, options ) { + + options = merge( { + + // phet-io + tandem: Tandem.OPTIONAL, // TODO: should this become REQUIRED? + phetioType: EnergyChunkIO, + phetioDynamicElement: true, + + id: null // to support recreating the same energyChunk through PhET-iO state + }, options ); + + super( options ); // @public - this.positionProperty = new Vector2Property( initialPosition, { useDeepEquality: true } ); + this.positionProperty = new Vector2Property( initialPosition, { + useDeepEquality: true, + tandem: options.tandem.createTandem( 'positionProperty' ) + } ); // @public - for simple 3D layering effects - this.zPositionProperty = new NumberProperty( 0 ); + this.zPositionProperty = new NumberProperty( 0, { + tandem: options.tandem.createTandem( 'zPositionProperty' ) + } ); // @public - this.energyTypeProperty = new EnumerationProperty( EnergyType, initialEnergyType ); + this.energyTypeProperty = new EnumerationProperty( EnergyType, initialEnergyType, { + tandem: options.tandem.createTandem( 'energyTypeProperty' ) + } ); // @public this.visibleProperty = visibleProperty; + assert && Tandem.VALIDATION && this.isPhetioInstrumented() && assert( this.visibleProperty.isPhetioInstrumented(), + 'if this EnergyChunk is instrumented, then the visibleProperty should be too' ); + // @public (read-only) {number} - an ID that will be used to track this energy chunk - this.id = instanceCount++; + this.id = options.id || instanceCount++; // @public (read-only) {Vector2} - for performance reasons, this is allocated once and should never be overwritten this.velocity = new Vector2( initialVelocity.x, initialVelocity.y ); } + // @public + toStateObject() { + return { + id: this.id, + velocity: this.velocity, + visibleProperyPhetioID: this.visibleProperty.tandem.phetioID, + phetioID: this.tandem.phetioID + }; + } + + // @public + static stateToArgsForConstructor( stateObject ) { + const visibleProperty = ReferenceIO( PropertyIO( BooleanIO ) ).fromStateObject( stateObject.visibleProperyPhetioID ) + return [ EnergyType.HIDDEN, Vector2.ZERO, Vector2.fromStateObject( stateObject.velocity ), visibleProperty, { id: stateObject.id } ]; + } + + /** + * @public + * @param stateObject + */ + applyState( stateObject ) { + this.visibleProperty = ReferenceIO( PropertyIO( BooleanIO ) ).fromStateObject( stateObject.visibleProperyPhetioID ); + } + /** * set the position * @param {number} x @@ -102,7 +157,44 @@ this.energyTypeProperty.reset(); this.visibleProperty.reset(); } + + + /** + * @public + * @override + */ + dispose() { + // this.visibleProperty = null; + this.positionProperty.dispose(); + this.zPositionProperty.dispose(); + this.energyTypeProperty.dispose(); + super.dispose(); + } } + +// Strategy A +class EnergyChunkIO extends ObjectIO { + + // @public @override + static toStateObject( energyChunk ) { return energyChunk.toStateObject(); } + + // @public @override + static applyeState( energyChunk ) { return energyChunk.applyeState(); } + + // @public @override + static stateToArgsForConstructor( state ) { return EnergyChunk.stateToArgsForConstructor( state ); } + + // @public - use refence serialization when a member of another data structure like ObservableArray + static fromStateObject( stateObject ) { + return ReferenceIO( EnergyChunkIO ).fromStateObject( stateObject.phetioID ); + } +} + +EnergyChunkIO.documentation = 'My Documentation'; +EnergyChunkIO.typeName = 'EnergyChunkIO'; +EnergyChunkIO.validator = { valueType: EnergyChunk }; + +EnergyChunk.EnergyChunkIO = EnergyChunkIO; energyFormsAndChanges.register( 'EnergyChunk', EnergyChunk ); export default EnergyChunk; \ No newline at end of file Index: js/systems/model/EnergyConverter.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/EnergyConverter.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/EnergyConverter.js (date 1599516176290) @@ -8,6 +8,9 @@ * @author Andrew Adare */ +import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; +import EnergyChunk from '../../common/model/EnergyChunk.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import EnergySystemElement from './EnergySystemElement.js'; @@ -19,8 +22,14 @@ */ constructor( iconImage, tandem ) { super( iconImage, tandem ); - this.incomingEnergyChunks = []; - this.outgoingEnergyChunks = []; + this.incomingEnergyChunks = new ObservableArray( { + tandem: tandem.createTandem( 'incomingEnergyChunks' ), + phetioType: ObservableArrayIO( EnergyChunk.EnergyChunkIO ) + } ); + this.outgoingEnergyChunks = new ObservableArray( { + tandem: tandem.createTandem( 'outgoingEnergyChunks' ), + phetioType: ObservableArrayIO( EnergyChunk.EnergyChunkIO ) + } ); } /** @@ -31,7 +40,10 @@ */ extractOutgoingEnergyChunks() { this.energyChunkList.removeAll( this.outgoingEnergyChunks ); - return this.outgoingEnergyChunks.splice( 0 ); + + const outgoingEnergyChunksCopy = this.outgoingEnergyChunks.getArrayCopy(); + this.outgoingEnergyChunks.clear(); + return outgoingEnergyChunksCopy; } /** @@ -41,7 +53,13 @@ * @public */ injectEnergyChunks( energyChunks ) { - this.incomingEnergyChunks = _.union( this.incomingEnergyChunks, energyChunks ); + + // TODO: I'm worried about this n^2 algorithm + this.incomingEnergyChunks.forEach( energyChunk => { + if ( !energyChunks.includes( energyChunk ) ) { + this.incomingEnergyChunks.remove( energyChunk ); + } + } ); } /** @@ -50,8 +68,8 @@ */ clearEnergyChunks() { super.clearEnergyChunks(); - this.incomingEnergyChunks.length = 0; - this.outgoingEnergyChunks.length = 0; + this.incomingEnergyChunks.clear(); + this.outgoingEnergyChunks.clear(); } } Index: js/systems/model/EnergyChunkPathMover.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/EnergyChunkPathMover.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/EnergyChunkPathMover.js (date 1599516444384) @@ -9,24 +9,42 @@ */ import Vector2 from '../../../../dot/js/Vector2.js'; +import Vector2IO from '../../../../dot/js/Vector2IO.js'; +import merge from '../../../../phet-core/js/merge.js'; +import PhetioObject from '../../../../tandem/js/PhetioObject.js'; +import Tandem from '../../../../tandem/js/Tandem.js'; +import ArrayIO from '../../../../tandem/js/types/ArrayIO.js'; +import ObjectIO from '../../../../tandem/js/types/ObjectIO.js'; +import ReferenceIO from '../../../../tandem/js/types/ReferenceIO.js'; import EFACConstants from '../../common/EFACConstants.js'; import EnergyChunk from '../../common/model/EnergyChunk.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; -class EnergyChunkPathMover { +class EnergyChunkPathMover extends PhetioObject { /** * @param {EnergyChunk} energyChunk - energy chunk to be moved * @param {Vector2[]} path - points along energy chunk path * @param {number} speed - in meters per second + * @param {Object} [options] */ - constructor( energyChunk, path, speed ) { + constructor( energyChunk, path, speed, options ) { + + options = merge( { + + // phet-io + tandem: Tandem.OPTIONAL, // TODO: should this become REQUIRED? + phetioType: EnergyChunkPathMoverIO, + phetioDynamicElement: true + }, options ); // validate args assert && assert( energyChunk instanceof EnergyChunk, `energyChunk is not of correct type: ${energyChunk}` ); assert && assert( path.length > 0, 'Path must have at least one point' ); assert && assert( speed >= 0, `speed must be a non-negative scalar. Received: ${speed}` ); + super( options ); + // @public (read-only) {EnergyChunk} this.energyChunk = energyChunk; @@ -37,6 +55,33 @@ this.nextPoint = path[ 0 ]; } + // @public + toStateObject() { + return { + path: ArrayIO( Vector2IO ).toStateObject( this.path ), + speed: this.speed, + pathFullyTraversed: this.pathFullyTraversed, + nextPoint: this.nextPoint, + energyChunkPhetioID: this.energyChunk.tandem.phetioID, + phetioID: this.tandem.phetioID + }; + } + + // @public + static stateToArgsForConstructor( stateObject ) { + const energyChunk = ReferenceIO( EnergyChunk.EnergyChunkIO ).fromStateObject( stateObject.energyChunkPhetioID ); + const path = ArrayIO( Vector2IO ).toStateObject( stateObject.path ); + return [ energyChunk, path, stateObject.speed ]; + } + + + // @public + applyState( stateObject ) { + this.pathFullyTraversed = stateObject.pathFullyTraversed; + this.nextPoint = stateObject.nextPoint; + } + + /** * advance chunk position along the path * @param {number} dt - time step in seconds @@ -167,7 +212,40 @@ return path; } + + /** + * @public + * @override + */ + dispose() { + // this.energyChunk = null; + super.dispose(); + } } + +// Strategy A +class EnergyChunkPathMoverIO extends ObjectIO { + + // @public @override + static toStateObject( energyChunk ) { return energyChunk.toStateObject(); } + + // @public @override + static stateToArgsForConstructor( state ) { return EnergyChunkPathMover.stateToArgsForConstructor( state ); } + + // @public - use refence serialization when a member of another data structure like ObservableArray + static fromStateObject( stateObject ) { + return ReferenceIO( EnergyChunkPathMoverIO ).fromStateObject( stateObject.phetioID ); + } + + // @public @override + static applyState( energyChunk, stateObject ) { energyChunk.applyState( stateObject ); } +} + +EnergyChunkPathMoverIO.documentation = 'My Documentation'; +EnergyChunkPathMoverIO.typeName = 'EnergyChunkPathMoverIO'; +EnergyChunkPathMoverIO.validator = { valueType: EnergyChunkPathMover }; + +EnergyChunkPathMover.EnergyChunkPathMoverIO = EnergyChunkPathMoverIO; energyFormsAndChanges.register( 'EnergyChunkPathMover', EnergyChunkPathMover ); export default EnergyChunkPathMover; \ No newline at end of file Index: js/systems/model/Biker.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/Biker.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/Biker.js (date 1599516063984) @@ -8,6 +8,8 @@ */ import NumberProperty from '../../../../axon/js/NumberProperty.js'; +import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; import Range from '../../../../dot/js/Range.js'; import Vector2 from '../../../../dot/js/Vector2.js'; import Image from '../../../../scenery/js/nodes/Image.js'; @@ -49,9 +51,13 @@ /** * @param {Property.} energyChunksVisibleProperty * @param {Property.} mechanicalPoweredSystemIsNextProperty - is a compatible energy system currently active + * @param {EnergyChunkPhetioGroup} energyChunkPhetioGroup + * @param {EnergyChunkPathMoverPhetioGroup} energyChunkPathMoverPhetioGroup * @param {Tandem} tandem */ - constructor( energyChunksVisibleProperty, mechanicalPoweredSystemIsNextProperty, tandem ) { + constructor( energyChunksVisibleProperty, mechanicalPoweredSystemIsNextProperty, + energyChunkPhetioGroup, energyChunkPathMoverPhetioGroup, + tandem ) { super( new Image( BICYCLE_ICON ), tandem ); // @public {string} - a11y name @@ -107,10 +113,19 @@ // @private - internal variables this.energyChunksVisibleProperty = energyChunksVisibleProperty; this.mechanicalPoweredSystemIsNextProperty = mechanicalPoweredSystemIsNextProperty; - this.energyChunkMovers = []; + this.energyChunkMovers = new ObservableArray( { + tandem: tandem.createTandem( 'energyChunkMovers' ), + phetioType: ObservableArrayIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); + + // TODO: support state with these? this.energyProducedSinceLastChunkEmitted = EFACConstants.ENERGY_PER_CHUNK * 0.9; this.mechanicalChunksSinceLastThermal = 0; + // @private + this.energyChunkPhetioGroup = energyChunkPhetioGroup; + this.energyChunkPathMoverPhetioGroup = energyChunkPathMoverPhetioGroup; + // monitor target rotation rate for validity if ( assert ) { this.targetCrankAngularVelocityProperty.link( omega => { @@ -126,7 +141,7 @@ // swapped out this.mechanicalPoweredSystemIsNextProperty.link( () => { - const movers = this.energyChunkMovers.slice(); + const movers = this.energyChunkMovers.getArrayCopy(); const hubPosition = this.positionProperty.value.plus( CENTER_OF_BACK_WHEEL_OFFSET ); movers.forEach( mover => { @@ -137,15 +152,15 @@ if ( energyChunk.positionProperty.get().x > hubPosition.x ) { // remove this energy chunk - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); this.energyChunkList.remove( energyChunk ); } else { // make sure that this energy chunk turns into thermal energy - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); - this.energyChunkMovers.push( new EnergyChunkPathMover( + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( energyChunk, createMechanicalToThermalEnergyChunkPath( this.positionProperty.value, energyChunk.positionProperty.get() ), EFACConstants.ENERGY_CHUNK_VELOCITY @@ -227,7 +242,7 @@ // start a new chunk moving const energyChunk = this.findNonMovingEnergyChunk(); if ( energyChunk ) { - this.energyChunkMovers.push( new EnergyChunkPathMover( + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( energyChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, CHEMICAL_ENERGY_CHUNK_OFFSETS ), EFACConstants.ENERGY_CHUNK_VELOCITY ) @@ -257,7 +272,7 @@ moveEnergyChunks( dt ) { // iterate through this copy while the original is mutated - const movers = this.energyChunkMovers.slice(); + const movers = this.energyChunkMovers.getArrayCopy(); movers.forEach( mover => { @@ -274,14 +289,14 @@ // turn this into mechanical energy chunk.energyTypeProperty.set( EnergyType.MECHANICAL ); - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); // add new mover for the mechanical energy chunk if ( this.mechanicalChunksSinceLastThermal >= MECHANICAL_TO_THERMAL_CHUNK_RATIO || !this.mechanicalPoweredSystemIsNextProperty.get() ) { // make this chunk travel to the rear hub, where it will become a chunk of thermal energy - this.energyChunkMovers.push( new EnergyChunkPathMover( chunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( chunk, createMechanicalToThermalEnergyChunkPath( this.positionProperty.value, chunk.positionProperty.get() ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -296,7 +311,7 @@ ]; // send this chunk to the next energy system - this.energyChunkMovers.push( new EnergyChunkPathMover( chunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( chunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.get(), mechanicalEnergyChunkOffsets ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -309,9 +324,9 @@ chunk.positionProperty.get().distance( this.positionProperty.value.plus( CENTER_OF_BACK_WHEEL_OFFSET ) ) < 1E-6 ) { // this is a mechanical energy chunk that has traveled to the hub and should now become thermal energy - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); chunk.energyTypeProperty.set( EnergyType.THERMAL ); - this.energyChunkMovers.push( new EnergyChunkPathMover( chunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( chunk, EnergyChunkPathMover.createRadiatedPath( this.positionProperty.value.plus( CENTER_OF_BACK_WHEEL_OFFSET ), Math.PI * -0.1 ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -321,7 +336,7 @@ else if ( chunk.energyTypeProperty.get() === EnergyType.THERMAL ) { // this is a radiating thermal energy chunk that has reached the end of its route - delete it - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); this.energyChunkList.remove( chunk ); } @@ -330,7 +345,7 @@ // must be mechanical energy that is being passed to the next energy system element this.outgoingEnergyChunks.push( chunk ); - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); } } ); } @@ -379,7 +394,7 @@ // we know the biker is not out of energy, so get one of the remaining chunks const energyChunk = this.findNonMovingEnergyChunk(); - this.energyChunkMovers.push( new EnergyChunkPathMover( + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( energyChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, CHEMICAL_ENERGY_CHUNK_OFFSETS ), EFACConstants.ENERGY_CHUNK_VELOCITY ) @@ -448,7 +463,7 @@ */ clearEnergyChunks() { super.clearEnergyChunks(); - this.energyChunkMovers.length = 0; + this.energyChunkMovers.clear(); } /** @@ -473,7 +488,7 @@ const displacement = new Vector2( ( phet.joist.random.nextDouble() - 0.5 ) * 0.02, 0 ).rotated( Math.PI * 0.7 ); const position = this.positionProperty.value.plus( nominalInitialOffset ).plus( displacement ); - const newEnergyChunk = new EnergyChunk( + const newEnergyChunk = this.energyChunkPhetioGroup.createNextElement( EnergyType.CHEMICAL, position, Vector2.ZERO, Index: js/systems/model/SystemsModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/SystemsModel.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/SystemsModel.js (date 1599517775702) @@ -11,23 +11,25 @@ import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; import Emitter from '../../../../axon/js/Emitter.js'; +import Property from '../../../../axon/js/Property.js'; +import PropertyIO from '../../../../axon/js/PropertyIO.js'; import Vector2 from '../../../../dot/js/Vector2.js'; import Enumeration from '../../../../phet-core/js/Enumeration.js'; +import merge from '../../../../phet-core/js/merge.js'; +import PhetioGroup from '../../../../tandem/js/PhetioGroup.js'; +import PhetioGroupIO from '../../../../tandem/js/PhetioGroupIO.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import BooleanIO from '../../../../tandem/js/types/BooleanIO.js'; import EFACConstants from '../../common/EFACConstants.js'; +import EnergyChunk from '../../common/model/EnergyChunk.js'; +import EnergyType from '../../common/model/EnergyType.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import BeakerHeater from './BeakerHeater.js'; import Belt from './Belt.js'; import Biker from './Biker.js'; +import EnergyChunkPathMover from './EnergyChunkPathMover.js'; import EnergySystemElementCarousel from './EnergySystemElementCarousel.js'; -import Fan from './Fan.js'; -import FaucetAndWater from './FaucetAndWater.js'; -import FluorescentBulb from './FluorescentBulb.js'; import Generator from './Generator.js'; -import IncandescentBulb from './IncandescentBulb.js'; -import SolarPanel from './SolarPanel.js'; -import SunEnergySource from './SunEnergySource.js'; -import TeaKettle from './TeaKettle.js'; // constants const OFFSET_BETWEEN_ELEMENTS_ON_CAROUSEL = new Vector2( 0, -0.4 ); // in meters @@ -58,31 +60,47 @@ phetioDocumentation: 'whether the screen is playing or paused' } ); + this.energyChunkPhetioGroup = new EnergyChunkPhetioGroup( { + tandem: tandem.createTandem( 'energyChunkPhetioGroup' ), + phetioType: PhetioGroupIO( EnergyChunk.EnergyChunkIO ) + } ); + + this.energyChunkPathMoverPhetioGroup = new EnergyChunkPathMoverPhetioGroup( this.energyChunkPhetioGroup, { + tandem: tandem.createTandem( 'energyChunkPathMoverPhetioGroup' ), + phetioType: PhetioGroupIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); + // @public (read-only) energy converters - this.generator = new Generator( this.energyChunksVisibleProperty, energyConvertersTandem.createTandem( 'generator' ) ); - this.solarPanel = new SolarPanel( this.energyChunksVisibleProperty, energyConvertersTandem.createTandem( 'solarPanel' ) ); + this.generator = new Generator( + this.energyChunksVisibleProperty, + this.energyChunkPhetioGroup, + this.energyChunkPathMoverPhetioGroup, + energyConvertersTandem.createTandem( 'generator' ) ); + // this.solarPanel = new SolarPanel( this.energyChunksVisibleProperty, energyConvertersTandem.createTandem( 'solarPanel' ) ); // @public (read-only) energy sources this.biker = new Biker( this.energyChunksVisibleProperty, this.generator.activeProperty, + this.energyChunkPhetioGroup, + this.energyChunkPathMoverPhetioGroup, energySourcesTandem.createTandem( 'biker' ) ); - this.faucetAndWater = new FaucetAndWater( - this.energyChunksVisibleProperty, - this.generator.activeProperty, - energySourcesTandem.createTandem( 'faucetAndWater' ) - ); - this.sun = new SunEnergySource( - this.solarPanel, - this.energyChunksVisibleProperty, - energySourcesTandem.createTandem( 'sun' ) - ); - this.teaKettle = new TeaKettle( - this.energyChunksVisibleProperty, - this.generator.activeProperty, - energySourcesTandem.createTandem( 'teaKettle' ) - ); + // this.faucetAndWater = new FaucetAndWater( + // this.energyChunksVisibleProperty, + // this.generator.activeProperty, + // energySourcesTandem.createTandem( 'faucetAndWater' ) + // ); + // this.sun = new SunEnergySource( + // this.solarPanel, + // this.energyChunksVisibleProperty, + // energySourcesTandem.createTandem( 'sun' ) + // ); + // this.teaKettle = new TeaKettle( + // this.energyChunksVisibleProperty, + // this.generator.activeProperty, + // energySourcesTandem.createTandem( 'teaKettle' ) + // ); const wheel1Center = ENERGY_SOURCES_CAROUSEL_SELECTED_ELEMENT_POSITION.plus( Biker.CENTER_OF_BACK_WHEEL_OFFSET ); const wheel2Center = ENERGY_CONVERTERS_CAROUSEL_SELECTED_ELEMENT_POSITION.plus( Generator.WHEEL_CENTER_OFFSET ); @@ -90,36 +108,39 @@ // @public (read-only) belt that connects biker to generator, which is not on a carousel this.belt = new Belt( Biker.REAR_WHEEL_RADIUS, wheel1Center, Generator.WHEEL_RADIUS, wheel2Center ); - // @public (read-only) energy users - this.fan = new Fan( this.energyChunksVisibleProperty, energyUsersTandem.createTandem( 'fan' ) ); - this.incandescentBulb = new IncandescentBulb( - this.energyChunksVisibleProperty, - energyUsersTandem.createTandem( 'incandescentBulb' ) - ); - this.fluorescentBulb = new FluorescentBulb( - this.energyChunksVisibleProperty, - energyUsersTandem.createTandem( 'fluorescentBulb' ) - ); - this.beakerHeater = new BeakerHeater( this.energyChunksVisibleProperty, energyUsersTandem.createTandem( 'beakerHeater' ) ); + // // @public (read-only) energy users + // this.fan = new Fan( this.energyChunksVisibleProperty, energyUsersTandem.createTandem( 'fan' ) ); + // this.incandescentBulb = new IncandescentBulb( + // this.energyChunksVisibleProperty, + // energyUsersTandem.createTandem( 'incandescentBulb' ) + // ); + // this.fluorescentBulb = new FluorescentBulb( + // this.energyChunksVisibleProperty, + // energyUsersTandem.createTandem( 'fluorescentBulb' ) + // ); + this.beakerHeater = new BeakerHeater( this.energyChunksVisibleProperty, + this.energyChunkPhetioGroup, + this.energyChunkPathMoverPhetioGroup, + energyUsersTandem.createTandem( 'beakerHeater' ) ); // @public (read-only) carousels that control the positions of the energy sources, converters, and users this.energySourcesCarousel = new EnergySystemElementCarousel( - [ this.biker, this.faucetAndWater, this.sun, this.teaKettle ], - Enumeration.byKeys( [ 'BIKER', 'FAUCET', 'SUN', 'TEA_KETTLE' ] ), + [ this.biker ], + Enumeration.byKeys( [ 'BIKER' ] ), ENERGY_SOURCES_CAROUSEL_SELECTED_ELEMENT_POSITION, OFFSET_BETWEEN_ELEMENTS_ON_CAROUSEL, tandem.createTandem( 'energySourcesCarousel' ) ); this.energyConvertersCarousel = new EnergySystemElementCarousel( - [ this.generator, this.solarPanel ], - Enumeration.byKeys( [ 'GENERATOR', 'SOLAR_PANEL' ] ), + [ this.generator ], + Enumeration.byKeys( [ 'GENERATOR' ] ), ENERGY_CONVERTERS_CAROUSEL_SELECTED_ELEMENT_POSITION, OFFSET_BETWEEN_ELEMENTS_ON_CAROUSEL, tandem.createTandem( 'energyConvertersCarousel' ) ); this.energyUsersCarousel = new EnergySystemElementCarousel( - [ this.beakerHeater, this.incandescentBulb, this.fluorescentBulb, this.fan ], - Enumeration.byKeys( [ 'BEAKER_HEATER', 'INCANDESCENT_BULB', 'FLUORESCENT_BULB', 'FAN' ] ), + [ this.beakerHeater ], + Enumeration.byKeys( [ 'BEAKER_HEATER' ] ), new Vector2( 0.09, 0 ), OFFSET_BETWEEN_ELEMENTS_ON_CAROUSEL, tandem.createTandem( 'energyUsersCarousel' ) @@ -241,5 +262,43 @@ } } + +class EnergyChunkPhetioGroup extends PhetioGroup { + + constructor( options ) { + + // TODO: making your own visibleProperty default? + const defaultPositionProperty = new Property( true, { + tandem: options.tandem.createTandem( 'positionProperty' ), + phetioType: PropertyIO( BooleanIO ) + } ); + super( EnergyChunkPhetioGroup.createEnergyChunk, + [ EnergyType.THERMAL, Vector2.ZERO, Vector2.ZERO, defaultPositionProperty, {} ], options ); + } + + // @public + static createEnergyChunk( tandem, energyType, position, velocity, visibleProperty, options ) { + return new EnergyChunk( energyType, position, velocity, visibleProperty, merge( {}, options, { tandem: tandem } ) ); + } +} + +class EnergyChunkPathMoverPhetioGroup extends PhetioGroup { + + /** + * + * @param energyChunkPhetioGroup + * @param options + */ + constructor( energyChunkPhetioGroup, options ) { + super( EnergyChunkPathMoverPhetioGroup.createEnergyChunkPathMover, + [ energyChunkPhetioGroup.archetype, [ Vector2.ZERO ], 1, {} ], options ); + } + + // @public + static createEnergyChunkPathMover( tandem, energyChunk, path, speed, options ) { + return new EnergyChunkPathMover( energyChunk, path, speed, merge( {}, options, { tandem: tandem } ) ); + } +} + energyFormsAndChanges.register( 'SystemsModel', SystemsModel ); export default SystemsModel; \ No newline at end of file Index: js/systems/model/EnergyUser.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/EnergyUser.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/EnergyUser.js (date 1599515796394) @@ -8,6 +8,9 @@ * @author Andrew Adare */ +import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; +import EnergyChunk from '../../common/model/EnergyChunk.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import EnergySystemElement from './EnergySystemElement.js'; @@ -21,17 +24,26 @@ super( iconImage, tandem ); // @private {EnergyChunk[]} - this.incomingEnergyChunks = []; + this.incomingEnergyChunks = new ObservableArray( { + tandem: tandem.createTandem( 'incomingEnergyChunks' ), + phetioType: ObservableArrayIO( EnergyChunk.EnergyChunkIO ) + } ); } /** * Inject a list of energy chunks into this energy system element. Once injected, it is the system's responsibility * to move, convert, and otherwise manage them. - * @param {Array{EnergyChunk}} energyChunks - list of energy chunks to inject + * @param {Array.} energyChunks - list of energy chunks to inject * @public */ injectEnergyChunks( energyChunks ) { - this.incomingEnergyChunks = _.union( this.incomingEnergyChunks, energyChunks ); + + // TODO: I'm worried about this n^2 algorithm + this.incomingEnergyChunks.forEach( energyChunk => { + if ( !energyChunks.includes( energyChunk ) ) { + this.incomingEnergyChunks.remove( energyChunk ); + } + } ); } /** @@ -40,7 +52,7 @@ */ clearEnergyChunks() { super.clearEnergyChunks(); - this.incomingEnergyChunks.length = 0; + this.incomingEnergyChunks.clear(); } } Index: js/common/view/EnergyChunkNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/common/view/EnergyChunkNode.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/common/view/EnergyChunkNode.js (date 1599511052009) @@ -9,19 +9,21 @@ */ import Vector2 from '../../../../dot/js/Vector2.js'; +import merge from '../../../../phet-core/js/merge.js'; import PhetFont from '../../../../scenery-phet/js/PhetFont.js'; import Circle from '../../../../scenery/js/nodes/Circle.js'; import Image from '../../../../scenery/js/nodes/Image.js'; import Node from '../../../../scenery/js/nodes/Node.js'; import Text from '../../../../scenery/js/nodes/Text.js'; +import Tandem from '../../../../tandem/js/Tandem.js'; import chemicalEnergyImage from '../../../images/energy_chemical_png.js'; import electricalEnergyImage from '../../../images/energy_electrical_png.js'; import hiddenEnergyImage from '../../../images/energy_hidden_png.js'; import lightEnergyImage from '../../../images/energy_light_png.js'; import mechanicalEnergyImage from '../../../images/energy_mechanical_png.js'; import thermalEnergyImage from '../../../images/energy_thermal_png.js'; -import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; +import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import EFACConstants from '../EFACConstants.js'; import EFACQueryParameters from '../EFACQueryParameters.js'; import EnergyType from '../model/EnergyType.js'; @@ -49,8 +51,13 @@ * @param {EnergyChunk} energyChunk - model of an energy chunk * @param {ModelViewTransform2} modelViewTransform */ - constructor( energyChunk, modelViewTransform ) { - super(); + constructor( energyChunk, modelViewTransform, options ) { + + options = merge( { + tandem: Tandem.OPTIONAL, // TODO: make this REQUIRED? + phetioDynamicElement: true + }, options ); + super( options ); // control the overall visibility of this node const handleVisibilityChanged = visible => { Index: js/systems/model/PositionableFadableModelElement.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/PositionableFadableModelElement.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/PositionableFadableModelElement.js (date 1599513658201) @@ -24,6 +24,7 @@ constructor( initialPosition, initialOpacity, tandem ) { super( initialPosition, tandem ); + // TODO: should these be instrumented? // @public {NumberProperty} this.opacityProperty = new NumberProperty( initialOpacity, { range: new Range( 0, 1 ) Index: js/systems/model/EnergySource.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/EnergySource.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/EnergySource.js (date 1599514853593) @@ -9,6 +9,9 @@ * @author Jesse Greenberg */ +import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; +import EnergyChunk from '../../common/model/EnergyChunk.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import EnergySystemElement from './EnergySystemElement.js'; @@ -20,7 +23,10 @@ */ constructor( iconImage, tandem ) { super( iconImage, tandem ); - this.outgoingEnergyChunks = []; + this.outgoingEnergyChunks = new ObservableArray( { + tandem: tandem.createTandem( 'outgoingEnergyChunks' ), + phetioType: ObservableArrayIO( EnergyChunk.EnergyChunkIO ) + } ); } /** @@ -32,10 +38,11 @@ extractOutgoingEnergyChunks() { // remove all outgoing chunks from this.energyChunkList - this.energyChunkList.removeAll( this.outgoingEnergyChunks ); + this.energyChunkList.removeAll( this.outgoingEnergyChunks.getArray() ); - // return a copy of the outgoing chunk list and clear it in one fell swoop - return this.outgoingEnergyChunks.splice( 0 ); + const outgoingEnergyChunksCopy = this.outgoingEnergyChunks.getArrayCopy(); + this.outgoingEnergyChunks.clear(); + return outgoingEnergyChunksCopy; } /** @@ -45,7 +52,7 @@ */ clearEnergyChunks() { super.clearEnergyChunks(); - this.outgoingEnergyChunks.length = 0; + this.outgoingEnergyChunks.clear(); } } Index: js/systems/view/SystemsScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/view/SystemsScreenView.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/view/SystemsScreenView.js (date 1599512271365) @@ -133,32 +133,32 @@ modelViewTransform, energyUsersTandem.createTandem( 'beakerHeaterNode' ) ); - const incandescentBulbNode = new LightBulbNode( - model.incandescentBulb, - model.energyChunksVisibleProperty, - modelViewTransform, { - bulbType: 'incandescent', - tandem: energyUsersTandem.createTandem( 'incandescentBulbNode' ) - } - ); - const fluorescentBulbNode = new LightBulbNode( - model.fluorescentBulb, - model.energyChunksVisibleProperty, - modelViewTransform, { - bulbType: 'fluorescent', - tandem: energyUsersTandem.createTandem( 'fluorescentBulbNode' ) - } - ); - const fanNode = new FanNode( - model.fan, - model.energyChunksVisibleProperty, - modelViewTransform, - energyUsersTandem.createTandem( 'fanNode' ) - ); + // const incandescentBulbNode = new LightBulbNode( + // model.incandescentBulb, + // model.energyChunksVisibleProperty, + // modelViewTransform, { + // bulbType: 'incandescent', + // tandem: energyUsersTandem.createTandem( 'incandescentBulbNode' ) + // } + // ); + // const fluorescentBulbNode = new LightBulbNode( + // model.fluorescentBulb, + // model.energyChunksVisibleProperty, + // modelViewTransform, { + // bulbType: 'fluorescent', + // tandem: energyUsersTandem.createTandem( 'fluorescentBulbNode' ) + // } + // ); + // const fanNode = new FanNode( + // model.fan, + // model.energyChunksVisibleProperty, + // modelViewTransform, + // energyUsersTandem.createTandem( 'fanNode' ) + // ); this.addChild( this.beakerHeaterNode ); - this.addChild( incandescentBulbNode ); - this.addChild( fluorescentBulbNode ); - this.addChild( fanNode ); + // this.addChild( incandescentBulbNode ); + // this.addChild( fluorescentBulbNode ); + // this.addChild( fanNode ); // create the energy converter nodes const generatorNode = new GeneratorNode( @@ -174,53 +174,53 @@ phetioReadOnly: true } ); - const solarPanelNode = new SolarPanelNode( - model.solarPanel, - modelViewTransform, - energyConvertersTandem.createTandem( 'solarPanelNode' ) - ); + // const solarPanelNode = new SolarPanelNode( + // model.solarPanel, + // modelViewTransform, + // energyConvertersTandem.createTandem( 'solarPanelNode' ) + // ); this.addChild( generatorNode ); this.addChild( beltNode ); - this.addChild( solarPanelNode ); + // this.addChild( solarPanelNode ); - // @private - this.faucetAndWaterNode = new FaucetAndWaterNode( - model.faucetAndWater, - model.energyChunksVisibleProperty, - modelViewTransform, - energySourcesTandem.createTandem( 'faucetAndWaterNode' ) - ); - this.addChild( this.faucetAndWaterNode ); + // // @private + // this.faucetAndWaterNode = new FaucetAndWaterNode( + // model.faucetAndWater, + // model.energyChunksVisibleProperty, + // modelViewTransform, + // energySourcesTandem.createTandem( 'faucetAndWaterNode' ) + // ); + // this.addChild( this.faucetAndWaterNode ); // get the mechanical energy chunk layer from the generator and add it after the faucet has been created. this is // desirable because the water from the faucet appears on top of the generator wheel, but the energy chunks that // are traveling on top of the falling water now remain in front of the water once the generator owns them. this.addChild( generatorNode.getMechanicalEnergyChunkLayer() ); - - // create the rest of the energy source nodes - const sunNode = new SunNode( - model.sun, - model.energyChunksVisibleProperty, - modelViewTransform, - energySourcesTandem.createTandem( 'sunNode' ) - ); + // + // // create the rest of the energy source nodes + // const sunNode = new SunNode( + // model.sun, + // model.energyChunksVisibleProperty, + // modelViewTransform, + // energySourcesTandem.createTandem( 'sunNode' ) + // ); // @private - this.teaKettleNode = new TeaKettleNode( - model.teaKettle, - model.energyChunksVisibleProperty, - modelViewTransform, - energySourcesTandem.createTandem( 'teaKettleNode' ) - ); + // this.teaKettleNode = new TeaKettleNode( + // model.teaKettle, + // model.energyChunksVisibleProperty, + // modelViewTransform, + // energySourcesTandem.createTandem( 'teaKettleNode' ) + // ); const bikerNode = new BikerNode( model.biker, model.energyChunksVisibleProperty, modelViewTransform, energySourcesTandem.createTandem( 'bikerNode' ) ); - this.addChild( sunNode ); + // this.addChild( sunNode ); this.addChild( bikerNode ); - this.addChild( this.teaKettleNode ); + // this.addChild( this.teaKettleNode ); // use this Tandem for the checkbox, too, so it appears as a child of the panel const controlPanelTandem = tandem.createTandem( 'controlPanel' ); @@ -375,9 +375,9 @@ * @public */ stepView( dt ) { - this.teaKettleNode.step( dt ); + // this.teaKettleNode.step( dt ); this.beakerHeaterNode.step( dt ); - this.faucetAndWaterNode.step( dt ); + // this.faucetAndWaterNode.step( dt ); } /** Index: js/systems/model/EnergySystemElement.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/EnergySystemElement.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/EnergySystemElement.js (date 1599510963025) @@ -11,7 +11,9 @@ import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; import Vector2 from '../../../../dot/js/Vector2.js'; +import EnergyChunk from '../../common/model/EnergyChunk.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; import PositionableFadableModelElement from './PositionableFadableModelElement.js'; @@ -29,7 +31,10 @@ this.iconImage = iconImage; // @public (read-only) {ObservableArray.} - this.energyChunkList = new ObservableArray(); + this.energyChunkList = new ObservableArray( { + tandem: tandem.createTandem( 'energyChunkList' ), + phetioType: ObservableArrayIO( EnergyChunk.EnergyChunkIO ) + } ); // @public {BooleanProperty} this.activeProperty = new BooleanProperty( false, { Index: js/systems/model/Generator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/Generator.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/Generator.js (date 1599513902900) @@ -10,6 +10,7 @@ import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; import NumberProperty from '../../../../axon/js/NumberProperty.js'; import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; import Range from '../../../../dot/js/Range.js'; import Utils from '../../../../dot/js/Utils.js'; import Vector2 from '../../../../dot/js/Vector2.js'; @@ -19,8 +20,8 @@ import EnergyChunk from '../../common/model/EnergyChunk.js'; import EnergyType from '../../common/model/EnergyType.js'; import EnergyChunkNode from '../../common/view/EnergyChunkNode.js'; -import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import energyFormsAndChanges from '../../energyFormsAndChanges.js'; +import energyFormsAndChangesStrings from '../../energyFormsAndChangesStrings.js'; import Energy from './Energy.js'; import EnergyChunkPathMover from './EnergyChunkPathMover.js'; import EnergyConverter from './EnergyConverter.js'; @@ -48,9 +49,11 @@ /** * @param {Property.} energyChunksVisibleProperty + * @param {EnergyChunkPhetioGroup} energyChunkPhetioGroup + * @param {EnergyChunkPathMoverPhetioGroup} energyChunkPathMoverPhetioGroup * @param {Tandem} tandem */ - constructor( energyChunksVisibleProperty, tandem ) { + constructor( energyChunksVisibleProperty, energyChunkPhetioGroup, energyChunkPathMoverPhetioGroup, tandem ) { super( new Image( GENERATOR_ICON ), tandem ); @@ -59,6 +62,8 @@ // @private {BooleanProperty} this.energyChunksVisibleProperty = energyChunksVisibleProperty; + this.energyChunkPhetioGroup = energyChunkPhetioGroup; + this.energyChunkPathMoverPhetioGroup = energyChunkPathMoverPhetioGroup; // @public (read-only) {NumberProperty} this.wheelRotationalAngleProperty = new NumberProperty( 0, { @@ -87,15 +92,24 @@ phetioHighFrequency: true, phetioDocumentation: 'the angular velocity of the wheel' } ); - this.energyChunkMovers = []; + this.energyChunkMovers = new ObservableArray( { + tandem: tandem.createTandem( 'energyChunkMovers' ), + phetioType: ObservableArrayIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); // @public (read-only) {ObservableArray. 0 ) { - const incomingChunks = _.clone( this.incomingEnergyChunks ); + const incomingChunks = _.clone( this.incomingEnergyChunks.getArray() ); incomingChunks.forEach( chunk => { // validate energy type @@ -182,10 +196,10 @@ // transfer chunk from incoming list to current list this.energyChunkList.push( chunk ); - _.pull( this.incomingEnergyChunks, chunk ); + this.incomingEnergyChunks.remove( chunk ); // add a "mover" that will move this energy chunk to the center of the wheel - this.energyChunkMovers.push( new EnergyChunkPathMover( chunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( chunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, [ WHEEL_CENTER_OFFSET ] ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -214,7 +228,7 @@ * @private */ updateEnergyChunkPositions( dt ) { - const chunkMovers = _.clone( this.energyChunkMovers ); + const chunkMovers = _.clone( this.energyChunkMovers.getArray() ); // TODO: use getArrayCopy? chunkMovers.forEach( mover => { @@ -242,14 +256,14 @@ // on its way. Also add a "hidden" chunk so that the movement through the generator can be seen by the // user. this.energyChunkList.remove( chunk ); - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); // TODO: is this the same as _.pull? I think so chunk.energyTypeProperty.set( EnergyType.ELECTRICAL ); this.electricalEnergyChunks.push( chunk ); - this.energyChunkMovers.push( new EnergyChunkPathMover( mover.energyChunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( mover.energyChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, electricalEnergyChunkOffsets ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); - const hiddenChunk = new EnergyChunk( + const hiddenChunk = this.energyChunkPhetioGroup.createNextElement( EnergyType.HIDDEN, chunk.positionProperty.get(), Vector2.ZERO, @@ -258,7 +272,7 @@ hiddenChunk.zPositionProperty.set( -EnergyChunkNode.Z_DISTANCE_WHERE_FULLY_FADED / 2 ); this.hiddenEnergyChunks.push( hiddenChunk ); const hiddenEnergyChunkOffsets = [ START_OF_WIRE_CURVE_OFFSET, WIRE_CURVE_POINT_1_OFFSET ]; - this.energyChunkMovers.push( new EnergyChunkPathMover( hiddenChunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( hiddenChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, hiddenEnergyChunkOffsets ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -269,7 +283,7 @@ // This electrical energy chunk has traveled to the end of its path, so transfer it to the next energy // system. - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); this.outgoingEnergyChunks.push( chunk ); break; @@ -278,7 +292,7 @@ // This hidden energy chunk has traveled to the end of its path, so just remove it, because the electrical // energy chunk to which is corresponds should now be visible to the user. this.hiddenEnergyChunks.remove( chunk ); - _.pull( this.energyChunkMovers, mover ); + this.energyChunkMovers.remove( mover ); break; default: @@ -320,7 +334,7 @@ // determine if time to add a new chunk if ( energySinceLastChunk >= EFACConstants.ENERGY_PER_CHUNK && !skipThisChunk ) { - const newChunk = new EnergyChunk( + const newChunk = this.energyChunkPhetioGroup.createNextElement( EnergyType.MECHANICAL, this.positionProperty.value.plus( LEFT_SIDE_OF_WHEEL_OFFSET ), Vector2.ZERO, @@ -330,7 +344,7 @@ this.energyChunkList.push( newChunk ); // add a 'mover' for this energy chunk - this.energyChunkMovers.push( new EnergyChunkPathMover( newChunk, + this.energyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( newChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.value, [ WHEEL_CENTER_OFFSET ] ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -384,7 +398,7 @@ super.clearEnergyChunks(); this.electricalEnergyChunks.clear(); this.hiddenEnergyChunks.clear(); - this.energyChunkMovers.length = 0; + this.energyChunkMovers.clear(); } /** @@ -392,11 +406,11 @@ * @override */ extractOutgoingEnergyChunks() { - const chunks = _.clone( this.outgoingEnergyChunks ); + const chunks = _.clone( this.outgoingEnergyChunks.getArray() ); // Remove outgoing chunks from electrical energy chunks list this.electricalEnergyChunks.removeAll( chunks ); - this.outgoingEnergyChunks.length = 0; + this.outgoingEnergyChunks.clear(); return chunks; } Index: js/systems/model/BeakerHeater.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/systems/model/BeakerHeater.js (revision 9ba49be04fd995262a129eb491b7513b368ae294) +++ js/systems/model/BeakerHeater.js (date 1599516203916) @@ -8,13 +8,13 @@ import NumberProperty from '../../../../axon/js/NumberProperty.js'; import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import ObservableArrayIO from '../../../../axon/js/ObservableArrayIO.js'; import Range from '../../../../dot/js/Range.js'; import Vector2 from '../../../../dot/js/Vector2.js'; import Image from '../../../../scenery/js/nodes/Image.js'; import WATER_ICON from '../../../images/water_icon_png.js'; import EFACConstants from '../../common/EFACConstants.js'; import Beaker from '../../common/model/Beaker.js'; -import EnergyChunk from '../../common/model/EnergyChunk.js'; import EnergyContainerCategory from '../../common/model/EnergyContainerCategory.js'; import EnergyType from '../../common/model/EnergyType.js'; import HeatTransferConstants from '../../common/model/HeatTransferConstants.js'; @@ -56,9 +56,13 @@ /** * @param {BooleanProperty} energyChunksVisibleProperty + * @param {EnergyChunkPhetioGroup} energyChunkPhetioGroup + * @param {EnergyChunkPathMoverPhetioGroup} energyChunkPathMoverPhetioGroup * @param {Tandem} tandem */ - constructor( energyChunksVisibleProperty, tandem ) { + constructor( energyChunksVisibleProperty, energyChunkPhetioGroup, + energyChunkPathMoverPhetioGroup, + tandem ) { super( new Image( WATER_ICON ), tandem ); // @public {string} - a11y name @@ -66,6 +70,8 @@ // @private this.energyChunksVisibleProperty = energyChunksVisibleProperty; + this.energyChunkPhetioGroup = energyChunkPhetioGroup; + this.energyChunkPathMoverPhetioGroup = energyChunkPathMoverPhetioGroup; // @public (read-only) {NumberProperty} this.heatProportionProperty = new NumberProperty( 0, { @@ -76,11 +82,20 @@ phetioDocumentation: 'proportion of how much heat the coils have' } ); - // @private {EnergyChunkPathMover[]} - arrays that move the energy chunks as they move into, within, and out of the + // @private {ObservableArray.} - arrays that move the energy chunks as they move into, within, and out of the // beaker - this.electricalEnergyChunkMovers = []; - this.heatingElementEnergyChunkMovers = []; - this.radiatedEnergyChunkMovers = []; + this.electricalEnergyChunkMovers = new ObservableArray( { + tandem: tandem.createTandem( 'electricalEnergyChunkMovers' ), + phetioType: ObservableArrayIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); + this.heatingElementEnergyChunkMovers = new ObservableArray( { + tandem: tandem.createTandem( 'heatingElementEnergyChunkMovers' ), + phetioType: ObservableArrayIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); + this.radiatedEnergyChunkMovers = new ObservableArray( { + tandem: tandem.createTandem( 'radiatedEnergyChunkMovers' ), + phetioType: ObservableArrayIO( EnergyChunkPathMover.EnergyChunkPathMoverIO ) + } ); // @public (read-only) {ObservableArray} - energy chunks that are radiated by this beaker this.radiatedEnergyChunkList = new ObservableArray(); @@ -146,13 +161,13 @@ this.energyChunkList.push( chunk ); // add a "mover" that will move this energy chunk through the wire to the heating element - this.electricalEnergyChunkMovers.push( new EnergyChunkPathMover( chunk, + this.electricalEnergyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( chunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.get(), ELECTRICAL_ENERGY_CHUNK_OFFSETS ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); } ); // clear incoming chunks array - this.incomingEnergyChunks.length = 0; + this.incomingEnergyChunks.clear(); } this.moveElectricalEnergyChunks( dt ); this.moveThermalEnergyChunks( dt ); @@ -221,7 +236,7 @@ ec.zPositionProperty.set( 0 ); // move to front of z order this.radiatedEnergyChunkList.push( ec ); this.radiatedEnergyChunkMovers.push( - new EnergyChunkPathMover( + this.energyChunkPathMoverPhetioGroup.createNextElement( ec, EnergyChunkPathMover.createRadiatedPath( ec.positionProperty.value, 0 ), EFACConstants.ENERGY_CHUNK_VELOCITY @@ -271,7 +286,7 @@ * @private */ moveRadiatedEnergyChunks( dt ) { - const movers = this.radiatedEnergyChunkMovers.slice(); + const movers = this.radiatedEnergyChunkMovers.getArrayCopy(); movers.forEach( mover => { mover.moveAlongPath( dt ); @@ -280,7 +295,7 @@ // remove this energy chunk entirely this.radiatedEnergyChunkList.remove( mover.energyChunk ); - _.pull( this.radiatedEnergyChunkMovers, mover ); + this.radiatedEnergyChunkMovers.remove( mover ); } } ); } @@ -290,7 +305,7 @@ * @private */ moveThermalEnergyChunks( dt ) { - const movers = _.clone( this.heatingElementEnergyChunkMovers ); + const movers = this.heatingElementEnergyChunkMovers.getArrayCopy(); movers.forEach( mover => { mover.moveAlongPath( dt ); @@ -301,7 +316,7 @@ // the chunk. this.beaker.addEnergyChunk( mover.energyChunk ); this.energyChunkList.remove( mover.energyChunk ); - _.pull( this.heatingElementEnergyChunkMovers, mover ); + this.heatingElementEnergyChunkMovers.remove( mover ); } } ); } @@ -311,7 +326,7 @@ * @private */ moveElectricalEnergyChunks( dt ) { - const movers = _.clone( this.electricalEnergyChunkMovers ); + const movers = this.electricalEnergyChunkMovers.getArrayCopy(); movers.forEach( mover => { mover.moveAlongPath( dt ); @@ -319,11 +334,11 @@ if ( mover.pathFullyTraversed ) { // the electrical energy chunk has reached the burner, so it needs to change into thermal energy - _.pull( this.electricalEnergyChunkMovers, mover ); + this.electricalEnergyChunkMovers.remove( mover ); mover.energyChunk.energyTypeProperty.set( EnergyType.THERMAL ); // have the thermal energy move a little on the element before moving into the beaker - this.heatingElementEnergyChunkMovers.push( new EnergyChunkPathMover( mover.energyChunk, + this.heatingElementEnergyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( mover.energyChunk, this.createHeaterElementEnergyChunkPath( mover.energyChunk.positionProperty.get() ), HEATING_ELEMENT_ENERGY_CHUNK_VELOCITY ) ); } @@ -355,7 +370,7 @@ if ( energySinceLastChunk >= EFACConstants.ENERGY_PER_CHUNK ) { // create and add a new chunk - const newEnergyChunk = new EnergyChunk( + const newEnergyChunk = this.energyChunkPhetioGroup.createNextElement( EnergyType.ELECTRICAL, this.positionProperty.get().plus( LEFT_SIDE_OF_WIRE_OFFSET ), Vector2.ZERO, @@ -364,7 +379,7 @@ this.energyChunkList.push( newEnergyChunk ); // add a "mover" that will move this energy chunk through the wire to the heating element - this.electricalEnergyChunkMovers.push( new EnergyChunkPathMover( newEnergyChunk, + this.electricalEnergyChunkMovers.push( this.energyChunkPathMoverPhetioGroup.createNextElement( newEnergyChunk, EnergyChunkPathMover.createPathFromOffsets( this.positionProperty.get(), ELECTRICAL_ENERGY_CHUNK_OFFSETS ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); @@ -401,9 +416,9 @@ */ clearEnergyChunks() { super.clearEnergyChunks(); - this.electricalEnergyChunkMovers.length = 0; - this.heatingElementEnergyChunkMovers.length = 0; - this.radiatedEnergyChunkMovers.length = 0; + this.electricalEnergyChunkMovers.clear(); + this.heatingElementEnergyChunkMovers.clear(); + this.radiatedEnergyChunkMovers.clear(); this.radiatedEnergyChunkList.clear(); } ```

[zepumph]

From this investigation, I think that it is likely to take between 8-16 more hours to get an initial feature working for all sim elements. I hope to touch base with the greater team once I have my mini system working fully with state (2-4 more hours?) Tagging @kathy-phet so she is aware of my current time estimates.

[zepumph]

The above fixes an infinite loop because of using string operations. The following case should return true for PhetioDynamicElementContainer.stateSetOnAllChildrenOfDynamicElement, but never returned:

potential parent: energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverPhetioGroup.energyChunkPathMoverPhetio_1 potential child: energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverPhetioGroup.energyChunkPathMoverPhetio_134

(oops). Tagging @samreid to note the bug fix

[zepumph]

Thanks to some consulting with @jbphet, I was able to get an initial pass implemented. Work done so far:

• EnergyChunk instrumented as a dynamic element
• EnergyChunkPathMover instrumented as a dynamic element
• A PhetioGroup for each in SystemsModel that is passed to any system element that wants to make either (so a single collection for each).
• Commented out all system elements except Biker/Generator/BeakerHeater.
• In those three types and super types: Many javascript arrays instrumented as ObservableArrayIO( ReferenceIO( EnergyChunkIO ) ) or the same for EnergyChunkPathMoverIO

Now state is successfully being set through the entire system until the very end where the beaker emits a chunk as heat energy. This is because I haven't yet instrumented EnergyChunkContainerSlice

Still to do in order:

• Instrument EnergyChunkContainerSlice
• Add support for other elements
• Regression test the first screen.
• Clean things up.

I'm working on a branch right now since I wanted to commit, but I didn't want to tip toe around the fact that I commented our most of the components on the second screen at this time. I will keep it up to date with master to make an easier transition later on.

[zepumph]

Things are looking really good. In https://github.com/phetsims/energy-forms-and-changes/commit/33160bed9f2a3d260bb1152ace21dce1d1c3e5d1 I got the state wrapper working for the original three system elements.

Then I got the first screen working

Now I am on to adding support to individual system elements. I think it will go pretty fast from here, like in https://github.com/phetsims/energy-forms-and-changes/commit/4899a57ac936f2f74ef76ec0a0abc2d573a20dc5. There are ~5 more or so.

[zepumph]

I have spent 8.5 Hours on this so far.

[zepumph]

All system elements have now been added back for the second screen:

The dynamic element implementation has leaked into the first screen, and I can't think of a way to isolate this feature just to the second screen because Beaker (meaning RectangularThermalMovableModelElement) needed to be outfitted with PhetioGroup support, so the first screen is still a bit broken. Right now the set state onto a block looks like:

Sad little chunks getting left behind. . . I hope it won't be too hard to sort it out in a way that is consistent, simple, and idiomatic with the behavior of energy chunks elsewhere.

[zepumph]

I created https://github.com/phetsims/energy-forms-and-changes/issues/353 to investigate some first-screen trouble mentioned above.

Up next:

• There are some memory concerns in the state wrapper, which I want to investigate
• Sometimes when fuzzing I get Error: Assertion failed: energyFormsAndChanges.systemsScreen.model.energyChunkGroup.countProperty should match array length.. I will need to take a look to see if it has to do with mutating that group while state is being set from Property listeners.

[zepumph]

Second bullet solved by https://github.com/phetsims/energy-forms-and-changes/commit/37104230caf31517d1231fba15172919d3f84594.

[zepumph]

RE performance, there seems to be many more instances of EnergyChunk (8250) and EnergyChunkContainerSlice (6084) in a random snapshot I made in the state wrapper. Then a couple of seconds later, we were up to EnergyChunk (18596) and EnergyChunkContainerSlice (2500).

What fun! Nothing like an obvious memory leak for a Thursday afternoon!!!

[zepumph]

The above fix in EnergyChunkContainerSlice fixed the massive memory leak. Now after a bit of running we still stay within the 50-60MB range:

That said, I think that still feel that there may be a smaller memory leak. I will report back after 10 minutes of fuzzing.

[zepumph]

There is definitely still a memory leak, just not as aggressive:

[zepumph]

I think that I am on to something with this patch. I would have committed, but after this patch, toggling the energy chunks visible caused this assertion:

Assertion failed: energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.countProperty should match array length.

This only seems to happen in cases where energy chunks get preloaded.

[zepumph]

```diff Index: energy-forms-and-changes/js/systems/model/Fan.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/Fan.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/Fan.js (date 1599785879068) @@ -242,6 +242,8 @@ this.mechanicalEnergyChunkMovers.push( this.energyChunkPathMoverGroup.createNextElement( mover.energyChunk, createBlownEnergyChunkPath( mover.energyChunk.positionProperty.get() ), EFACConstants.ENERGY_CHUNK_VELOCITY ) ); + + mover.dispose(); } else { mover.energyChunk.energyTypeProperty.set( EnergyType.THERMAL ); @@ -276,6 +278,9 @@ if ( mover.pathFullyTraversed ) { this.energyChunkList.remove( mover.energyChunk ); this.radiatedEnergyChunkMovers.remove( mover ); + + mover.energyChunk.dispose(); + mover.dispose(); } } ); } @@ -296,6 +301,9 @@ if ( mover.pathFullyTraversed ) { this.energyChunkList.remove( mover.energyChunk ); this.mechanicalEnergyChunkMovers.remove( mover ); + + mover.energyChunk.dispose(); + mover.dispose(); } } ); } Index: energy-forms-and-changes/js/systems/model/LightBulb.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/LightBulb.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/LightBulb.js (date 1599786081602) @@ -191,6 +191,8 @@ if ( mover.pathFullyTraversed ) { this.energyChunkList.remove( mover.energyChunk ); this.radiatedEnergyChunkMovers.remove( mover ); + mover.energyChunk.dispose(); + mover.dispose(); } } ); } @@ -211,6 +213,9 @@ if ( mover.pathFullyTraversed ) { this.filamentEnergyChunkMovers.remove( mover ); this.radiateEnergyChunk( mover.energyChunk ); + mover.energyChunk.dispose(); + mover.dispose(); + } } ); } @@ -229,6 +234,7 @@ if ( mover.pathFullyTraversed ) { this.electricalEnergyChunkMovers.remove( mover ); + mover.dispose(); // turn this energy chunk into thermal energy on the filament if ( this.hasFilament ) { Index: energy-forms-and-changes/js/systems/model/EnergyChunkPathMover.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/EnergyChunkPathMover.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/EnergyChunkPathMover.js (date 1599784851543) @@ -57,7 +57,7 @@ this.nextPoint = path[ 0 ]; } - // @public + // @private toStateObject() { return { path: ArrayIO( Vector2IO ).toStateObject( this.path ), @@ -69,21 +69,19 @@ }; } - // @public + // @private static stateToArgsForConstructor( stateObject ) { const energyChunk = ReferenceIO( EnergyChunk.EnergyChunkIO ).fromStateObject( stateObject.energyChunkPhetioID ); const path = ArrayIO( Vector2IO ).fromStateObject( stateObject.path ); return [ energyChunk, path, stateObject.speed ]; } - - // @public + // @private applyState( stateObject ) { this.pathFullyTraversed = stateObject.pathFullyTraversed; this.nextPoint = stateObject.nextPoint; } - /** * advance chunk position along the path * @param {number} dt - time step in seconds Index: energy-forms-and-changes/js/systems/model/Biker.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/Biker.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/Biker.js (date 1599787390727) @@ -141,6 +141,11 @@ // swapped out this.mechanicalPoweredSystemIsNextProperty.link( () => { + // TODO: does this work? #350 + if ( phet.joist.sim.isSettingPhetioStateProperty.value ) { + return; + } + const movers = this.energyChunkMovers.getArrayCopy(); const hubPosition = this.positionProperty.value.plus( CENTER_OF_BACK_WHEEL_OFFSET ); @@ -154,11 +159,13 @@ // remove this energy chunk this.energyChunkMovers.remove( mover ); this.energyChunkList.remove( energyChunk ); + mover.dispose(); } else { // make sure that this energy chunk turns into thermal energy this.energyChunkMovers.remove( mover ); + mover.dispose(); this.energyChunkMovers.push( this.energyChunkPathMoverGroup.createNextElement( energyChunk, @@ -290,6 +297,7 @@ // turn this into mechanical energy chunk.energyTypeProperty.set( EnergyType.MECHANICAL ); this.energyChunkMovers.remove( mover ); + mover.dispose(); // add new mover for the mechanical energy chunk if ( this.mechanicalChunksSinceLastThermal >= MECHANICAL_TO_THERMAL_CHUNK_RATIO || @@ -325,6 +333,8 @@ // this is a mechanical energy chunk that has traveled to the hub and should now become thermal energy this.energyChunkMovers.remove( mover ); + mover.dispose(); + chunk.energyTypeProperty.set( EnergyType.THERMAL ); this.energyChunkMovers.push( this.energyChunkPathMoverGroup.createNextElement( chunk, EnergyChunkPathMover.createRadiatedPath( this.positionProperty.value.plus( CENTER_OF_BACK_WHEEL_OFFSET ), Math.PI * -0.1 ), @@ -338,6 +348,8 @@ // this is a radiating thermal energy chunk that has reached the end of its route - delete it this.energyChunkMovers.remove( mover ); this.energyChunkList.remove( chunk ); + chunk.dispose(); + mover.dispose(); } // MECHANICAL @@ -346,6 +358,7 @@ // must be mechanical energy that is being passed to the next energy system element this.outgoingEnergyChunks.push( chunk ); this.energyChunkMovers.remove( mover ); + mover.dispose(); } } ); } Index: energy-forms-and-changes/js/systems/model/TeaKettle.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/TeaKettle.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/TeaKettle.js (date 1599786184469) @@ -179,6 +179,7 @@ if ( mover.pathFullyTraversed ) { this.energyChunkMovers.remove( mover ); + mover.dispose(); // This is a thermal chunk that is coming out of the water. if ( chunk.energyTypeProperty.get() === EnergyType.THERMAL && @@ -208,6 +209,7 @@ // This chunk is out of view, and we are done with it. else { this.energyChunkList.remove( chunk ); + chunk.dispose(); } } @@ -226,6 +228,7 @@ this.outgoingEnergyChunks.push( chunk ); this.energyChunkMovers.remove( mover ); + mover.dispose(); // Alternate sending or keeping chunks. this.transferNextAvailableChunk = false; Index: energy-forms-and-changes/js/systems/model/SolarPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/SolarPanel.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/SolarPanel.js (date 1599786094404) @@ -234,6 +234,7 @@ this.outgoingEnergyChunks.push( mover.energyChunk ); this.energyChunkList.remove( mover.energyChunk ); } + mover.dispose(); } } ); } @@ -255,6 +256,8 @@ if ( mover.pathFullyTraversed ) { this.lightEnergyChunkMovers.remove( mover ); this.energyChunkList.remove( mover.energyChunk ); + mover.energyChunk.dispose(); + mover.dispose(); } } ); } Index: tandem/js/PhetioDynamicElementContainer.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- tandem/js/PhetioDynamicElementContainer.js (revision 6d056dd3a566bff59947f9531830bfe698a04404) +++ tandem/js/PhetioDynamicElementContainer.js (date 1599786526298) @@ -317,7 +317,7 @@ disposeElement( element, fromStateSetting ) { element.dispose(); - assert && this.supportsDynamicState && _.hasIn( window, 'phet.joist.sim.' ) && + assert && this.supportsDynamicState && _.hasIn( window, 'phet.joist.sim' ) && phet.joist.sim.isSettingPhetioStateProperty.value && assert( fromStateSetting, 'should not dispose a dynamic element while setting phet-io state' ); Index: energy-forms-and-changes/js/systems/model/Generator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/Generator.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/Generator.js (date 1599786042283) @@ -257,7 +257,8 @@ // on its way. Also add a "hidden" chunk so that the movement through the generator can be seen by the // user. this.energyChunkList.remove( chunk ); - this.energyChunkMovers.remove( mover ); // TODO: is this the same as _.pull? I think so https://github.com/phetsims/energy-forms-and-changes/issues/350 + this.energyChunkMovers.remove( mover ); + chunk.energyTypeProperty.set( EnergyType.ELECTRICAL ); this.electricalEnergyChunks.push( chunk ); this.energyChunkMovers.push( this.energyChunkPathMoverGroup.createNextElement( mover.energyChunk, @@ -278,6 +279,8 @@ EFACConstants.ENERGY_CHUNK_VELOCITY ) ); + mover.dispose(); + break; } case EnergyType.ELECTRICAL: @@ -286,6 +289,7 @@ // system. this.energyChunkMovers.remove( mover ); this.outgoingEnergyChunks.push( chunk ); + mover.dispose(); break; case EnergyType.HIDDEN: @@ -294,6 +298,7 @@ // energy chunk to which is corresponds should now be visible to the user. this.hiddenEnergyChunks.remove( chunk ); this.energyChunkMovers.remove( mover ); + mover.dispose(); break; default: @@ -403,6 +408,7 @@ } /** + * TODO: should * @public * @override */ Index: energy-forms-and-changes/js/systems/model/BeakerHeater.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- energy-forms-and-changes/js/systems/model/BeakerHeater.js (revision 1f67bd06d9b5888390bee8f637492e65de5ff215) +++ energy-forms-and-changes/js/systems/model/BeakerHeater.js (date 1599785798513) @@ -308,6 +308,7 @@ // remove this energy chunk entirely this.radiatedEnergyChunkList.remove( mover.energyChunk ); this.radiatedEnergyChunkMovers.remove( mover ); + mover.dispose(); } } ); } @@ -329,6 +330,7 @@ this.beaker.addEnergyChunk( mover.energyChunk ); this.energyChunkList.remove( mover.energyChunk ); this.heatingElementEnergyChunkMovers.remove( mover ); + mover.dispose(); } } ); } @@ -353,6 +355,7 @@ this.heatingElementEnergyChunkMovers.push( this.energyChunkPathMoverGroup.createNextElement( mover.energyChunk, this.createHeaterElementEnergyChunkPath( mover.energyChunk.positionProperty.get() ), HEATING_ELEMENT_ENERGY_CHUNK_VELOCITY ) ); + mover.dispose(); } } ); } ```

[zepumph]

The main issue with https://github.com/phetsims/energy-forms-and-changes/issues/350#issuecomment-690818017 was that I was directly calling dispose, instead of PhetioGroup.disposeElement. This handled some of the memory leak I was finding. Still though we are increasing the number of EnergyChunk and EnergyChunkPathMover instances without bound (just slower).

Still needing work:

• Something I did in the above commits has hidden the visible EnergyChunks from EnergyChunkContainerSlices, other EnergyChunks that are single and get emitted/move along path movers work alright.
• The memory leak I discussed above, I feel like we aren't getting rid of all of the EnergyChunks still, and I may reach out to responsible dev for ideas on what I'm missing. Here is an example of two chrome dev tools snapshots to compare: https://drive.google.com/file/d/1Ep2cB9fhIU98lxK_09NEUnTxc18dMgrS/view?usp=sharing and https://drive.google.com/file/d/1v06jwpsYD8ZEWPVWa-7ypOfq0AFZqmsG/view?usp=sharing
• I was able to get an assertion once for an impossible setState, I wouldn't be surprised if there turn out to be more:

  Error: Assertion failed: Impossible set state from iterate; unset state:
  {
  "energyFormsAndChanges.systemsScreen.model.energyUsers.incandescentBulb.energyChunkList": {
  "array": [
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_653",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_654",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_655",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_656",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_657",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_658",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_659",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_660",
    "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_661"
  ]
  },
  "energyFormsAndChanges.systemsScreen.model.energyUsers.incandescentBulb.radiatedEnergyChunkMovers": {
  "array": [
    "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_20",
    "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_24"
  ]
  },
  "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_20": {
  "path": [
    {
      "x": 0.04051891480807868,
      "y": 0.15000000000000002
    }
  ],
  "speed": 0.04,
  "pathFullyTraversed": false,
  "nextPoint": {
    "x": 0.04051891480807868,
    "y": 0.15000000000000002
  },
  "energyChunkPhetioID": "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_653",
  "phetioID": "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_20"
  },
  "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_24": {
  "path": [
    {
      "x": 0.1296498160048301,
      "y": 0.15000000000000002
    }
  ],
  "speed": 0.04,
  "pathFullyTraversed": false,
  "nextPoint": {
    "x": 0.1296498160048301,
    "y": 0.15000000000000002
  },
  "energyChunkPhetioID": "energyFormsAndChanges.systemsScreen.model.energyChunkGroup.energyChunk_654",
  "phetioID": "energyFormsAndChanges.systemsScreen.model.energyChunkPathMoverGroup.energyChunkPathMover_24"
  }
  }

I didn't have access to the full state when this happened.

[zepumph]

https://github.com/phetsims/energy-forms-and-changes/commit/0e5daa3bc04a161a9fc34525e21261f857215b71 fixed the first bullet by recognizing that energyChunk slices are actually static, and don't need to a PhetioGroup. This cleaned up a lot of code. It also changed the buggy behavior of https://github.com/phetsims/energy-forms-and-changes/issues/353. I'll comment over there.

From a basic check, it doesn't look like that changed the memory footprint of energyChunks (still incrementing slowly without bound)

[zepumph]

@jbphet and I found that I'm not disposing for clearEnergyChunks. I'll do that now!

[zepumph]

I just merged to master, and @jbphet and I will work together in the same repo as we go from here.

[zepumph]

In the above I am disposing the appropriate chunks and movers when clearing the chunks. This has led to a much more reasonable counts in the groups while interacting with the state wrapper, but we may still have a slow leak in some cases.

[zepumph]

• [x] Make sure to cherry-pick #353 and #352 when adding to the RC branch.

[zepumph]

• [x] Make sure to cherry-pick https://github.com/phetsims/energy-forms-and-changes/issues/354 when adding to the RC branch
• [x] Make sure to cherry-pick https://github.com/phetsims/energy-forms-and-changes/issues/355 when adding to the RC branch

[zepumph]

As you can see from linked issues, there is still a fair amount of work to do. The main stuff now is just caused by bugs in the friction with how PhetioGroup is strict with registration and de-registration.

• [x] I also need to work on cleanup and documentation.

[zepumph]

• [x] Make sure to cherry-pick https://github.com/phetsims/energy-forms-and-changes/issues/358 to the rc branch.

[zepumph]

• [x] Make sure to cherry-pick #357 to the rc branch.

[zepumph]

I successfully cherry-picked everything above into the branch. Note I left a few superfluous tandem commits out, but included all of the EFAC ones. Here is the ordered list:

https://github.com/phetsims/tandem/commit/3420fef7da5d83f6948ff1eba9850e93aa251307

https://github.com/phetsims/energy-forms-and-changes/commit/65ceee44a9c10181b00c893154283c1ad1f89b5b https://github.com/phetsims/energy-forms-and-changes/commit/33160bed9f2a3d260bb1152ace21dce1d1c3e5d1 https://github.com/phetsims/energy-forms-and-changes/commit/41fcd8523d08f3bb7645fef60efc9884eb5b08f9 https://github.com/phetsims/energy-forms-and-changes/commit/61f3833f649d1a3f0b89676d5f704bd04780836f https://github.com/phetsims/energy-forms-and-changes/commit/4899a57ac936f2f74ef76ec0a0abc2d573a20dc5 https://github.com/phetsims/energy-forms-and-changes/commit/ffbe4d316deaa88c9bd0a5ec5f56f9ebf62b921f https://github.com/phetsims/energy-forms-and-changes/commit/2d0cf12e2c253e9879249087d8ce002cd3ef21e8 https://github.com/phetsims/energy-forms-and-changes/commit/270e5d9bd5d2fba9c7e4e77e06b7171ef8258f61 https://github.com/phetsims/energy-forms-and-changes/commit/9393dd74163ca992f5b71fa14f76c2eda5863cfa https://github.com/phetsims/energy-forms-and-changes/commit/7e2e8fa4bec05bd0338477ee13e79654f6348150 https://github.com/phetsims/energy-forms-and-changes/commit/37104230caf31517d1231fba15172919d3f84594 https://github.com/phetsims/energy-forms-and-changes/commit/1f67bd06d9b5888390bee8f637492e65de5ff215 https://github.com/phetsims/energy-forms-and-changes/commit/458d17a4ef684a2acdea3aa92b1f88f0c1316164 https://github.com/phetsims/energy-forms-and-changes/commit/996ddaa27fa93fb27e53efec1ac059cedd4f0568 https://github.com/phetsims/energy-forms-and-changes/commit/7d926c598605622b12780199c3f15a5c2976c458 https://github.com/phetsims/energy-forms-and-changes/commit/634c3d305d5ac3e786074b587f235c96960d0aca https://github.com/phetsims/energy-forms-and-changes/commit/0e5daa3bc04a161a9fc34525e21261f857215b71 https://github.com/phetsims/energy-forms-and-changes/commit/12203c18eecc3d9a05cf040f477cef5d3dcbf9a7 https://github.com/phetsims/energy-forms-and-changes/commit/e13ef6220ed7b473a3097c672de9026903fa415e https://github.com/phetsims/energy-forms-and-changes/commit/ae4615bb217f9f7c0d5080d065b82099e2f77425 https://github.com/phetsims/energy-forms-and-changes/commit/083035b0c699474337f6b7496e8df5d73e0e4173 https://github.com/phetsims/energy-forms-and-changes/commit/b678ead9a0a731222025f419dce3b63d1ed56c2b https://github.com/phetsims/energy-forms-and-changes/commit/ae4e901b36e341eed40b7ea0b09e5d307b3e9f54 https://github.com/phetsims/energy-forms-and-changes/commit/856ca5a6521711ba3b4582b651dd5962567a3c90 https://github.com/phetsims/energy-forms-and-changes/commit/8fb6ed189f80b5509f65a964f4e6b6d0db9fc238 https://github.com/phetsims/energy-forms-and-changes/commit/6209cfaeeaa696276fcd0c68a369fe255d1de410 https://github.com/phetsims/energy-forms-and-changes/commit/915c30c8fd122058e20f6b4018d0806e19e114ee https://github.com/phetsims/energy-forms-and-changes/commit/09af565e90d2194c961b5197742282e68a0d50f5 https://github.com/phetsims/energy-forms-and-changes/commit/bb27f799537bb1f4d2ab9bbbba993097062d1408 https://github.com/phetsims/energy-forms-and-changes/commit/7c9b8e5c1b368e59b1b0eb90c9d97b3f7b3bc922 https://github.com/phetsims/energy-forms-and-changes/commit/51a142ec2ada794e9d9b98d33a3e74e97da29c76 https://github.com/phetsims/energy-forms-and-changes/commit/c2dd784ad6dfdbb1efe9bf94a0aeb89e0c2fadbe https://github.com/phetsims/energy-forms-and-changes/commit/8e072d13ea4665f49fffb7d4226dd90f0ee337eb https://github.com/phetsims/energy-forms-and-changes/commit/a9693f520b7c9fd730aa24805e4324b248395d30

[zepumph]

Everything above is a cherry-pick into 1.4 branch

[zepumph]

• [x] Many IO Types just have EnergySystemElementIO.documentation = 'My Documentation'; as documentation. Oops.

[jbphet]

Thanks for all the work that you did here @zepumph. 1.4.0-rc.3 is in RC as of this writing (rc.2 had the first round of instrumented energy chunks, rc.1 did not), and so far things are looking good. I've done a fair amount of testing of this code, and have done some reviewing, and will continue the review under #367, so I think it's best to close this issue at this point.