Use NumberControl instead of custom button+slider in SolarSystemCommonSlider

[zepumph]

In phetsims/solar-system-common@7404291 buttons were added to this solar system common slider. This should instead be a NumberControl because you get a lot of support for free with that solution. I'll take a look!

[samreid]

In today's meeting, it sounded like this is done or nearly done? I'll label as ready for review in case that's true.

[zepumph]

I do not agree. Let's first take care of the TODOs pointing to this issue:

https://github.com/phetsims/solar-system-common/blob/d0174288e9e758bf6109e80f4c89945f906ed5f8/js/view/SolarSystemCommonSlider.ts#L79-L85

[samreid]

I added different sounds for the min and max, but the implementation is not very elegant and there may be a better way to do it.

[AgustinVallejo]

Thanks! Although found a problem with this implementation: When you move it to the border and back, it doesn't sound. We can pair tomorrow to take a look into it and review :)

[zepumph]

Ok, @samreid and I got to a good point on this issue. We have two more remaining todos, and they are both sound related:

• What should the min/max sounds be? Currently committed is the same sound as general steps throughout the slider. Do we want something else? If we keep it the way it is, Slider doesn't have good support for it, so we should work on the code quality a bit.
• What should the arrow button sounds be? @samreid and I experimented with mapping the value continuously, but found that the playback rate only changes 0.003 per button tweak, so it takes ~6 button presses to hear any noticeable change.

It will be best to proceed with a sound design meeting.

[pixelzoom]

In phetsims/scenery-phet@574887b, @zepumph left dead code in NumberControl.ts, at line 591:

        children: [
    /*      new HBox( {
            spacing: options.titleXSpacing,
            children: [ titleNode, numberDisplay ]
          } ),*/

[arouinfar]

To be discussed in conjunction with #131

[samreid]

I'll reassign @zepumph in case he is needed regarding #105 (comment). Self-unassigning, but please re-invite me if needed.

[zepumph]

I am not, thanks for the ping. That was accidental commit that was correctly reverted by @jessegreenberg. Sorry for the trouble.

[arouinfar]

Discussed with @DianaTavares @emily-phet @Ashton-Morris @jbphet. We reviewed the latest Mass control sounds on master. The increment/decrement buttons use the same sound as the slider.

What should the min/max sounds be? Currently committed is the same sound as general steps throughout the slider. Do we want something else?

Not all siders need special min/max sounds. This feels analogous to the Mass slider on Gravity Force Lab (turn on Enhanced Sound) or Solution Volume in Molarity. We think it can stay the way it is.

If we keep it the way it is, Slider doesn't have good support for it, so we should work on the code quality a bit.

@jbphet was surprised by this and will follow up with @zepumph to find out more.

What should the arrow button sounds be? @samreid and I experimented with mapping the value continuously, but found that the playback rate only changes 0.003 per button tweak, so it takes ~6 button presses to hear any noticeable change.

The increment/decrement buttons make small changes, so this behavior is expected and reasonable. The sound design team is happy with the way it currently sounds.

Leaving open for @AgustinVallejo to review, and assigning @jbphet to reach out to @zepumph.

[zepumph]

Happy to do something sync, but likely you will see it to. Basically we want three sound features, and have to duplicate something 3 times to get it:

1. Slider sound
2. Arrow Button sounds
3. Min max sounds

All of which to be the same ValueChangeSoundGenerator, and potentially to be internal to Slider/NumberControl if possible.

https://github.com/phetsims/solar-system-common/blob/1da44066f95e716afd931270bdb80eefe7f7d445/js/view/SolarSystemCommonNumberControl.ts#L32-L44

Some more specifics:

1. We love the way the the ValueChangeSoundGenerator is working internally to slider. We want to use it everywhere because it sounds so good.
2. passing in valueChangeSoundGeneratorOptions to slider doesn't work well because that creates a private soundGenerator that we can't use, so we are making our own and passing it through.
3. We found that numberControl was providing defaults, and couldn't be opted out, so we needed to remove an assertion in Slider about mutual exclusive options (only the options or the sound generator, but never both).
4. We want to use that valueChangeSoundGenerator on the ArrowButtons, so that they can play the sound associated with each tweaker change.
5. We want to use the same valueChangeSoundGenerator as the min/max sounds, but there is no support for that, so we have pulled out hard coded sounds to play in those cases, what about an option like defaultSoundForMinMaxToo

I would highly recommend @jbphet puts some eyes on the code linked above. I'm happy to help in the cleanup if desired, but it seems like you all have a good design and can execute from here. I have no skin in the game, but was just running up against a lot of duplication needs for the sound generation.

[AgustinVallejo]

04/07 with @arouinfar and @jonathanolson:

Hey @jbphet, as this is no longer a My Solar System issue, if you think this should be worked on, could you transfer the necessary comments to a Sun issue and close this one?

[zepumph]

as this is no longer a My Solar System issue

I'm not really sure what you mean, there are so many problems with sound in SolarSystemCommonNumberControl. @samreid and I wrote hacky "talking points" as we were working on the problem 2 months ago and now it has been long enough that we can just put those into production?

1. Arrow button tweakers sound never changes based on mass, it is just the "default" mass sound. This is a bug to the sonification, and likely hasn't been signed off on.

2. I don't know how we can feel good about this block of code. If it was my sim, I would want to work on ValueChangeSoundPlayer to remove all the duplication in the implementation. If that isn't in the cards can we at least open up a common code code issue to support our min-max- sound needs generally, and write TODOs here so that it can get cleaned up.

   https://github.com/phetsims/solar-system-common/blob/4fbca25621c20a43ae664264923f3669f7286a54/js/view/SolarSystemCommonNumberControl.ts#L33-L58

3. I stepped in here because, from my understanding, @jbphet has been on the outskirts of this issue, and I didn't want to put parsing my giant comment solely on him.

[zepumph]

Ok. @jbphet @AgustinVallejo and I met and had a productive time. There are three sub issues, but generally we will want an issue to hone in on some NumberControl + sound common fixes (in another issue phetsims/scenery-phet#806).

I believe a good number of those items should block publication of this, and may require cleanup over here once complete, so I'll mark as blocked and blocking :)

[AgustinVallejo]

@jbphet Fixed this in phetsims/scenery-phet#806, thanks! Closing