phetsims / neuron

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

model is not created on demand #129

Closed pixelzoom closed 8 years ago

pixelzoom commented 8 years ago

Similar to https://github.com/phetsims/resistance-in-a-wire/issues/76.

NeuronScreen constructor looks like this:

  function NeuronScreen() {

    var neuronModel = new NeuronModel();

    // NeuronModelAdapter intercepts the default Step function and provides "constant" clock and record Playback
    // features to NeuronModel, see NeuronClockModelAdapter
    var neuronClockModelAdapter = new NeuronClockModelAdapter( neuronModel );

    Screen.call( this,
      function() { return neuronClockModelAdapter; },
      function( model ) { return new NeuronScreenView( model ); },
      { backgroundColor: '#ccfefa' }
    );
  }

The model is not supposed to be instantiated by the constructor, it's supposed to be instantiated on demand by Sim.js, when the Screen's createModel function is called. Unless there's a good reason to do things differently in this sim, the constructor should look like this:

  function NeuronScreen() {
    Screen.call( this,
      function() { return new NeuronClockModelAdapter( new NeuronModel() ); },
      function( model ) { return new NeuronScreenView( model ); },
      { backgroundColor: '#ccfefa' }
    );
  }

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

pixelzoom commented 8 years ago

In https://github.com/phetsims/bending-light/issues/356#issuecomment-245826609, @samreid said:

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

samreid commented 8 years ago

I realized this will not interfere with the current loading bar proposal, but it should still be fixed.

jbphet commented 8 years ago

My guess is that this was done to make it easier for future maintainers to see the steps necessary to create the model and then the "clock adapter" that wraps it, but I agree that doing this is not consistent with how other sims use the Screen constructor. I've changed it so that both the model and the clock adapter are now created on demand, and it works fine when tested on master. Closing.