phetsims / bending-light

"Bending Light" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/bending-light
GNU General Public License v3.0
8 stars 8 forks source link

PrismsModel is not instantiated on demand #356

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

See PrismScreen constructor:

  function PrismsScreen( tandem ) {

    var options = {
      name: prismsString,
      homeScreenIcon: new Image( iconImage ),
      navigationBarIcon: new Image( navbarIconImage ),
      tandem: tandem
    };

39  var prismModel = new PrismsModel();

    Screen.call( this,
42    function() { return prismModel; },
      function( model ) { return new PrismsView( model ); },
      options );
  }

Note that PrismsModel is instantiated in the constructor (line 39), instead of in the createModel function argument (line 42) to Screen. This doesn't match the pattern used throughout PhET sims. Is there a reason for this deviation, or is it accidental?

pixelzoom commented 7 years ago

Unless there's a reason to do things differently, the model should be instantiated in the createModel argument, like this:

function PrismsScreen( tandem ) {

  var options = {
      name: prismsString,
      homeScreenIcon: new Image( iconImage ),
      navigationBarIcon: new Image( navbarIconImage ),
      tandem: tandem
    };

  Screen.call( this,
    function() { return new PrismsModel(); }, 
    function( model ) { return new PrismsView( model ); }, 
    options );
}

If there is a legitimate reason why it needs to be different, please document it.

samreid commented 7 years ago

Yes, this looks like a problem, I'll work on it this week.

pixelzoom commented 7 years ago

FYI, similar problems in other sims: https://github.com/phetsims/neuron/issues/129, https://github.com/phetsims/resistance-in-a-wire/issues/76.

samreid commented 7 years ago

Those problems will interfere with the loading bar (if nothing else), we should address them.

samreid commented 7 years ago

Fixed above. I realized that the loading bar now goes by screen, so this would not have interfered. But if we break up the loading bar to load models and views separately, then this could have shown up as odd.

I tested the app by launching it after making this change, and all seems well.

Closing.