search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Slider API for sound #697

Closed pixelzoom closed 2 years ago

pixelzoom commented 3 years ago

From https://github.com/phetsims/fourier-making-waves/issues/54#issuecomment-825159430:

It's time to investigate generalized support for sound in Slider. What do sliders with pedagogical sounds have in common that can be supported in common code? Is there a good default for sliders with non-pedagogical sounds? Continuous vs discrete sliders? etc. @kathy-phet would like @jbphet to work on this.

pixelzoom commented 3 years ago

Required for Fourier, see https://github.com/phetsims/fourier-making-waves/issues/56.

@jbphet FYI, until the Slider sound API is available, I've implemented crude non-pedagogical sounds for amplitude sliders in Fourier. See AudibleSlider.js. It uses the same sounds as WaveInterferenceSlider, at the request of Fourier design team.

pixelzoom commented 3 years ago

See https://github.com/phetsims/wave-interference/issues/523 for some question about WaveInterferenceSlider.js that might inform the Slider sound API.

Ashton-Morris commented 3 years ago

A lot of sliders end up having a pedagogical need, but for the ones that don't I feel that the Waves Intro slider UI sound would work well as a default.

https://phet.colorado.edu/sims/html/waves-intro/latest/waves-intro_en.html

pixelzoom commented 3 years ago

... I feel that the Waves Intro slider UI sound would work well as a default. ...

Indeed. From https://github.com/phetsims/fourier-making-waves/issues/54#issuecomment-820753954:

Amplitude sliders would probably not have a "harmonic" sound, probably just click, click, click (like Waves Intro).

And the current implementation of fourier-making-waves/AudibleSlider.js uses the same sounds and behavior as WaveInterferenceSlider.js (but is not a copy-paste of its implementation).

jbphet commented 3 years ago

@Ashton-Morris and I discussed this in the 4/27/2021 sound design meeting, here are our initial thoughts:

pixelzoom commented 3 years ago

Don't forget to consider the Slider track. Clicking on the track moves immediately to that value. Does that need a sound? How does that work with discrete vs continuous?

jbphet commented 3 years ago

@Ashton-Morris - Can you comment on @pixelzoom's question just above? I don't think we have considered this much in previous designs. In Waves Intro there doesn't appear to be a sound at all. In Molarity, it makes a single sound for the new position, which seems quite nice. In Gravity Force Lab it activates the continuous sound for a short time (and I know this isn't coming directly from the slider, but the effect is what we're looking at).

pixelzoom commented 3 years ago

I noticed the lack of a sound for the slider track when I was implementing sound for Fourier's amplitude sliders, and patterning it on the sliders in Waves Intro / Wave Interference - which as you noted, do not support sound for the track. It wasn't clear to me how I'd even implement sound for the default slider track on the client side. Fourier has a custom slider track, so I can do anything I want there, but that doesn't address the general issue for Slider.

For discrete sliders, at the very least I would expect to hear a click as the thumb moves to its new location.

For continuous sliders, I'm not sure.

Ashton-Morris commented 3 years ago

For clicking on the track I feel that a single "middle slider sound" like the ones on the waves intro slider would be sufficient.

For continuous sounds I am not sure. My instinct is that if we had a short sample of the continuous slider sound play after clicking it would sound out of place.

pixelzoom commented 3 years ago

Raising the priority of this issue to medium. Fourier is supposed to be feature-complete by 6/30/2021. That will likely slip a few weeks, but I would expect Fourier to be ready for dev testing in mid to late July. This issue definitely blocks dev testing.

pixelzoom commented 3 years ago

This API will also need to be accessible via NumberControl, which has Slider as a subcomponent.

pixelzoom commented 3 years ago

Fourier feature-complete milestone has been revised to 8/24/21. It will then go immediately into QA, for 9/30/21 publication.

pixelzoom commented 3 years ago

What little support there is for sound in Slider doesn't seem to play nice with custom tracks. For example, see https://github.com/phetsims/fourier-making-waves/issues/179. This is probably because Slider passes options to DefaultSliderTrack, but doesn't do anything for a custom track. So... Please keep in mind that Slider needs to support custom tracks - and thumbs! And if I pass a custom thumb or track to Slider, I shouldn't have to assume responsibility for sound.

pixelzoom commented 3 years ago

Another thing to consider, which came up in https://github.com/phetsims/fourier-making-waves/issues/197... When using keyboard navigation, sounds need to play when using Page Up/Down to go to max/min.

pixelzoom commented 3 years ago

UI sounds will be added for Geometric Optics, see https://github.com/phetsims/geometric-optics/issues/236. That sim has 3 sliders (NumberControl) which are central to it's UI. They will not have sound until this issue is addressed.

pixelzoom commented 3 years ago

UI sound has been added to the requirements for the first version of Geometric Optics, see https://github.com/phetsims/geometric-optics/issues/236. Some of its most important controls are sliders.

There has also been discussion about making UI sound a standard part of sim implementation. I guess that's possible without Slider sound. But it's not going to be an optimal UX, depending on how important the sliders are in the sim. And it's going to be expensive (implementation + QA) to go back and add Slider sound to sims later.

Should the priority of this issue be raised, given the interest in UI sounds?

pixelzoom commented 2 years ago

Slider sound support came up at 11/11/21 dev meeting. @jbphet mentioned that he was adding sound to sliders in greenhouse-effect, and that may move this common-code issue forward. Let me know if you'd like me to review anything.

jbphet commented 2 years ago

It will be important to make sure that adding generalized sound doesn't mess up sound in any existing simulations, or turn on something that we don't want turned on. To that end, I'm going to try to make some lists of sims and common-code components that will need to be checked as part of this effort.

I made a list of all sims with sound currently turned out by searching for supportsSound: true in package.json files, then searched for all sims that import any variants of Slider directly, and then cross referenced the two using the command grep -Fxf temp.txt temp2.txt. This provides a list of sims whose slider behavior will need to be regression tested after implementing generalized support for sound in the sliders. Note that this is not an exhaustive list of every sim that will need regression testing, since sims could have included a slider via a common-code component.

Here is a list of the components in scenery-phet that import a slider. These will also need to be tested once generalized sound has been added:

pixelzoom commented 2 years ago

@jbphet added sound support in the above commits. @arouinfar and I have been reviewing its usage in Geometric Optics, see https://github.com/phetsims/geometric-optics/issues/259, and it's looking (and sounding!) good.

But there are some general questions that seem to belong in this issue:

pixelzoom commented 2 years ago

Here's some boilerplate that I see duplicated in all of the NumberControl instances of Geometric Optics. Presumably this will also need to be done for other NumberControl instances. 

const numberControlDefaults: OptionizeDefaults<{}, NumberControlOptions> = merge( {}, GOConstants.NUMBER_CONTROL_OPTIONS, {
  delta: GOConstants.DIAMETER_SPINNER_STEP,
  sliderOptions: {
    soundGeneratorOptions: {
      numberOfMiddleThresholds: diameterProperty.range!.getLength() / GOConstants.DIAMETER_SLIDER_STEP - 1
  }

What I see here is that numberOfMiddleThresholds is typically a function of options.delta and numberProperty.range. So how about adding this default to NumberControl?

options.sliderOptions.soundGeneratorOptions.numberOfMiddleThresholds:
  numberProperty.range!.getLength() / options.delta - 1

A default like this might result in fewer sims that don't need adjusting. The sims that I've looked at so far definitely need adjusting.

pixelzoom commented 2 years ago

I've added the blocks-publication label. Now that Sliders have sound, the sims that I've spot-checked definitely need to be reviewed (and probably adjusted) before publishing. That likely includes sims in the 4/1/2022 batch for https://github.com/orgs/phetsims/projects/44.

pixelzoom commented 2 years ago

In https://github.com/phetsims/fourier-making-waves/issues/56, I enabled Slider sound for the amplitude sliders in Fourier's Discrete screen. After setting numberOfMiddleThresholds to match the slider interval (0.05), these sliders sound a bit like an angry bumble bee -- too much sound. So that raises a couple of design questions:

pixelzoom commented 2 years ago

The soundGenerator option is strange.

I expected to turn off sound by doing:

soundGenerator: null

But here's a usage in fourier-making-waves AmplitudeSlider.js

soundGenerator: ValueChangeSoundGenerator.NO_SOUND

In ValueChangeSoundGenerator.ts:

// This is used to generate sounds as the slider is moved by the user.  If set to null, the default sound generator
// will be created.  Set to ValueChangeSoundGenerator.NO_SOUND to disable sound generation for this slider.
soundGenerator?: ValueChangeSoundGenerator | null;

Getting a non-null default when you set an option to null is very odd and non-intiuitive.

What I expected is:

If it makes your implementation cleaner to use ValueChangeSoundGenerator.NO_SOUND internally when soundGenerator: null, that's fine.

pixelzoom commented 2 years ago

@jbphet and I paired on this for awhile today. In the above commits, we addressed the soundGenerator option comments that I had made in https://github.com/phetsims/sun/issues/697#issuecomment-1061074504. The new behavior is summarized in the documentation, and is the same for Slider and SliderTrack:

  // This is used to generate sounds as the slider is moved by the user.  If not provided, the default sound generator
  // will be created. If set to null, the slider will generate no sound.
  soundGenerator?: ValueChangeSoundGenerator | null;

  // Options for the default sound generator.  These should only be provided when using the default.
  soundGeneratorOptions?: ValueChangeSoundGeneratorOptions
jbphet commented 2 years ago

In recent review discussions with various designers and developers, we decided to:

  1. Add support for asymmetric sounds (different going up versus going down)
  2. Add support for asymmetric min and max sounds
  3. The sounds going to the right and reach the max should be one musical step higher than those moving to the left and reaching the min
  4. In NumberControl, we should use the inputs to derive the number of middle threshold sounds so that the client doesn't have to do it, since this will be the most typical case
  5. I should try to add code to prevent the sounds from being played too much, and thus avoid what we were calling the "angry bumblebee effect"

Once this is done, we will review at a design meeting. @arouinfar will line up the meeting and will work with @kathy-phet to determine who should be there.

jbphet commented 2 years ago

@arouinfar and @pixelzoom - In regard to item 5 above, I tried something fairly simple for the sound generator to moderate the number of "middle" sounds that are played. The change basically just prevents the sound from being played again if it was played within the last X seconds, and I'm using 0.033 seconds as the default value. I feel like this works surprisingly well. Can you please check it out in Geometric Optics and Fourier and let me know what you think? I have some other ideas, but it would be quite nice if we can make it work with something this simple.

For reference, I added some code to see how many plays of the middle sound where played versus skipped when moving the sliders quickly in these two sims, and in Geo Opts it played a little less than half the number of tick sounds on a very fast drag, and in Fourier is played around 1/3 of them.

jbphet commented 2 years ago

@pixelzoom - In addition to the question above, could you also please review the commit for using the information available in NumberControl to set the sound generator thresholds? It seems to work well in Geometric Optics (thanks for the suggestion), but I'd like you to take a look since I don't know NumberControl that well.

pixelzoom commented 2 years ago

@jbphet CT is complaining about the sound generator thresholds, failing in several sims. For example:

fourier-making-waves : interactive-description-fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/fourier-making-waves/fourier-making-waves_en.html?continuousTest=%7B%22test%22%3A%5B%22fourier-making-waves%22%2C%22interactive-description-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1646964676568%22%2C%22timestamp%22%3A1646968422080%7D&brand=phet&ea&fuzz&supportsInteractiveDescription=true&memoryLimit=1000
Query: brand=phet&ea&fuzz&supportsInteractiveDescription=true&memoryLimit=1000
Uncaught Error: Assertion failed: not enough unique thresholds were produced - is the constraint function too constraining?
Error: Assertion failed: not enough unique thresholds were produced - is the constraint function too constraining?
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/assert/js/assert.js:25:13)
at new ValueChangeSoundGenerator (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/tambo/js/sound-generators/ValueChangeSoundGenerator.js:84:15)
at new Slider (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/sun/js/Slider.js:107:32)
at new HSlider (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/sun/js/HSlider.js:18:5)
at new NumberControl (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/scenery-phet/js/NumberControl.js:221:19)
at new WavePacketNumberControl (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/fourier-making-waves/js/wavepacket/view/WavePacketNumberControl.js:35:5)
at new ConjugateStandardDeviationControl (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/fourier-making-waves/js/wavepacket/view/ConjugateStandardDeviationControl.js:67:5)
at new WavePacketWidthSubpanel (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js:271:47)
at new WavePacketControlPanel (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/fourier-making-waves/js/wavepacket/view/WavePacketControlPanel.js:67:5)
at new WavePacketScreenView (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1646964676568/chipper/dist/js/fourier-making-waves/js/wavepacket/view/WavePacketScreenView.js:154:26)
id: Bayes Chrome
Snapshot from 3/10/2022, 7:11:16 PM
jbphet commented 2 years ago

The CT issue is related to constraining values, reaching & crossing thresholds, and floating point inaccuracies as related to NumberControl. This may take me a while to get dialed in, so I've commented out the assertion for now.

arouinfar commented 2 years ago

@jbphet the slider sounds great! I tested in geometric-optics, fourier-making-waves, and waves-intro in master. I really like the directionality and min/max behavior. The threshold of 0.033 seconds seems reasonable to me. I still hear individual clicks when holding down an arrow key or moving the mouse slowly, but the angry bumblebee is nowhere to be found when rapidly dragging the thumb around.

jbphet commented 2 years ago

The CT issue described above was due to something I hadn't anticipated. Because NumberControl uses a delta value option to specify valid values, it is possible to create a situation where the positions where sounds should be played are not spaced evenly over the length of the slider. I thought about changing the sound generator to have the same API, i.e. use a delta value instead of a number of thresholds, but that felt unnatural and potentially error prone to me. In the end, I decided to allow either the number of thresholds or a delta value to be used to specify where along the slider's range the sounds should be played.. The NumberControl is using the delta option for its sound generator options, all other places currently in the code base are specifying the number of thresholds.

I added an instance of NumberControl to tambo that exhibits this asymmetry as a test case. It has some odd behavior that is unrelated to sound generation, so I'll follow up on that. The main point is that there is a demo and test case for the asymmetric case that can be easily run.

As part of the effort to get this working correctly in all the test cases, I had to expand the use of thresholding in ValueChangeSoundGenerator. I'll revisit this in the future, since I got it working but probably should think it through a bit more.

pixelzoom commented 2 years ago

Sliders sound great in Fourier and Geometric Optics -- no more angry bees.

https://github.com/phetsims/scenery-phet/commit/396ea26e30510c0f655c8fd9ba905cfd5739c1b8 looks good. But Slider has option constrainValue (singular), and you've added soundGeneratorOptions.constrainValues (plural). That's going to result in programming errors. Any reason why they can't have the same name?

Related to soundGeneratorOptions.constrainValues.... SoundGeneratorOptions has no such option. It took me some poking around to determine that soundGeneratorOptions is actually of type ValueChangeSoundGeneratorOptions | null. The names of nested options should generally match the fully name of the type that their associated type. So please rename soundGeneratorOptions to valueSoundGeneratorOptions -- in Slider.ts, NumberControl.ts, and elsewhere if relevant.

jbphet commented 2 years ago

Both of @pixelzoom's suggestions in the comment above sounded good to me, so I've implemented them.

jbphet commented 2 years ago

The behavior of this feature as it currently stands was reviewed in a design meeting yesterday, 3/17/2022. For the most part, the design team was good with it. There were two questions that came up, and it was recommended that I discuss these with @terracoda. They are:

I'll set up a discussion and note the outcome.

jbphet commented 2 years ago

@terracoda and I just met over Zoom, and her opinion is that it is not necessary to have unique sounds for the arrow versus shift-arrow keys, nor does there need to be a difference for discrete versus continuous movement. We both felt like if there were cases where the amount of motion mattered, we'd use the pitch-mapping capability to vary the sound that was produced.

She did suggest that I run it by @emily-phet, so I'll do that in the next sound design meeting, which will be Tuesday 3/22/2022.

jbphet commented 2 years ago

This is now in a state where I believe it's ready for code review. @pixelzoom said he'd do it (thanks!), so I'll add the appropriate label and assign to him.

My general approach for this was to create a sound generator that had methods that could be used to evaluate changes to a value and decide which if any sounds to play, then pass this sound generator into Slider, and have slider hook it up in the appropriate places so that sounds are produced on user-initiated changes to the value. The sound generator does not monitor an axon Property, because it's often difficult to discern when changes to the property value were triggered by some direct user action (such as moving the slider) versus some indirect user action (such as pressing the reset button). So, with that for background, the review should probably focus on:

pixelzoom commented 2 years ago

I reviewed everything that @jbphet mentioned in https://github.com/phetsims/sun/issues/697#issuecomment-1072811918, and took a peek at NumberControl.ts too. Useful (and fun) tests in SliderSoundTestNode.ts. Everything looks really nice, very clean, excellent documentation. Thanks for doing this.

Closing!

jbphet commented 2 years ago

In one of the comments above, I said:

[@terracoda] did suggest that I run it by @emily-phet, so I'll do that in the next sound design meeting[.]

I wanted to close the loop on this, so we discussed it in today's sound design meeting, and @emily-phet is fine with all the movement sounds being the same with no special sounds for arrow keys or discrete versus continuous.

jbphet commented 2 years ago

Reopening. A question arose in my mind about whether the sounds produced by a slider would stop being produced if the output level for the "user-interface" category was set to zero. My hope was that it would, but I couldn't recall specifically implementing code to do so. I tested it, and it worked, but when I dug into it, I realized that it wasn't working for the right reasons. It works because ValueChangeSoundGenerator is using SoundClipPlayer instances for its sounds, and the instances that it is using are all individually set up to be in the "user-interface" category. However, ValueChangeSoundGenerator extends SoundGenerator, so in principle, it should be added to the soundManager and connected to the audio path, but it's not. This seems messed up.

So, at this juncture, this feature works correctly, but will likely be confusing to future users of ValueChangeSoundGenerator and even more confusing to anyone who needs to maintain it. I should either make it explicit that ValueChangedSoundGenerator is not a sound generator itself, and is instead a container for a bunch of SoundClipPlayer instances (and should change its name), or I should make it into a real sound generator. My inclination is for the latter, but I'd like to think on it for a bit. Using SoundClipPlayer instances reduces the memory resources that are used and keeps everything at consistent output levels, but I don't think the resource difference is very large versus using plain SoundClips, and it would be easier for people to follow if separate sound clips were used. Plus, it would be easier to set different output levels for different instances of ValueChangedSoundGenerator, which may well be something that we want to do, and wouldn't work at all with the current implementation.

jbphet commented 2 years ago

I've addressed the concern that I raised in the previous comment by changing ValueChangedSoundGenerator to ValueChangedSoundPlayer and make it not be a sound generator. I opted for this approach because if it was a sound generator, the client would have to pass in sound generators with connect methods, and not hook them up to anything, and wouldn't be able to use the shared sound players. This seemed worse.

I think this is done (again), closing.