phetsims / build-an-atom

"Build an Atom" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/build-an-atom
GNU General Public License v3.0
11 stars 10 forks source link

change model to use 1-based level numbering #188

Closed pixelzoom closed 6 years ago

pixelzoom commented 6 years ago

In https://github.com/phetsims/vegas/issues/73 and https://github.com/phetsims/vegas/issues/75, the consensus was to make vegas use 1-based level numbering. And that 1-based level numbering in the model may be advantageous for PhET-iO, but should be evaluated on a sim-by-sim basis.

Making the requested change for FiniteStatusBar proved to be complicated. It has option levelProperty, which is typically a model Property. And several sims (including this one) use 0-based level numbering in the model, so that the level number can be used as an index into arrays. I investigated changing the semantics of the model Property to be 1-based, but that turned into a large task, touching dozens of files, potentially error prone.

To avoid changing the model semantics, I added a DerivedProperty that handles the mapping from 0-based to 1-based level numbering. In general, it looks like this:

var statusBar = new FiniteStatusBar( ..., {
  ...
  // FiniteStatusBar uses 1-based level numbering, model is 0-based.
  levelProperty: new DerivedProperty( [ model.levelProperty ], 
    function( level ) { return level + 1; } )
} ); 

At some point, evaluate whether this sim's model should be converted to 1-based level numbering. If so, after converting, the above DerivedProperty can be removed, i.e.:

var statusBar = new FiniteStatusBar( ..., {
  ...
  levelProperty: model.levelProperty
} ); 
pixelzoom commented 6 years ago

Here's this sim's usage of FiniteStatusBar, in BAAGameScreenView:

var scoreboard = new FiniteStatusBar( {
  ...
  levelProperty: gameModel.levelProperty,
  ...
  levelVisible: false,
  ...
} );

Because the level is not visible, there's no reason to provide option levelProperty. So rather than add the DerivedProperty adapter described above, I'm going to simply remove the levelProperty option.

jbphet commented 6 years ago

The change looks reasonable, and I played a level of the game and everything looked fine. Thanks, closing.