move stuff out of Sim.js

[pixelzoom]

Sim.js has bloated to 1059 lines. It looks like it was a convenient place to put experimental features, and then those features were never moved elsewhere.

I see the following features in Sim.js. Let's see if we can encapsulate some of these elsewhere.

• [x] fuzzMouse @jonathanolson
• [x] WebGL context loss
• [x] Done button
• [x] data collection @samreid
• [x] profiling @samreid
• [x] performance slowing @jonathanolson
• [x] screenshot @jonathanolson
• [x] fullscreen
• [x] record & playback @jonathanolson (tracked in #301) (deleted in https://github.com/phetsims/joist/issues/754)
• [ ] ❌ show pointer areas @jonathanolson (see https://github.com/phetsims/joist/issues/208#issuecomment-343233397)
• [ ] show/hide popups @jonathanolson
• [x] Display-related code and listeners (https://github.com/phetsims/joist/issues/753)

EDIT: @samreid Made the list into checkboxes

[jonathanolson]

I'd like to move fuzzMouse to Scenery, and just leave a single query-parameter check (and the Scenery method on every frame if present).

I don't have strong preferences on the rest.

[samreid]

I think the best pattern to address the issues raised by @pixelzoom above (and other similar issues) is to put hooks into Sim.js so that events are delivered at certain points in its lifecycle. Then the features above can become require modules that observe the Sim itself for certain events, and act accordingly. Sim.js is already sending some events for together.js--we may need to instrument it with more.

[pixelzoom]

Decided I don't want to do this. I think the developers who put the features in Sim.js should handle moving them out of Sim.js. Unassigning.

[samreid]

I removed the done button in 7dd042a624a6522893720b36f67ff952cd59aa2e

[pixelzoom]

So much for a moratorium on adding more stuff to Sim.js. Since I opened this issue, together initialization and hooks for build-sever have been added.

[samreid]

From the original list, Done button is now gone--but perhaps other data collection features should get rearranged?

[samreid]

So much for a moratorium on adding more stuff to Sim.js.

I'm about to add a proposed replacement for Sim.getStateJSON called Sim.togetherState. Sorry! Once the new one is tested/discussed/debugged/etc, we can delete the old one.

[samreid]

I'm going to need @jonathanolson's help on the features for which he was the primary developer. I'll mark the original comment with @jonathanolson attributions where they seem appropriate. @jonathanolson let me know if you'll be able to work on those.

[samreid]

I can still move these things out:

    // Assuming that together.js API features are enabled, return the state for this screen
    get togetherState() {
      return JSON.stringify( together.getState( this ), SimJSON.replacer, 2 );
    },

    // Assuming that together.js API features are enabled, set the state for this screen
    set togetherState( stateString ) {
      together.setState( this, JSON.parse( stateString, SimJSON.reviver ) );
    },

Would be good to discuss with @jonathanolson where the screenshotting capability should lie.

[jonathanolson]

Should we have a SimView that is responsible for the visual Scenery display separate from Sim.js?

It would help if I was more clear what Sim.js was for, so I could know what specifically should be moved out.

[samreid]

I moved several more features out of Sim.js. Reassigning to @jonathanolson to work on the rest.

[samreid]

Also, @pixelzoom feel free to review the parts I have already worked on.

[samreid]

Should we have a SimView that is responsible for the visual Scenery display separate from Sim.js?

That seems like a great idea for the long run.

It would help if I was more clear what Sim.js was for, so I could know what specifically should be moved out.

Ideally just a container for the screens and PropertySet indicating which screen is selected. SimView could manage the navbar, homescreenview, etc. Maybe some of the startup features (including query parameter processing) could/should be moved to SimLauncher?

[samreid]

It would help if I was more clear what Sim.js was for, so I could know what specifically should be moved out.

Anyways, for our immediate needs, just focusing on what @pixelzoom identified in the issue description would be a great start.

[pixelzoom]

Sim.js is still at 900+ lines.

[samreid]

As noted in #301, refactoring related to scenery input event recording/playback has got Sim.js down to 750 lines. The proposed changes are undergoing testing and will be committed soon.

[samreid]

Getting rid of setChildren/setVisible screenDisplayStrategy in https://github.com/phetsims/joist/issues/298 shaved off another 63 lines.

[ariel-phet]

Marking for developer meeting, this seems like a technical debt issue we should revisit

[samreid]

Sim.js is at 737 lines.

[pixelzoom]

Is the check list of Sim responsibilities (https://github.com/phetsims/joist/issues/208#issue-56107561) up to date? I recall that additional responsibilities were added after this issue was created.

[samreid]

@pixelzoom agrees this needs work, but we will probably have to work with @ariel-phet to set a priority here. It is a fundamental part of the framework, but more than 1hr worth of work.

@jonathanolson volunteered to self-assign and look at the checklist items.

[samreid]

We'll let @jonathanolson take a look around before we consider a larger refactor like SimView.js as described in a comment above.

[pixelzoom]

Since this issue was create, Sim.js has been reduced by 1/3 to 703 lines. Skimming what's left, I think we're in much better shape, and can close this issue.

[pixelzoom]

Reopening.

Sim.js has once again bloated to 906 lines, most recently with 44 lines in https://github.com/phetsims/joist/commit/a148ed90979eb7f148106d0216dd855399608949. Looking at commit messages, my guess is that the bloat is due to PhET-iO.

[samreid]

Around 70 lines are from the animated splash screen.

[samreid]

We are back down to 861 lines. @pixelzoom do you recommend that a developer work on reducing this or call it OK for now?

[pixelzoom]

I see 884 lines, but looks like there have been some commits since https://github.com/phetsims/joist/issues/208#issuecomment-341567471.

In https://github.com/phetsims/joist/issues/208#issuecomment-341150916, @samreid said

Around 70 lines are from the animated splash screen.

Can this be factored out into something that handles the splash screen?

[samreid]

The code is Sim.start:

    start: function() {

      var self = this;

      // In order to animate the loading progress bar, we must schedule work with setTimeout
      // This array of {function} is the work that must be completed to launch the sim.
      var workItems = [];

      var screens = this.screens;

      // Schedule instantiation of the screens
      screens.forEach( function initializeScreen( screen ) {
        workItems.push( function() {

          // Screens may share the same instance of backgroundProperty, see joist#441
          if ( !screen.backgroundColorProperty.hasListener( self.updateBackground ) ) {
            screen.backgroundColorProperty.link( self.updateBackground );
          }
          screen.initializeModel();
        } );
        workItems.push( function() {
          screen.initializeView();
        } );
      } );

      // loop to run startup items asynchronously so the DOM can be updated to show animation on the progress bar
      var runItem = function( i ) {
        setTimeout(
          function() {
            workItems[ i ]();
            // Move the progress ahead by one so we show the full progress bar for a moment before the sim starts up

            var progress = DotUtil.linear( 0, workItems.length - 1, 0.25, 1.0, i );

            // Support iOS Reading Mode, which saves a DOM snapshot after the progressBarForeground has already been
            // removed from the document, see https://github.com/phetsims/joist/issues/389
            if ( document.getElementById( 'progressBarForeground' ) ) {

              // Grow the progress bar foreground to the right based on the progress so far.
              document.getElementById( 'progressBarForeground' ).setAttribute( 'width', (progress * PROGRESS_BAR_WIDTH) + '' );
            }
            if ( i + 1 < workItems.length ) {
              runItem( i + 1 );
            }
            else {

              setTimeout( function() {
                self.finishInit( screens, self.tandem );

                // Make sure requestAnimationFrame is defined
                Util.polyfillRequestAnimationFrame();

                // Option for profiling
                // if true, prints screen initialization time (total, model, view) to the console and displays
                // profiling information on the screen
                if ( phet.chipper.queryParameters.profiler ) {
                  Profiler.start( self );
                }

                // place the rAF *before* the render() to assure as close to 60fps with the setTimeout fallback.
                // http://paulirish.com/2011/requestanimationframe-for-smart-animating/
                // Launch the bound version so it can easily be swapped out for debugging.
                self.boundRunAnimationLoop();

                // Communicate sim load (successfully) to joist/tests/test-sims.html
                if ( phet.chipper.queryParameters.postMessageOnLoad ) {
                  window.parent && window.parent.postMessage( JSON.stringify( {
                    type: 'load',
                    url: window.location.href
                  } ), '*' );
                }

                // After the application is ready to go, remove the splash screen and progress bar
                window.phetSplashScreen.dispose();

                // Signify the end of simulation startup, finish the simStartedEvent.  Used by PhET-iO. This does not
                // coincide with the end of the Sim constructor (because Sim has asynchronous steps that finish after
                // the constructor is completed )
                phetioEvents.end( self.phetioSimStartedEventId );
                self.endedSimConstructionEmitter.emit();

                // Sanity check that there is no phetio object in phet brand, see https://github.com/phetsims/phet-io/issues/1229
                Brand.id === 'phet' && assert && assert( !phet.phetio, 'window.phet.phetio should not be truthy if loading form phet brand' );

              }, 25 ); // pause for a few milliseconds with the progress bar filled in before going to the home screen
            }
          },
          // The following sets the amount of delay between each work item to make it easier to see the changes to the
          // progress bar.  A total value is divided by the number of work items.  This makes it possible to see the
          // progress bar when few work items exist, such as for a single screen sim, but allows things to move
          // reasonably quickly when more work items exist, such as for a four-screen sim.
          30 / workItems.length
        );
      };

      runItem( 0 );
    }

It doesn't seem trivial to separate, but let me know what you recommend.

[pixelzoom]

I don't believe that Sim.js can possibly need to have as many responsibilities as it currently does. I think it's just been easier/convenient to jam more code in Sim.js when we want to add a feature (like the splash screen). But because of things like #460, I am unable to understand https://github.com/phetsims/joist/issues/208#issuecomment-342328089 and other large portions of Sim.js.

I don't recommend closing this issue. I recommend enumerating the responsibilities that Sim.js currently has and who implemented them, targeting some responsibilities for extraction, and having the original implementor do the extraction.

[samreid]

Would this be similar to keeping some functionality (like the Accessibility trait) out of scenery.Node?

[samreid]

• [x] Heartbeat div looks like a candidate for moving out of Sim.js

[pixelzoom]

Would this be similar to keeping some functionality (like the Accessibility trait) out of scenery.Node?

I don't think we'd even need to go that far. I suspect that there are many things that could be factored out via composition.

[samreid]

A few minor improvements in the 3 preceding commits. What about moving this from Sim.js to initialize-globals.js?

    function sleep( millis ) {
      var date = new Date();
      var curDate;
      do {
        curDate = new Date();
      } while ( curDate - date < millis );
    }

    /*
     * These are used to make sure our sims still behave properly with an artificially higher load (so we can test what happens
     * at 30fps, 5fps, etc). There tend to be bugs that only happen on less-powerful devices, and these functions facilitate
     * testing a sim for robustness, and allowing others to reproduce slow-behavior bugs.
     */
    window.phet.joist.makeEverythingSlow = function() {
      window.setInterval( function() { sleep( 64 ); }, 16 );
    };
    window.phet.joist.makeRandomSlowness = function() {
      window.setInterval( function() { sleep( Math.ceil( 100 + Math.random() * 200 ) ); }, Math.ceil( 100 + Math.random() * 200 ) );
    };

[pixelzoom]

... What about moving this from Sim.js to initialize-globals.js?

Since that chunk of code is initializing globals, initialize-globals.js does seem like the correct place for it.

[samreid]

I moved phet.joist.makeEverythingSlow => phet.chipper.makeEverythingSlow and phet.joist.makeRandomSlowness => phet.chipper.makeRandomSlowness, and I'll notify the developers on slack.

[samreid]

After the above commits, we are down to 826 lines of code in Sim.js. One possibility for next steps would be to factor out view parts into something like SimView.js. This would include the display, navigation bar setup, layout, showPopup/hidePopup and display updates. This would be a nontrivial refactoring step. @pixelzoom what do you think?

[samreid]

After the above commit, Sim.js is down to 807 lines of code. All of today's commits will need a careful review, perhaps by @pixelzoom, and we can still discuss factoring out SimView.

[pixelzoom]

I reviewed the commits and don't see anything amiss. @jonathanolson should probably review the factoring out of Heartbeat.js in https://github.com/phetsims/joist/commit/1fe2a7b1a4447de5de824be7d02a8aab842427ff, since I'm unfamiliar with that.

[jonathanolson]

Generally the "singleton" bit is an anti-pattern in Heartbeat. You're also asserting with a string as the first param, which is truthy. In the future, don't assert like that, throw an error.

I'd expect something like:

var Heartbeat = {
  // TODO: at least some documentation, is blank now
  start: function( sim ) {
    // ...
  }
};

joist.register( 'Heartbeat', Heartbeat );

return Heartbeat;

[jonathanolson]

show pointer areas @jonathanolson

Also, this appears to be 2 lines of code. Unless we factor out creation and handling of the Display outside of Sim, the code for this is in the best place.

show/hide popups @jonathanolson

Would the topLayer/barrierRectangle/associated methods be refactored out into a separate file (SimGlassPane?).

[samreid]

I converted Heartbeat.js to the recommended singleton pattern, @jonathanolson can you please verify?

[samreid]

It seems a decision for the team whether we will factor out SimView.js as described in https://github.com/phetsims/joist/issues/208#issuecomment-342368303

To summarize, Sim.js is currently at 809 lines of code (23% smaller than when this issue was created in Jan 30, 2015). Minor cleanup may still be possible. The next "major" refactoring that could help significantly would be to factor out all of the parts that relate to the view into SimView and leave Sim.js as more of the sim model. This would be temporarily destabilizing and at least several hours of work, we need to make the cost/benefit analysis of whether to go for it or not.

[pixelzoom]

In https://github.com/phetsims/joist/issues/208#issuecomment-342368303, @samreid asked:

... One possibility for next steps would be to factor out view parts into something like SimView.js. This would include the display, navigation bar setup, layout, showPopup/hidePopup and display updates. This would be a nontrivial refactoring step. @pixelzoom what do you think?

Model-view separation seems like a fine pattern to apply to Sim.

If we're looking for lower (?) hanging fruit... These things were also identified in https://github.com/phetsims/joist/issues/208#issue-56107561, and I don't know the status of them.

• [ ] record & playback @jonathanolson (tracked in #301)
• [ ] show pointer areas @jonathanolson
• [ ] show/hide popups @jonathanolson

[samreid]

@jonathanolson commented on 2 of these in https://github.com/phetsims/joist/issues/208#issuecomment-342619142

[pixelzoom]

Based on https://github.com/phetsims/joist/issues/208#issuecomment-342619142, it seems OK to ignore "show pointer areas" checklist item.

Re "show/hide popups" checklist item, @jonathanolson asked:

Would the topLayer/barrierRectangle/associated methods be refactored out into a separate file (SimGlassPane?).

Sound like a good idea, but something that should be coordinated with the redesign of Dialog that's being lead by @jessegreenberg. I think we need a general "glass pane" that can be used by all types of popups (dialogs, menus, combo box lists, ...)

[pixelzoom]

11/9/17 dev meeting notes: • Let's see what impact Dialog work has on topLayer/barrierRectangle responsibilities, then reevaluate. Assign this issue to @jessegreenberg to kick back to dev meeting when Dialog work is done. • Longterm, separation of model and view is desirable. • Review record/playback responsibility later.