Replace SoundGenerator enableControlProperties with enabledProperty

[pixelzoom]

As noted in #186 (comment) ...

SoundClip enableControlProperties is a bad API.

• It differs from the enabledProperty: TReadOnlyProperty<boolean> API used everywhere else
• It requires all of its Properties to be boolean and to use "true === enabled" logic
• It will make selective PhET-iO instrumentation of the enabledProperty more complicated
• It complicates the usages sites, most of which require only a single Property, e.g.

enableControlProperties: [ energyBalanceVisibleProperty ]

• Some usage sites already need to use DerivedProperty, to deal with a non-boolean Property, or a boolean whose logic is "enabled === false". E.g.:

      enableControlProperties: [
        playTickMarkBumpSoundProperty,
        new DerivedProperty( [ tickMarkViewProperty ], tickMarkView => tickMarkView !== TickMarkView.NONE )
      ]

So I recommend replacing enableControlProperties with enabledProperty. Then usages like the one above would be:

enabledProperty: new DerivedProperty(
  [ playTickMarkBumpSoundProperty, tickMarkViewProperty  ],
  ( playTickMarkBumpSound, tickMarkView ) => playTickMarkBumpSound  && tickMarkView !== TickMarkView.NONE
)

... which can be easily instrumented if desired by adding tandem:.

@jbphet @zepumph thoughts?

[pixelzoom]

There are 33 usages of enableControlProperties. Many (most?) involve 1 boolean Property. The few that involve purely "enabled === true" logic could be replaced with DerivedProperty.or. All of the others involve creating a DerivedProperty anyway.

        enableControlProperties: [
          new DerivedProperty( [ model.showChargesProperty ], showCharges => showCharges === 'all' )
        ]

        enableControlProperties: [ DerivedProperty.not( resetAllButton.buttonModel.isFiringProperty ) ]

        enableControlProperties: [ surfaceTemperatureIndicatorEnabledProperty, isPlayingProperty ]

        enableControlProperties: [ resetNotInProgressProperty, shapeSoundEnabledProperty ],

      enableControlProperties: [
        playTickMarkBumpSoundProperty,
        new DerivedProperty( [ tickMarkViewProperty ], tickMarkView => tickMarkView !== TickMarkView.NONE )
      ]

          enableControlProperties: [
            model.isAudioEnabledProperty,
            model.isRunningProperty
          ]

    const pitchedPopGenerator = new PitchedPopGenerator( { enableControlProperties: [ resetNotInProgressProperty ] } )

         enableControlProperties: [
            model.soundScene.isTonePlayingProperty,
            model.soundScene.button1PressedProperty,
            model.isRunningProperty,
            DerivedProperty.not( model.isResettingProperty )
          ]

There are an additional 4 cases where @zepumph recently applies this odd pattern for #186, which I recommended reverting:

        enableControlProperties: [ DerivedProperty.not( resetInProgressProperty ) ]

        enableControlProperties: [ DerivedProperty.not( model.resetInProgressProperty ) ],

      enableControlProperties: [ DerivedProperty.not( model.resetInProgressProperty ) ]

      enableControlProperties: [ DerivedProperty.not( resetInProgressProperty ) ],

[zepumph]

I think that @jbphet is your lead to discuss this one. My understanding was that sound required that extra level of control for enabled, but if the usages or potential design don't seem to need that, then perhaps it is best to remove this. But I believe it is critical to the implementation of tambo to be able to add in enableControlProperties after construction. I think the primary reason for this is because sound is often not as simple as other tree-like data structures, where each individual instance knows best how to silence itself to avoid clicking/pops and other weirdness. I believe things get problematic when we shutoff from the central gain node, which is why we use enable-control Properties:

tambo/js/soundManager.ts

Lines 467 to 479 in bf81ff4

	// Add the global enable Property to the list of Properties that enable this sound generator.
	soundGenerator.addEnableControlProperty( this.enabledProperty );
	// If this sound generator is only enabled in extra mode, add the extra mode Property as an enable-control.
	if ( options.sonificationLevel === SoundLevelEnum.EXTRA ) {
	soundGenerator.addEnableControlProperty( this.extraSoundEnabledProperty );
	}
	// If a view node was specified, create and pass in a boolean Property that is true only when the node is displayed.
	if ( options.associatedViewNode ) {
	const viewNodeDisplayedProperty = new DisplayedProperty( options.associatedViewNode );
	soundGenerator.addEnableControlProperty( viewNodeDisplayedProperty );

Let's hear from @jbphet.

[pixelzoom]

I hadn't seen SoundGenerator addEnableControlProperty. Diving into the implementation, it makes me even more convinces that this is a horrible API.

[pixelzoom]

SoundGenerator removeEnableControlProperty is never used. So enableControlProperties has no need to be dynamic, no need to be an ObservableArray, and addEnableControlProperty is a convenience method -- at the cost of considerable added complexity in SoundGenerator. I've confirmed that by inspecting uses of addEnableControlProperty.

There's 1 use of addEnableControlProperty in InProportionSoundGenerator that could have been avoided by using the enableControlProperties options.

The other 3 uses of addEnableControlProperty are in soundManger.ts. Again they are a convenience, and could have been avoided by assembling a Property[] that was passed to soundGenerator via the enableControlProperties options. And could therefore be converted to a DerivedProperty passed to as an enabledProperty option.

Also related to soundManger.ts... If we don't want sound when doing a "Reset All", why are we making every generator/clip deal with it? Why are we not handling it globally in soundManager.ts.

[pixelzoom]

I'm going to unassign myself, since it's not a priority right now. PhET might consider allocating resources for sound common code in a future iteration.

[jbphet]

@jbphet thoughts?

I believe I created enableControlProperties as an array so that new Properties could be added at different times in case everything wasn't available to be bundled into a DerivedProperty at construction, or conditions within the sim warranted adding or removing things to gate the sounds. From @pixelzoom's investigation above, it sounds like he has found that using a single DerivedProperty would work fine and would be more consistent with other PhET APIs. I'd be fine with it changing that way, assuming it really does work out in all usages.

[pixelzoom]

Thanks @jbphet. Unassigning until addressing sound issues is prioritized for an iteration.

[jbphet]

This change has been implemented. It seems like a good change in that it is both more consistent with other PhET libraries (such as Scenery) and simplifies the tambo code.

Assigning to @pixelzoom for review, as discussed in today's developer meeting. The biggest code risk is probably how fullyEnabledProperty works in SoundGenerator, so that's probably a decent place to start the review. The changes to SoundGenerator and soundManager should, in general, be the focus of the review, with some spot checking of changes made in the sim code to adapt to the revised API.

[pixelzoom]

I reviewed changes to SoundGenerator in 4f52df7 and soundManager in 4f52df7. The changes looks nice, better API, and I don't have any suggestions or concerns.

I also spot checked some of the sim-specific commits, and noted that there were no commits needed for FEL.

Back to @jbphet in case there's additional work to be done.

[jbphet]

No additional work needed - I think we're done here. Closing.