search

phetsims / blackbody-spectrum

"Blackbody Spectrum" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

CT impossible set state #117

Closed KatieWoe closed 3 years ago

KatieWoe commented 4 years ago 
blackbody-spectrum : phet-io-state-fuzz : require.js : run
Assertion failed: Impossible set state:
blackbodySpectrum.blackbodySpectrumScreen.model.savedBlackbodyBodyModel~0.temperatureProperty
Error: Assertion failed: Impossible set state:
blackbodySpectrum.blackbodySpectrumScreen.model.savedBlackbodyBodyModel~0.temperatureProperty
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/assert/js/assert.js?bust=1581939143588:22:13)
    at PhetioStateEngine.iterate (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:278:17)
    at setStateOnce (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:176:35)
    at PhetioStateEngine.setState (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:182:9)
    at PhetioEngineIO.implementation (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioEngineIO.js?bust=1581939143732:203:45)
    at PhetioCommandProcessor.processCommand (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:236:53)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:147:38
    at Array.map (<anonymous>)
    at PhetioCommandProcessor.processCommands (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:145:32)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:88:18
id: Bayes Chrome
Approximately 2/17/2020, 3:20:17 AM
blackbody-spectrum : phet-io-state-fuzz : require.js : run
Uncaught Error: Assertion failed: Impossible set state:
blackbodySpectrum.blackbodySpectrumScreen.model.savedBlackbodyBodyModel~0.temperatureProperty
Error: Assertion failed: Impossible set state:
blackbodySpectrum.blackbodySpectrumScreen.model.savedBlackbodyBodyModel~0.temperatureProperty
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/assert/js/assert.js?bust=1581939143588:22:13)
    at PhetioStateEngine.iterate (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:278:17)
    at setStateOnce (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:176:35)
    at PhetioStateEngine.setState (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioStateEngine.js?bust=1581939143732:182:9)
    at PhetioEngineIO.implementation (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/PhetioEngineIO.js?bust=1581939143732:203:45)
    at PhetioCommandProcessor.processCommand (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:236:53)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:147:38
    at Array.map (<anonymous>)
    at PhetioCommandProcessor.processCommands (https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:145:32)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1581934817490/phet-io/js/phetioCommandProcessor.js?bust=1581939143732:88:18
id: Bayes Chrome
Approximately 2/17/2020, 3:20:17 AM
blackbody-spectrum : phet-io-tests : assert
7 out of 7 tests passed. 0 failed.

Approximately 2/17/2020, 3:20:17 AM
blackbody-spectrum : phet-io-tests : no-assert
7 out of 7 tests passed. 0 failed.

Approximately 2/17/2020, 3:20:17 AM
chrisklus commented 4 years ago

On Friday I started looking into fixing this since the team was requested to clear up CT a little. I started by converting to PhetioGroup, but was still getting the same error. @zepumph assisted by pointing out that Group elements need an IO type even if only the element's children are stateful (as is the case in BlackbodyBodyModel). @zepumph also removed this sim from state testing before knowing I was working on this (which is why this error isn't CT anymore).

Today @samreid and I discussed after he helped me get state to a non-breaking point with PhetioGroup (I needed a dispose method in BlackbodyBodyModel to clear out stateful children). Since state still wasn't quite correct and because the implementation wasn't super clean, we decided it would be better to convert to not use any dynamic elements. This means having savedBodyOne and savedBodyTwo instead of savedBodyGroup. I decided to do this now while it's fresh in my mind instead of leaving the sim in a confusing and partially complete phet-io state for someone to stumble upon in the future.

Implemented above. Since the length of the original group was linked to in a few places, I first made a DerivedProperty that had a similar behavior by indicating which saved bodies have a non-null temperature. But then I realized that all of those use cases were assuming that there were only two saved bodies (not an arbitrary-sized group of them), so I refactored those usages to instead access savedBodyOne and savedBodyTwo, and removed the number of saved bodies from the logic. In most places, this felt cleaner than getting the group elements by index. @samreid also mentioned the idea of removing BlackbodyBodyModel by just tracking the three temperatures in BlackbodySpectrumModel and moving all of its methods into BlackbodySpectrumModel, but that felt like a bigger design change that should be signed off on by the responsible dev, if desired. I thought that would be better at first, but there's now an extra validation step in BlackbodyBodyModel since we lost the validation of NumberProperty, so the structure seems pretty nice to me as is.

Since state is fully working and passing fuzzing now, I also removed this sim from perennial/phet-io-state-unsupported.

Assigning to @jbphet to review sim code changes at low priority.

Below is a patch of where I got to with PhetioGroup in case we consider reverting to that in the future:

```diff Index: phet-io/js/phetioEngine.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- phet-io/js/phetioEngine.js (revision 97c59f3e1961b38837f45be471bdf4b2b66c3654) +++ phet-io/js/phetioEngine.js (date 1589222745000) @@ -417,7 +417,15 @@ assert && assert( phetioObject, 'Only non-falsy components should be registered' ); const phetioID = phetioObject.tandem.phetioID; + const validateTandems = Tandem.errorOnFailedValidation(); + if ( this.phetioObjectMap.hasOwnProperty( phetioID ) ) { + debugger; + } + + if ( phetioID === 'blackbodySpectrum.blackbodySpectrumScreen.model.savedBodyGroup.savedBody_0.temperatureProperty' ) { + console.log( 'created temperature property' ); + } assert && validateTandems && assert( !this.phetioObjectMap.hasOwnProperty( phetioID ), 'phetioID already registered: ' + phetioID ); Index: blackbody-spectrum/js/blackbody-spectrum/model/BlackbodySpectrumModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/model/BlackbodySpectrumModel.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/model/BlackbodySpectrumModel.js (date 1589220508000) @@ -10,10 +10,12 @@ */ import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; -import ObservableArray from '../../../../axon/js/ObservableArray.js'; +import PhetioGroup from '../../../../tandem/js/PhetioGroup.js'; +import PhetioGroupIO from '../../../../tandem/js/PhetioGroupIO.js'; import BlackbodyConstants from '../../BlackbodyConstants.js'; import blackbodySpectrum from '../../blackbodySpectrum.js'; import BlackbodyBodyModel from './BlackbodyBodyModel.js'; +import BlackbodyBodyModelIO from './BlackbodyBodyModelIO.js'; class BlackbodySpectrumModel { @@ -42,15 +44,22 @@ // @public {BlackbodyBodyModel} the main body for the simulation this.mainBody = new BlackbodyBodyModel( - BlackbodyConstants.sunTemperature, - tandem.createTandem( 'blackbodyBodyModel' ) - ); + BlackbodyConstants.sunTemperature, { + tandem: tandem.createTandem( 'blackbodyBodyModel' ) + } ); - // @private - this.savedBodiesGroupTandem = tandem.createGroupTandem( 'savedBlackbodyBodyModel' ); - - // @public {ObservableArray.} a property for the user's saved blackbodies - this.savedBodies = new ObservableArray(); + // @public {PhetioGroup.} a property for the user's saved blackbodies + this.savedBodyGroup = new PhetioGroup( ( tandem, temperature ) => { + return new BlackbodyBodyModel( + temperature, { + tandem: tandem, + phetioDynamicElement: true + } ); + }, [ BlackbodyConstants.sunTemperature ], { + tandem: tandem.createTandem( 'savedBodyGroup' ), + phetioType: PhetioGroupIO( BlackbodyBodyModelIO ), + phetioDocumentation: 'group that contains any bodies saved by the user' + } ); // @public {number} max wavelength in nanometers this.wavelengthMax = 3000; @@ -76,12 +85,9 @@ * @public */ saveMainBody() { - this.savedBodies.add( new BlackbodyBodyModel( - this.mainBody.temperatureProperty.value, - this.savedBodiesGroupTandem.createNextTandem() - ) ); - while ( this.savedBodies.length > this.maxSavedGraphs ) { - this.savedBodies.shift(); + this.savedBodyGroup.createNextElement( this.mainBody.temperatureProperty.value ); + while ( this.savedBodyGroup.count > this.maxSavedGraphs ) { + this.savedBodyGroup.disposeElement( this.savedBodyGroup.getElement( 0 ) ); } } @@ -90,7 +96,7 @@ * @public */ clearSavedGraphs() { - this.savedBodies.clear(); + this.savedBodyGroup.clear(); } } Index: blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModel.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModel.js (date 1589228941000) @@ -11,9 +11,13 @@ import NumberProperty from '../../../../axon/js/NumberProperty.js'; import Range from '../../../../dot/js/Range.js'; import Utils from '../../../../dot/js/Utils.js'; +import merge from '../../../../phet-core/js/merge.js'; import Color from '../../../../scenery/js/util/Color.js'; +import PhetioObject from '../../../../tandem/js/PhetioObject.js'; +import Tandem from '../../../../tandem/js/Tandem.js'; import BlackbodyConstants from '../../BlackbodyConstants.js'; import blackbodySpectrum from '../../blackbodySpectrum.js'; +import BlackbodyBodyModelIO from './BlackbodyBodyModelIO.js'; // constants // colors used for glowing star and circles @@ -23,19 +27,26 @@ const GLOWING_STAR_HALO_MINIMUM_RADIUS = 5; // in pixels const GLOWING_STAR_HALO_MAXIMUM_RADIUS = 100; // in pixels -class BlackbodyBodyModel { +class BlackbodyBodyModel extends PhetioObject { /** * Constructs a Blackbody body at the given temperature * @param {number} temperature * @param {Tandem} tandem */ - constructor( temperature, tandem ) { + constructor( temperature, options ) { + + options = merge( { + tandem: Tandem.REQUIRED, + phetioType: BlackbodyBodyModelIO + }, options ); + + super( options ); // @public {Property.} this.temperatureProperty = new NumberProperty( temperature, { range: new Range( BlackbodyConstants.minTemperature, BlackbodyConstants.maxTemperature ), - tandem: tandem.createTandem( 'temperatureProperty' ), + tandem: options.tandem.createTandem( 'temperatureProperty' ), phetioDocumentation: 'blackbody temperature' } ); } @@ -218,6 +229,13 @@ get starColor() { return this.getStarColor(); } + + dispose() { + console.log( 'disposing' ); + this.temperatureProperty.dispose(); + super.dispose(); + } + } blackbodySpectrum.register( 'BlackbodyBodyModel', BlackbodyBodyModel ); Index: blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModelIO.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModelIO.js (date 1589226990000) +++ blackbody-spectrum/js/blackbody-spectrum/model/BlackbodyBodyModelIO.js (date 1589226990000) @@ -0,0 +1,51 @@ +// Copyright 2020, University of Colorado Boulder + +/** + * BlackbodyBodyModelIO allows BlackbodyBodyModel to be serializable (even though its state is tracked by a Property) + * + * @author Chris Klusendorf (PhET Interactive Simulations) + */ + +import validate from '../../../../axon/js/validate.js'; +import ModelElementIO from '../../../../charges-and-fields/js/charges-and-fields/model/ModelElementIO.js'; +import EnumerationIO from '../../../../phet-core/js/EnumerationIO.js'; +import ObjectIO from '../../../../tandem/js/types/ObjectIO.js'; +import BlackbodyConstants from '../../BlackbodyConstants.js'; +import blackbodySpectrum from '../../blackbodySpectrum.js'; +import BlackbodyBodyModel from './BlackbodyBodyModel.js'; + +class BlackbodyBodyModelIO extends ObjectIO { + + /** + * Return the json that BlackbodyBodyModelIO is wrapping. This can be overridden by subclasses, or types can use BlackbodyBodyModelIO type + * directly to use this implementation. + * @param {Object} o + * @returns {Object} + * @public + */ + static toStateObject( o ) { + validate( o, this.validator ); + return { + temperature: o.temperatureProperty.value + }; + } + + /** + * @override + * @param {Object} state - see BlackbodyBodyModelIO.toStateObject + * @returns {Array.<*>} + * @public + */ + static stateToArgsForConstructor( state ) { + + // TODO + return [ state.temperature ]; + } +} + +BlackbodyBodyModelIO.documentation = 'A BlackbodyBodyModel'; +BlackbodyBodyModelIO.typeName = 'BlackbodyBodyModelIO'; +ObjectIO.validateSubtype( BlackbodyBodyModelIO ); + +blackbodySpectrum.register( 'BlackbodyBodyModelIO', BlackbodyBodyModelIO ); +export default BlackbodyBodyModelIO; \ No newline at end of file Index: blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumScreenView.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumScreenView.js (date 1588960331000) @@ -93,7 +93,7 @@ const controlPanel = new BlackbodySpectrumControlPanel( model, { tandem: tandem.createTandem( 'controlPanel' ) } ); - const savedInformationPanel = new SavedGraphInformationPanel( model.mainBody, model.savedBodies, { + const savedInformationPanel = new SavedGraphInformationPanel( model.mainBody, model.savedBodyGroup, { minWidth: controlPanel.width, tandem: tandem.createTandem( 'savedGraphsPanel' ) } ); Index: blackbody-spectrum/js/blackbody-spectrum/view/GraphDrawingNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/view/GraphDrawingNode.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/view/GraphDrawingNode.js (date 1588960331000) @@ -152,7 +152,7 @@ this.updateSavedGraphPaths(); }; model.mainBody.temperatureProperty.link( updateMainGraphAndLayout ); - model.savedBodies.lengthProperty.link( updateSavedGraphAndLayout ); + model.savedBodyGroup.countProperty.link( updateSavedGraphAndLayout ); this.axes.horizontalZoomProperty.link( updateAllGraphs ); this.axes.verticalZoomProperty.link( updateAllGraphs ); @@ -251,13 +251,13 @@ */ updateSavedGraphPaths() { // Updates the saved graph(s) - const numberOfSavedBodies = this.model.savedBodies.length; + const numberOfSavedBodies = this.model.savedBodyGroup.count; this.primarySavedGraph.shape = null; this.secondarySavedGraph.shape = null; if ( numberOfSavedBodies > 0 ) { - this.primarySavedGraph.shape = this.shapeOfBody( this.model.savedBodies.get( numberOfSavedBodies - 1 ) ); + this.primarySavedGraph.shape = this.shapeOfBody( this.model.savedBodyGroup.getElement( numberOfSavedBodies - 1 ) ); if ( numberOfSavedBodies === 2 ) { - this.secondarySavedGraph.shape = this.shapeOfBody( this.model.savedBodies.get( 0 ) ); + this.secondarySavedGraph.shape = this.shapeOfBody( this.model.savedBodyGroup.getElement( 0 ) ); } } } Index: blackbody-spectrum/js/blackbody-spectrum/view/SavedGraphInformationPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/view/SavedGraphInformationPanel.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/view/SavedGraphInformationPanel.js (date 1588960331000) @@ -27,10 +27,11 @@ /** * Makes a SavedGraphInformationPanel given a model that has saved bodies - * @param {BlackbodySpectrumModel} model + * @param {BlackbodyBodyModel} mainBody + * @param {PhetioGroup.} savedBodyGroup * @param {Object} [options] */ - constructor( mainBody, savedBodies, options ) { + constructor( mainBody, savedBodyGroup, options ) { options = merge( { panelFill: 'rgba( 0, 0, 0, 0 )', @@ -121,18 +122,18 @@ } ); // Links the saved bodies to the saved temperature boxes' visibility and text - savedBodies.lengthProperty.link( numberOfSavedBodies => { + savedBodyGroup.countProperty.link( numberOfSavedBodies => { const oldCenterX = this.centerX; this.visible = numberOfSavedBodies > 0; secondarySavedTemperatureBox.visible = numberOfSavedBodies > 1; if ( numberOfSavedBodies > 0 ) { primarySavedTemperatureLabel.text = formatTemperature( - savedBodies.get( numberOfSavedBodies - 1 ).temperatureProperty.value + savedBodyGroup.getElement( numberOfSavedBodies - 1 ).temperatureProperty.value ); // text is set, but this label isn't necessarily visible secondarySavedTemperatureLabel.text = formatTemperature( - savedBodies.get( 0 ).temperatureProperty.value + savedBodyGroup.getElement( 0 ).temperatureProperty.value ); } this.centerX = oldCenterX; Index: blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumControlPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumControlPanel.js (revision 976393308edbab32b9d10bc083b8dd3e74fc9b47) +++ blackbody-spectrum/js/blackbody-spectrum/view/BlackbodySpectrumControlPanel.js (date 1588960331000) @@ -106,8 +106,8 @@ } ); // Makes the eraseButton enabled when there is a saved graph to clear, and disabled when there is no graph to clear - model.savedBodies.lengthProperty.link( length => { - eraseButton.enabled = length !== 0; + model.savedBodyGroup.countProperty.link( count => { + eraseButton.enabled = count !== 0; } ); // 3 checkboxes: Peak Values, Intensity, Labels ```
jbphet commented 4 years ago

@chrisklus - thanks for the clear explanation, I appreciate it.

[W]e decided it would be better to convert to not use any dynamic elements.

I don't love this solution since it reduces the generality of the simulation. However, I understand the tradeoffs between getting something working and keeping it general, and I'm somewhat familiar with the challenges of using PhetioGroup for dynamic elements.

[T]he structure seems pretty nice to me as is.

Agreed, let's not do too big of a refactor at this point.

@chrisklus - This has been assigned to me for almost a month, and I think some evolution of PhetioGroup has taken place in that time due to the work being done to support its usage in Natural Selection. Please have one more look at this and, if you still feel that this is the best solution, please add a note in BlackbodySpectrumModel about why there are two separate instances instead of an array, and then close. I've reviewed the code and, assuming we stick with the two-separate-non-dynamic-saved-graphs solution, I'm okay with it.

If PhetioGroup is to the point where it has better support for dynamic elements, then having the saved BlackbodyBodyModel instances in an array would still be the preferred choice. I'll leave it to your discretion.

chrisklus commented 3 years ago

I looked into this again and still feel the existing state is the best solution. The original implementation was not general to any number of saved bodies, see here, and the new implementation is cleaner and simpler in my opinion, see here and here.

Since there hasn't been a request for more than two saved bodies, the way it is seems best to me. I don't think it would be a ton of work to generalize it if ever needed, but this issue feel very low priority and has been sitting for a long time, so my preference is to not take that time now based on my reasoning above. I'm going to go ahead and close since this was left to my discretion, but @jbphet feel free to reopen and assign me to the refactor if you feel strongly.

chrisklus commented 3 years ago

Oops, I forgot to commit my comment, added above.