phetsims / area-builder

"Area Builder" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/area-builder
GNU General Public License v3.0
1 stars 2 forks source link

Support reproducible playbacks via reproducible random number sequences #98

Open samreid opened 8 years ago

samreid commented 8 years ago

In order to support https://github.com/phetsims/phet-io/issues/548 and playback by input events, we will need to have replicable random number sequences. This simulation uses the following:

and those usages are done in static code (before phet-io has a chance to provide a seed). We will need to overcome those 4 problems in order to support replicable playback.

samreid commented 8 years ago

In the above commit, I replaced Math.random, .shuffle and .random with equivalents in dot.Random, which is statically seeded, and moved to a pattern where the random numbers are not used until after launchApplication is called. In my limited testing, things seem OK but this should be carefully reviewed by @jbphet.

samreid commented 8 years ago

I added another commit that uses the new joist.random.

jbphet commented 8 years ago

I don't understand why AreaBuilderChallengeFactory was converted from a static object with methods for generating challenges to an object that has to be instantiated. I prefer the former. I looked through the old version, and I think it wasn't invoking the methods that generated random behavior during the load time, so it should be okay to have it remain as a static factory object.

Assigning back to @samreid, and I'll continue the review based on his response.

samreid commented 8 years ago

Here's a truncated version of the previous version of AreaBuilderChallengeFactory that shows where _.shuffle was used statically:

// Copyright 2014-2015, University of Colorado Boulder

/**
 * A factory object that creates the challenges for the Area Builder game.
 *
 * @author John Blanco
 */
define( function( require ) {
  'use strict';

  // modules
  var areaBuilder = require( 'AREA_BUILDER/areaBuilder' );
  // etc...

  // more etc...

  // Color pair chooser, used for selecting randomized colors for two tone 'build it' challenges.
  var COLOR_PAIR_CHOOSER = {
    colorPairList: _.shuffle( [
      {
        color1: AreaBuilderSharedConstants.GREENISH_COLOR,
        color2: AreaBuilderSharedConstants.DARK_GREEN_COLOR
      },
      {
        color1: AreaBuilderSharedConstants.PURPLISH_COLOR,
        color2: AreaBuilderSharedConstants.DARK_PURPLE_COLOR
      },
      {
        color1: AreaBuilderSharedConstants.PALE_BLUE_COLOR,
        color2: AreaBuilderSharedConstants.DARK_BLUE_COLOR
      },
      {
        color1: AreaBuilderSharedConstants.PINKISH_COLOR,
        color2: AreaBuilderSharedConstants.PURPLE_PINK_COLOR
      }
    ] ),
    index: 0
    // etc...
  };

  // etc...

  areaBuilder.register( 'AreaBuilderChallengeFactory', AreaBuilderChallengeFactory );

  return AreaBuilderChallengeFactory;
} );

I'm not opposed to using the former pattern, and I experimented with keeping the former pattern but moving the random calls to be nonstatic, but eventually I settled on this because it was a refactoring step I was fairly confident that wouldn't break anything. Perhaps since you are more familiar with the simulation you can help get a nice pattern and also no static randoms.

jbphet commented 6 years ago

Unassigning. This should be investigated and addressed the next time work is done on this sim.