Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support reproducible playbacks via reproducible random number sequences #98

Open
samreid opened this issue Aug 17, 2016 · 5 comments
Open

Comments

@samreid
Copy link
Member

samreid commented Aug 17, 2016

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:

  • Math.random
  • _.shuffle
  • _.random

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
Copy link
Member Author

samreid commented Aug 17, 2016

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 samreid assigned jbphet and unassigned samreid Aug 17, 2016
@samreid
Copy link
Member Author

samreid commented Aug 17, 2016

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

@jbphet
Copy link
Contributor

jbphet commented Aug 18, 2016

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.

@jbphet jbphet assigned samreid and unassigned jbphet Aug 18, 2016
@samreid
Copy link
Member Author

samreid commented Aug 18, 2016

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.

@samreid samreid assigned jbphet and unassigned samreid Aug 18, 2016
@jbphet
Copy link
Contributor

jbphet commented Apr 5, 2018

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

@jbphet jbphet removed their assignment Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants