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

Sound for sliders #56

Open
pixelzoom opened this issue Apr 16, 2021 · 18 comments
Open

Sound for sliders #56

pixelzoom opened this issue Apr 16, 2021 · 18 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 16, 2021

This sim has the following sliders:

  • Discrete: custom amplitude sliders
  • Discrete: standard volume slider for Fourier series sound
  • Wave Pack: 4 standard sliders in the control panel

From #54

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

@pixelzoom pixelzoom self-assigned this Apr 16, 2021
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 16, 2021

Waves Intro adds sounds in WaveInterferenceSlider.js:

import generalBoundaryBoopSoundPlayer from '../../../../tambo/js/shared-sound-players/generalBoundaryBoopSoundPlayer.js';
import generalSoftClickSoundPlayer from '../../../../tambo/js/shared-sound-players/generalSoftClickSoundPlayer.js';

Playing these sounds must be handled in the Slider's options.drag function.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 16, 2021

Below is the code that plays sounds in WaveInterferenceSlider.js.

@jbphet @samreid is this really the recommended pattern? Is this the level of support that's available for Slider sound? And PDOM events have to be handled separately? And the client is responsible for checking elapsed time between sounds?!?

      drag: event => {

        const value = property.value;

        if ( event.isFromPDOM() ) {

          if ( Math.abs( value - property.range.max ) <= TOLERANCE ||
               Math.abs( value - property.range.min ) <= TOLERANCE ) {
            generalBoundaryBoopSoundPlayer.play();
          }
          else {
            generalSoftClickSoundPlayer.play();
          }
        }
        else {

          // handle the sound as desired for mouse/touch style input
          for ( let i = 0; i < ticks.length; i++ ) {
            const tick = ticks[ i ];
            if ( lastValue !== value && ( value === property.range.min || value === property.range.max ) ) {
              generalBoundaryBoopSoundPlayer.play();
              break;
            }
            else if ( lastValue < tick.value && value >= tick.value || lastValue > tick.value && value <= tick.value ) {
              if ( phet.joist.elapsedTime - timeOfLastClick >= MIN_INTER_CLICK_TIME ) {
                generalSoftClickSoundPlayer.play();
                timeOfLastClick = phet.joist.elapsedTime;
              }
              break;
            }
          }
        }

@samreid
Copy link
Member

samreid commented Apr 16, 2021

That pattern was recommended when we worked on it in mid-2020. @jbphet is there better common code support for this now? Should there be?

@samreid samreid removed their assignment Apr 16, 2021
pixelzoom added a commit that referenced this issue Apr 22, 2021
pixelzoom added a commit that referenced this issue Apr 22, 2021
@pixelzoom
Copy link
Contributor Author

From 4/22/21 design meeting

There is currently no sound API for Slider, because most sounds were pedagogical.

Now that some sims have been instrumented, there are probably some common patterns for sound in Sliders. @jbphet will identify those in phetsims/sun#697 and develop a sound API for Slider, including some type of default non-pedagogical sound, like WaveInterfaceSlider.

In the meantime, I've added rudimentary sound to the amplitude sliders in Fourier. I did that totally in AudibleSlider, a Slider subclass. It's a bit like the approach used in WaveInterfaceSlider, and I'm hoping that AudibleSlider can be entirely deleted once phetsims/sun#697 is completed.

Work on this is on hold until the Slider sound API is ready.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 14, 2021

Common-code work for Slider sound is being done by @jbphet in phetsims/sun#697.

Fourier currently has a sim-specific implementation of Slider sound for Amplitude sliders, with behavior identical to Waves Intro. As the thumb is moved (with mouse, touch, or keyboard), there are discrete "tick" sounds. At the min/max there is a specific sound. And there is no sound when the value is changed by clicking on the track (a deficiency in the Waves Intro implementation, not easily correct in Fourier). You can test-drive this is master or any recent dev version.

To help prioritize our work, it would be helpful to know whether the sim-specific implementation is sufficient for Fourer 1.0, or whether the common-code work needs to be completed (and is therefore blocking).

@arouinfar @kathy-phet your thoughts?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 6, 2021

Raising priority to get feedback from @arouinfar and @kathy-phet. Please re-read the previous comment. Slider sound API is a large task, so if this is needed, that work needs to start very soon.

@pixelzoom pixelzoom changed the title Add sound to amplitude sliders Add sound to sliders Jul 12, 2021
@pixelzoom pixelzoom changed the title Add sound to sliders Sound for sliders Jul 12, 2021
@arouinfar
Copy link
Contributor

A sim-specific approach seems reasonable to me. Perhaps we can defer the common code work until we get to a 3rd sim that wants to use the slider clicks. I know it's unconventional to wait so long to generalize, but given current priorities, it seems okay. I can't make that call, however, so leaving it up to @kathy-phet.

@arouinfar arouinfar removed their assignment Jul 12, 2021
@kathy-phet
Copy link

Sim-specific is OK, so this shouldn't be blocking for this sim publication. But I will add as a line item in PhET overview, to keep it on the radar for a future quarterly goal.

@arouinfar
Copy link
Contributor

Discussed in the 7/15/21 design meeting with @kathy-phet @pixelzoom

(3) is the preferred option, but time may not permit.
(1) is our fallback option.

@pixelzoom
Copy link
Contributor Author

More notes from 7/15/21:

@kathy-phet will check in with @jbphet to see whether phetsims/sun#697 and option (3) is doable in the next month. Feature-complete milestone is 8/24/21, with the goal of starting QA testing shortly after.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 29, 2021

@kathy-phet gave an update at Q3 planning meeting today. She discussed with @jbphet and @arouinfar. phetsims/sun#697 is probably Q4 work, after @jbphet has published Greenhouse. So we're going with this option for Fourier 1.0:

(1) Sim-specific sound for Amplitude sliders, no sound for other sliders.

So there's no additional work to do right now, and this issue is deferred.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 7, 2022

@jbphet recently added sound support for Slider. But in fa33958 for phetsims/sun#697, he disabled that support in AmplitudeSlider.js:

soundGenerator: ValueChangeSoundGenerator.NO_SOUND,

What really needs to be done is to delete the ad hoc sound from AmplitudeSlider.js (SoundDragHandler) and enable the Slider sound.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 7, 2022

In the above commit, I replaced ad hoc sound (SoundDragHandler) with Slider sound support. A sound in now generated on each value change, in 0.05 interval. This results a LOT of sound when dragging the slider, kind of like an angry bumble bee. So we'll need to discuss what (in general) should we do when there are a lot of value changes? Is it OK to have no sound for some values? I guess this would have to be the case if we're going to use discrete sounds for contiuous sliders.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 7, 2022

Now that Sliders have sound by default, all Sliders (and NumberControls) in this sim will need to be reviewed.

In particular, I think that it makes sense to turn sound off for the slider that controls the output level of the Fourier Series (see screenshot below). Since this slider is itself controlling sound, it's distracting/confusing to have it make sound. @arouinfar do you agree?

screenshot_1590

@pixelzoom
Copy link
Contributor Author

@arouinfar agrees that the slider that controls the output level of the Fourier Series should not have sound enabled.

@pixelzoom
Copy link
Contributor Author

Labeling as blocks-sim-publication because all sliders need to be reviewed before publishing from master again.

Self unassigning until we work on this sim again.

@pixelzoom pixelzoom self-assigned this Apr 1, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 5, 2022

phetsims/sun#697 was completed, so Sliders in this sim now have sound. This needs to be reviewed when work on this sim resumes. It's on-hold and unassigned until then.

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