Add sound to NumberControl

[zepumph]

Over in phetsims/my-solar-system#105, My Solar System is in need of some sound-related help for its NumberControl. Here are three things we are feeling:

1. We need to create our own ValueChangeSoundGenerator so that arrow buttons and the slider can use it, but this duplicates the code in Slider that is creating the Slider, and that would be much nicer to not need to instantiate it for dependency injection, and instead use the options.
2. @jbphet @AgustinVallejo and I totally couldn't figure out how to get the sound to change on every value change in the arrow buttons. It has to do with how slider accepts a sound generator, but arrow buttons only support a soundPlayer.
3. The mix/max sounds for ValueChangeSoundGenerator have to be provided separately from the main value change sound. It would be nice to add an option where you don't use the min/maxSoundPlayer options and just play the "main" sound for min and max, calibrated to the min/max values.

Subject: [PATCH] f
---
Index: js/view/SolarSystemCommonNumberControl.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/view/SolarSystemCommonNumberControl.ts b/js/view/SolarSystemCommonNumberControl.ts
--- a/js/view/SolarSystemCommonNumberControl.ts	(revision cc0dbed21416b18cef320f0cf5862e29feb31ea0)
+++ b/js/view/SolarSystemCommonNumberControl.ts	(date 1681162302060)
@@ -21,6 +21,7 @@
 import Tandem from '../../../tandem/js/Tandem.js';
 import ValueChangeSoundPlayer from '../../../tambo/js/sound-generators/ValueChangeSoundPlayer.js';
 import SolarSystemCommonConstants from '../SolarSystemCommonConstants.js';
+import nullSoundPlayer from '../../../tambo/js/shared-sound-players/nullSoundPlayer.js';
 
 type SelfOptions = EmptySelfOptions;
 
@@ -44,20 +45,24 @@
     const massSliderSoundClip = new SoundClip( Mass_Slider_Bass_Pluck_mp3 );
     soundManager.addSoundGenerator( massSliderSoundClip );
 
+    const delta = 1; // TODO: duplicated with the default for Number Control
     const valueChangeSoundGeneratorOptions = {
       middleMovingUpSoundPlayer: massSliderSoundClip,
       middleMovingDownSoundPlayer: massSliderSoundClip,
 
-      minSoundPlayer: minMassSliderSoundClip,
-      maxSoundPlayer: maxMassSliderSoundClip,
+      noMinMaxSoundsPlease: true, // TODO: and assert that this doesn't get provided with other soundPlayers for min/max
+      // minSoundPlayer: minMassSliderSoundClip, // Don't null this out as the option because that applies too much semantics to `null`
+      // maxSoundPlayer: maxMassSliderSoundClip,
       middleMovingUpPlaybackRateMapper: playbackRateMapper,
 
       interThresholdDelta: SolarSystemCommonConstants.SLIDER_STEP - 0.1
     };
 
+    // TODO: Likely nicer if valueChangeSoundPlayer was created in NumberControl and supported by arrows and slider there!
     const valueChangeSoundGenerator = new ValueChangeSoundPlayer( range, valueChangeSoundGeneratorOptions );
 
     const options = optionize<SolarSystemCommonNumberControlOptions, SelfOptions, NumberControlOptions>()( {
+      delta: delta,
       sliderOptions: {
         trackSize: new Dimension2( 226, 2 ),
         thumbSize: new Dimension2( 15, 25 ),
@@ -65,7 +70,6 @@
         thumbCenterLineStroke: 'black',
         trackFillEnabled: SolarSystemCommonColors.foregroundProperty,
         trackStroke: SolarSystemCommonColors.foregroundProperty,
-        soundGenerator: valueChangeSoundGenerator,
 
         //a11y
         accessibleName: SolarSystemCommonStrings.a11y.massSliderStringProperty
@@ -82,7 +86,19 @@
         lineWidth: 1,
         scale: 0.8,
         touchAreaYDilation: 2.5,
-        soundPlayer: massSliderSoundClip
+        soundPlayer: nullSoundPlayer,
+        // TODO: this will error because it also has "general" change callbacks, assertion at the bottom of NumberControl
+        leftStart() { // TODO: This is only on first press!?!?
+          valueChangeSoundGenerator.playSoundForValueChange( valueProperty.value, valueProperty.value + delta );
+        },
+        leftEnd() { // TODO: This is only on release!?!?
+          // TODO: having both makes a double sound on a single press
+          // valueChangeSoundGenerator.playSoundForValueChange( valueProperty.value, valueProperty.value + delta );
+          console.log( 'fjdkslafdkjsla' );
+        },
+        rightStart() {
+          valueChangeSoundGenerator.playSoundForValueChange( valueProperty.value, valueProperty.value - delta );
+        }
       },
       layoutFunction: ( titleNode, numberDisplay, slider, decrementButton, incrementButton ) => {
         assert && assert( decrementButton && incrementButton );

Here are the notes I took (poorly) during the meeting, with TODOs as thoughts that mostly just copy the above statements. Over to @jbphet to get some time before the publication of My Solar System. (3) is just a cleanup, but to me (1) and (2) are actually blocking the sim.

[jbphet]

I've implemented the requested behavior. I'm assigning to @zepumph for review of the common code parts (scenery-phet and tambo) and to @AgustinVallejo for review of the changes to My Solar System. The changes aren't huge, so they shouldn't take too long to review.

A note for historical reference: I ended up going with an approach where NumberControl takes options for a sound player rather than one where the sound player is extracted from the Slider instance used within NumberControl. My reasoning for this was that the latter breaks encapsulation, and could lead to a situation where someone changes the default behavior of the sound in Slider and has no idea that they are also affecting NumberControl. Also, it just doesn't seem very intuitive that the sound-related behavior for a NumberControl would be configured through its sliderOptions. While this leads to a bit of code duplication between NumberControl and Slider, I'm okay with that, since it's not a ton, and since I think it is possible that they could diverge over time, i.e. if we decide at some point that we want different default sounds for NumberControl versus Slider.

[jbphet]

Also, I tested the changes in my-solar-system, and regression tested the slider and number control behavior in geometric-optics, greenhouse-effect, and fourier-making-waves, and they all seemed fine.

[AgustinVallejo]

Works as a charm in MSS, closing phetsims/my-solar-system#105

[zepumph]

• Now that we have more control about NumberControl passing soundGenerating logic into slider, should we revert phetsims/sun@b4a5fd2?

Otherwise things are looking really nice. Thanks for the fast turnaround. I love all the code that was cleaned up in MSS.

[jbphet]

Now that we have more control about NumberControl passing soundGenerating logic into slider, should we revert phetsims/sun@b4a5fd2?

Yes, I think so, at least I'm fairly sure it should be reverted. I didn't fully understand the original motivation for this, and there is an issue assigned to @AgustinVallejo about it, see #802. @AgustinVallejo - please review these issues and undo the commenting out of the assertion unless there's a good reason not to do so.