use factory pattern for creating scene icons

[pixelzoom]

Related to #259 code review.

This was originally a REVIEW comment, promoted to an issue because it got complicated.

WaveInterferenceSceneIcons is currently a class that is instantiated, and it's fields are the individual icons. Current implementation:

  class WaveInterferenceSceneIcons {
    constructor() {

      // @public {FaucetNode} - Faucet icon, rasterized to clip out invisible parts (like the ShooterNode)
      this.faucetIcon = new FaucetNode( 1, new NumberProperty( 0 ), new BooleanProperty( true ), {
        interactiveProperty: new BooleanProperty( false )
      } ).rasterized();

      // @public - Speaker icon
      this.speakerIcon = new Image( speakerImage );

      // @public - Laser Pointer icon
      this.laserPointerIcon = new LaserPointerNode( new BooleanProperty( false ), LightWaveGeneratorNode.DEFAULT_OPTIONS );

      // Uniform sizing.
      const iconWidth = 29;
      this.faucetIcon.scale( iconWidth / this.faucetIcon.width * 0.7 );
      this.speakerIcon.scale( iconWidth / this.speakerIcon.height );
      this.laserPointerIcon.scale( iconWidth / this.laserPointerIcon.width );
    }
  }

And the usage in WaveInterferenceControlPanel is:

179 const sceneIcons = new WaveInterferenceSceneIcons();
...
const sceneRadioButtonGroup = new RadioButtonGroup( model.sceneProperty, [
        { value: model.waterScene, node: sceneIcons.faucetIcon },
        { value: model.soundScene, node: sceneIcons.speakerIcon },
        { value: model.lightScene, node: sceneIcons.laserPointerIcon }
      ], {

REVIEW comment thread in WaveInterferenceSceneIcons:

//REVIEW very odd to have a class for this, especially since the icons are not at all related. Factory pattern would be better.
//REVIEW* They are all related in that they have commensurate dimensions, please see the last few lines of the constructor. What do you recommend?

[pixelzoom]

@samreid asked:

//REVIEW* They are all related in that they have commensurate dimensions, please see the last few lines of the constructor. What do you recommend?

Factory implementation (untested):

// Copyright 2018, University of Colorado Boulder

/**
 * Creates icons for each of the scenes.
 *
 * @author Sam Reid (PhET Interactive Simulations)
 */
define( require => {
  'use strict';

  // modules
  const BooleanProperty = require( 'AXON/BooleanProperty' );
  const FaucetNode = require( 'SCENERY_PHET/FaucetNode' );
  const Image = require( 'SCENERY/nodes/Image' );
  const LaserPointerNode = require( 'SCENERY_PHET/LaserPointerNode' );
  const LightWaveGeneratorNode = require( 'WAVE_INTERFERENCE/common/view/LightWaveGeneratorNode' );
  const NumberProperty = require( 'AXON/NumberProperty' );
  const speakerImage = require( 'image!WAVE_INTERFERENCE/speaker/speaker_MID.png' );
  const waveInterference = require( 'WAVE_INTERFERENCE/waveInterference' );

  const SceneIconFactory = {

    /**
     * Creates the icon for the 'water' scene, a faucet.
     * @param {number} width
     * @returns {Node}
     */
    createWaterSceneIcon( width ) {
      const icon = new FaucetNode( 1, new NumberProperty( 0 ), new BooleanProperty( true ), {
        interactiveProperty: new BooleanProperty( false )
      } ).rasterized();
      icon.scale( width / icon .width * 0.7 );
      return icon;
    },

    /**
     * Creates the icon for the 'light' scene, a laser pointer.
     * @param {number} width
     * @returns {Node}
     */
    createLightSceneIcon( width ) {
      const icon = new LaserPointerNode( new BooleanProperty( false ), LightWaveGeneratorNode.DEFAULT_OPTIONS );
      icon.scale( width / icon.width );
      return icon;
    },

    /**
     * Creates the icon for the 'sound' scene, a speaker.
     * @param {number} width
     * @returns {Node}
     */
    createSoundSceneIcon( width ) {
      const icon = new Image( speakerImage );
      icon.scale( width / icon.height );
      return icon;
    }
  };

  return waveInterference.register( 'SceneIconFactory', SceneIconFactory );
} );

Usage in WaveInterferenceControlPanel :

const SceneIconFactory = require( 'WAVE_INTERFERENCE/common/view/SceneIconFactory' );
...
const SCENE_ICON_WIDTH = 29;
...
const sceneRadioButtonGroup = new RadioButtonGroup( model.sceneProperty, [
  { value: model.waterScene, node: SceneIconFactory.createWaterSceneIcon( SCENE_ICON_WIDTH ) },
  { value: model.soundScene, node: SceneIconFactory.createSoundSceneIcon( SCENE_ICON_WIDTH ) },
  { value: model.lightScene, node: SceneIconFactory.createLightSceneIcon( SCENE_ICON_WIDTH ) }
], {
...

[pixelzoom]

While preparing the above recommendation, I noticed a couple of anomalies in WaveInterferenceSceneIcons related to @samreid's statement that "they have commensurate dimensions":

(1)faucetIcon is in fact being sized differently: this.faucetIcon.scale( iconWidth / this.faucetIcon.width * 0.7 ); Why not just use a different iconWidth, if that's what is desired?

(2) speakerIcon is being scaled based on its height: this.speakerIcon.scale( iconWidth / this.speakerIcon.height ); while the others are based on width. Is this a bug? Reason that needs to be documented?

[pixelzoom]

One more nit about WaveInterferenceSceneIcons, in case you choose to keep the "instantiated class" approach instead of factory... The icon names (faucetIcon, speakerIcon, laserPointerIcon) are not at all related to scene names. waterIcon, soundIcon, and lightIcon would be more appropriate.

[samreid]

I converted the icons to constants and improved the naming, please review.

[pixelzoom]

Code looks good. Node constants can sometimes result in an unintended memory leak, so just confirming... Are these icons children of any Node that is disposed? If so, you'll need to explicitly removeChild before dispose, so that these constant Nodes don't keep a reference to their parent.

Btw... This potential memory leak is why I prefer factory methods to constants for icons, unless there's a clear performance/memory advantage to the constants.

[samreid]

These nodes are not involved in disposal, the scene selection buttons persist for the life of the sim. Thanks for explaining that memory leak, I have seen it before but not been able to explain it so clearly. OK to close?

[pixelzoom]

👍 Closing.

[samreid]

This is causing crashing during startup, see #301. The problem is that calling new FaucetNode outside of main tries to use the image before it has dimensions.

Built issue: Error: attribute transform: Expected number, "scale(NaN,NaN)". #301

[samreid]

Proposed fix is pushed, @pixelzoom can you please review?

[pixelzoom]

This is causing crashing during startup ...

Yeah, that's why I recommended factory methods and not constants. Now we're sort of back to where we started, with an instantiated class. But I guess that's OK.

[samreid]

I prefer the instantiated class to #280 (comment), can this issue be closed?

[pixelzoom]

Yep.