Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slider API for sound #697

Closed
pixelzoom opened this issue Apr 22, 2021 · 40 comments
Closed

Slider API for sound #697

pixelzoom opened this issue Apr 22, 2021 · 40 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 22, 2021

From phetsims/fourier-making-waves#54 (comment):

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
Copy link
Contributor Author

pixelzoom commented Apr 22, 2021

Required for Fourier, see phetsims/fourier-making-waves#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
Copy link
Contributor Author

See phetsims/wave-interference#523 for some question about WaveInterferenceSlider.js that might inform the Slider sound API.

@Ashton-Morris
Copy link

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
Copy link
Contributor Author

pixelzoom commented Apr 27, 2021

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

Indeed. From phetsims/fourier-making-waves#54 (comment):

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
Copy link
Contributor

jbphet commented Apr 27, 2021

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

  • We should consider support support for discrete and continuous modes, but it may turn out that just having a discrete mode by default is sufficient.
  • In the discrete mode, if tick marks are specified, we should play sounds that match them, i.e. are played each time a tick mark is crossed.
  • Use the sounds currently used for Wave Interference.
  • The clicks should be the default.
  • The continuous mode should have something pretty generic sounding, since we often use continuous sounds that change pitch for pedagogical purposes, and we don't want this to be too overtly similar to such sounds. @Ashton-Morris suggested trying the noise generator used in JT's foot drag sound generator in conjunction with a filter.

@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor

jbphet commented Apr 27, 2021

@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
Copy link
Contributor Author

pixelzoom commented Apr 28, 2021

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
Copy link

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
Copy link
Contributor Author

pixelzoom commented Jun 10, 2021

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
Copy link
Contributor Author

pixelzoom commented Jul 7, 2021

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

@pixelzoom
Copy link
Contributor Author

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

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 27, 2021

What little support there is for sound in Slider doesn't seem to play nice with custom tracks. For example, see phetsims/fourier-making-waves#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
Copy link
Contributor Author

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

@pixelzoom
Copy link
Contributor Author

UI sounds will be added for Geometric Optics, see phetsims/geometric-optics#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
Copy link
Contributor Author

pixelzoom commented Oct 25, 2021

UI sound has been added to the requirements for the first version of Geometric Optics, see phetsims/geometric-optics#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
Copy link
Contributor Author

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
Copy link
Contributor

jbphet commented Mar 16, 2022

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

@jbphet
Copy link
Contributor

jbphet commented Mar 18, 2022

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:

  • Should there be a different sound for the motion produced with arrow keys and shift-arrow keys?
  • Should there be a different sound for discrete versus continuous movement?

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

@jbphet
Copy link
Contributor

jbphet commented Mar 18, 2022

@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 added a commit to phetsims/pendulum-lab that referenced this issue Mar 18, 2022
jbphet added a commit to phetsims/tambo that referenced this issue Mar 18, 2022
jbphet added a commit to phetsims/tambo that referenced this issue Mar 18, 2022
jbphet added a commit to phetsims/tambo that referenced this issue Mar 18, 2022
@jbphet
Copy link
Contributor

jbphet commented Mar 18, 2022

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:

  • All of ValueChangeSoundGenerator.ts, which is new and was created as a part of this effort
  • The changes made to Slider.ts, mostly in the options and drag handler
  • The test/demo capabilities added to the tambo demo. Most of the code for this is in SliderSoundTestNode.ts, and it would be good to look at the demo too and see if it seems like it covers most of the important bases

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 23, 2022

I reviewed everything that @jbphet mentioned in #697 (comment), 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
Copy link
Contributor

jbphet commented Apr 5, 2022

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
Copy link
Contributor

jbphet commented Apr 15, 2022

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
Copy link
Contributor

jbphet commented May 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants