Staccato success sound toggles volume on/off without stopping

[zepumph]

In the StaccatoFrequencySoundGenerator, we have this chunk of code in the step function:

    // if we were in ratio, but now we are not, then fade out the successSoundClip
    if ( this.isInSuccessfulRatio( this.oldFitness ) && !isInRatio ) {

      // TODO: is there a way to get a notification when this is done ramping down? #9
      this.successSoundClip.setOutputLevel( 0, .1 );
      this.playedSuccessYet = false;
    }

basically, if we were in proportion, but then we were not, then fade the success sound away. The issue here has to do with how the staccato sound is quite long. Long enough that it can fade away completely, still silently be playing reverby decay, and then have the user interact with the ratio and play the success sound again.

Steps to reproduce:

• Ratio and Proportion sim
• drag a hand around until you make the success sound
• immediately let go
• then just click it again to activate it again and the reverby ending of the success sound will still be playing.

I want to be able to add something like this to the step function, but not checking the outputLevel, which is updated synchronously, but instead checking on the actual gain of the soundGenerator as it dampens.

    if ( this.successSoundClip.isPlaying && !this.fullyEnabled && this.successSoundClip.outputLevel === 0 ) {
      this.successSoundClip.stop();
    }

An alternative solution would be to get a notification when setOutputLevel has completed its time transition.

@jbphet and I discussed this for 20 seconds during the meeting today, but I thought it would be worth an issue, because likely, before too long I hope, we will begin cleaning up the sound view and working on smaller tweaks like this.

@jbphet can you reproduce? Can you recommend a solution?

[jbphet]

I have a number of thoughts on this. First off, I don't think the staccato sounds should be fading out. It sounds quite unnatural to me, and I suspect that others will hear it the same way. If a user was using a metal detector, or a Geiger counter, or sonar in a submarine - all examples of things that use periodic sounds to indicate something - the individual sounds are always allowed to play out. I feel that it should be the same here. If you want the ability to fade out the success sound, which may be desirable (though I think one could make the same argument that it shouldn't be faded), I would suggest having it in its own separate sound generator.

As for checking the value of the output level, this may be problematic due to differences in Web Audio between browsers. I haven't tested this for a while, but a couple of years ago when I was trying to use this, some browsers supported reading of actual gain values and some did not. I don't know whether this has changed, but I would suggest avoiding it for now. If you feel it's crucial to what is needed for this sim, we can take some time and investigate whether this is more consistent these days.

As for getting a notification for when a gain change has finished, I think I may have misspoke when I talked about it. There is an event notification for when a sound finishes playing that is built-in to Web Audio in the form of AudioScheduledSourceNode.onended, but I don't think there is an event system for notifications for things like gain changes.

So, given the constraints in the 2nd two paragraphs above, I would recommend starting with the suggestions in the first one.

[zepumph]

I have a number of thoughts on this. First off, I don't think the staccato sounds should be fading out. It sounds quite unnatural to me, and I suspect that others will hear it the same way. If a user was using a metal detector, or a Geiger counter, or sonar in a submarine - all examples of things that use periodic sounds to indicate something - the individual sounds are always allowed to play out. I feel that it should be the same here. If you want the ability to fade out the success sound, which may be desirable (though I think one could make the same argument that it shouldn't be faded), I would suggest having it in its own separate sound generator.

I am only discussing the success sound. Not the individual notes that get more frequent as the ratio get's more correct. I have a separate sound generator for the success sound that the fading logic applies to.

Thanks for the feedback on library-level support. I see two paths forward: decide that fading isn't needed, or build out a simple timer in the soundGenerator that can execute independently after the same amount of time passes as is passed to setOutputLevel. I will think of this issue on-hold until we solidify the sound design further and move to more of a "fine tuning" phase.

[zepumph]

I added two potential TODOs tagged to this issue.

[zepumph]

Here is a patch that listens to when the ratio is no longer being interacted with, and tries to stop the success sound so that it can start again the next time to click on it. It doesn't yet work though because it seems like there is a race condition between the isBeingInteractedWithProperty and the step function, and it seems like there is one more frame after the logic in the Property listener where the success sound fires again and then stops. I think the way to solve this is to do everything in the step function.

```diff Index: js/common/view/sound/StaccatoFrequencySoundGenerator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/common/view/sound/StaccatoFrequencySoundGenerator.js (revision f77ccded35362273049e912db0f82da338b6a86c) +++ js/common/view/sound/StaccatoFrequencySoundGenerator.js (date 1592350109176) @@ -42,9 +42,10 @@ /** * @param {Property.} fitnessProperty * @param {Range} fitnessRange + * @param {Property.} isBeingInteractedWithProperty * @param {Object} [options] */ - constructor( fitnessProperty, fitnessRange, options ) { + constructor( fitnessProperty, fitnessRange, isBeingInteractedWithProperty, options ) { options = merge( { initialOutputLevel: 0.4 }, options ); @@ -89,6 +90,13 @@ // @private - keep track of if the success sound has already played. This will be set back to false when the fitness // goes back out of range for the success sound. this.playedSuccessYet = false; + + isBeingInteractedWithProperty.lazyLink( ( isBeingInteractedWith, wasBeingInteractedWith ) => { + if ( wasBeingInteractedWith && !isBeingInteractedWith ) { + this.successSoundClip.stop(); + this.playedSuccessYet = false; + } + } ); } /** Index: js/common/view/sound/ProportionFitnessSoundGenerator.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/common/view/sound/ProportionFitnessSoundGenerator.js (revision f77ccded35362273049e912db0f82da338b6a86c) +++ js/common/view/sound/ProportionFitnessSoundGenerator.js (date 1592349540664) @@ -150,14 +150,17 @@ ////////////////////////////////////////////////////////////////// // staccato - this.staccatoFrequencySoundGenerator = new StaccatoFrequencySoundGenerator( proportionFitnessProperty, fitnessRange, { - enableControlProperties: [ - isBeingInteractedWithProperty, - fitnessNotMinProperty, - new DerivedProperty( [ designingProperties.proportionFitnessSoundSelectorProperty ], - value => value === 5 || value === 6 || value === 7 ) - ] - } ); + this.staccatoFrequencySoundGenerator = new StaccatoFrequencySoundGenerator( + proportionFitnessProperty, + fitnessRange, + isBeingInteractedWithProperty, { + enableControlProperties: [ + isBeingInteractedWithProperty, + fitnessNotMinProperty, + new DerivedProperty( [ designingProperties.proportionFitnessSoundSelectorProperty ], + value => value === 5 || value === 6 || value === 7 ) + ] + } ); this.staccatoFrequencySoundGenerator.connect( this.soundSourceDestination ); ////////////////////////////////////////////////////////////////// ```

[zepumph]

This code is now in a file called InProportionSoundGenerator.js

[zepumph]

I just had an idea, why not just stop the previous sound before playing the next one. It will stop on a delay so it shouldn't be abrupt or anything. But then when I add this patch, I can't hear a difference at all.

Index: js/common/view/sound/InProportionSoundGenerator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/sound/InProportionSoundGenerator.js  (revision 78cf62e7844c9d9d0ac71d27c7d2496c3b2f8e07)
+++ js/common/view/sound/InProportionSoundGenerator.js  (date 1598906413333)
@@ -73,6 +73,8 @@
     const hysteresisThreshold = this.model.movingInDirection() ? RatioAndProportionQueryParameters.hysteresisThreshold : 0;

     if ( !this.playedSuccessYet && ( isInRatio || currentRatioIsLargerThanTarget !== this.currentRatioWasLargerThanTarget ) ) {
+      this.successSoundClip.stop();
+
       this.successSoundClip.setOutputLevel( SUCCESS_OUTPUT_LEVEL, 0 );
       this.successSoundClip.play();
       this.playedSuccessYet = true;

I can't really think of a problem here with this issue. Everything with the sound's design is correct, and there have been no more issues. The main fix that occurred in this space but not in this issue was making sure that the previous ring wasn't still playing the next time that you clicked on the hand when you were still in the proportion. This has been solved with this line of code (not exactly what the file looks like):

    // if we were in ratio, but now we are not, then fade out the successSoundClip
    if ( !isInRatio && this.successSoundClip.outputLevel !== 0) {

      this.successSoundClip.setOutputLevel( 0, .1 );
    }

[zepumph]

Oh! I figured it out. When you are not fully enabled, the sound clip will stop (not just mute). Thus when no longer interacting with a hand, it stops itself. My best guess as to why this wasn't happening before was because it was embedded in the same class as the staccato sounds, or perhaps we were recreating SoundClips too often because of different sound options logic.

UPDATE: This is not it, see below.

[zepumph]

After looking into https://github.com/phetsims/tambo/issues/120 with @jbphet, we decided that the best way to solve this issue is to use inheritance instead of composition so that the fullyEnabledProperties are set to the SoundClip.

[zepumph]

We now no longer get the tail of the previous success sound when beginning a new interaction in success conditions. I'm going to close this issue, as I don't think there is anything else to discuss, and this was a minor bug to begin with.