SoundGenerator doesn't set gain unless fullyEnabledProperty is true

[jessegreenberg]

I have a SoundGenerator with two enableControlProperties. When one of them is false, I am still hearing sound from the SoundGenerator. I am not sure if that is expected, but I tried to set the output level directly and found that it is a noop in this case.

Around here: https://github.com/phetsims/tambo/blob/05c0c7b8f1188f18fcca0747066e212a137beb05/js/sound-generators/SoundGenerator.ts#L244-L268

If the timeConstant is non-zero or the fullyEnabledProperty is false masterGainNode.gain is not set. I am wondering if the elseif should change to this?

Index: js/sound-generators/SoundGenerator.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sound-generators/SoundGenerator.ts b/js/sound-generators/SoundGenerator.ts
--- a/js/sound-generators/SoundGenerator.ts (revision 5fa44dd369d0e620cf872d4438cae456cdb81ade)
+++ b/js/sound-generators/SoundGenerator.ts (date 1665520820548)
@@ -251,7 +251,7 @@
         this.masterGainNode.gain.setValueAtTime( outputLevel, now );
       }
     }
-    else if ( this.fullyEnabledProperty.value ) {
+    else if ( outputLevel === 0 || this.fullyEnabledProperty.value ) {

       // The setTargetAtTime method doesn't seem to work if the audio context isn't running, and the event doesn't seem
       // to be scheduled - it's just ignored.  So, if the audio context isn't running, use an alternative approach.  See

Or maybe the issue is happening earlier than this and is more about having a non-zero outputLevel at all when there is a false value in enableControlProperties. @jbphet what do you think?

[jbphet]

Above, @jessegreenberg said:

I have a SoundGenerator with two enableControlProperties. When one of them is false, I am still hearing sound from the SoundGenerator. I am not sure if that is expected[.]

This is definitely not expected. The volume should go to zero when any of the enableControlProperties transition to false. Can you tell me how to duplicate the problem? I'll try it and see why it seems to be behaving incorrectly.

@jessegreenberg also said:

I tried to set the output level directly and found that it is a noop in this case.

That's intentional. The output level can be set via the methods in a case where the sound is disabled, but the change doesn't take effect immediately because the gain level of the master gain stage is turned down to zero (at least it's supposed to be) because the sound is disabled.

So, as I said above, I think the best thing is to show me how to duplicate the behavior you're seeing and I'll investigate.

[jessegreenberg]

Sounds good. I am seeing this in Quadrilateral when shape sound tracks are fading in or fading out. The way to see the behavior is to 1) Comment out the workaround added to TracksSoundView in https://github.com/phetsims/quadrilateral/commit/6df21623e869dca8a2da15429ab2b21b9bc90fff. 2) Add a console.log inside of this else/if case so you can watch when the sound tracks are fading out. https://github.com/phetsims/quadrilateral/blob/6df21623e869dca8a2da15429ab2b21b9bc90fff/js/quadrilateral/view/sound/TracksSoundView.ts#L190 3) Load the sim and open the dev tools. Change the quadrilateral shape to play sounds. While the tracks are "fading out" press the checkbox with the music icon. That will set one of the enableControlProperties to false. 4) Sound will continue to play forever, even though the SoundGenerator.fullyEnabledProperty.value is false.

TracksSoundView is a SoundGenerator that controls the output for many sound clips which are set up here: https://github.com/phetsims/quadrilateral/blob/6df21623e869dca8a2da15429ab2b21b9bc90fff/js/quadrilateral/view/sound/TracksSoundView.ts#L87-L106

I hope that is clear but let me know if it would be easiest to pair sometime to demonstrate!

[jbphet]

Nice bug you found here @jessegreenberg! Here's the story: When making gain changes on a Web Audio gain node, it's often important to cancel previously scheduled changes. The code in SoundGenerator.setOutputLevel was doing this, but it was doing it always, regardless of whether the sound generator was fully enabled. The code in Quadrilateral was changing the value of an enable control Property and then immediately setting the output level to zero via setOutputLevel. This was causing the gain change that was scheduled when the enable control Property transitioned to false to be canceled, leaving the gain essentially unchanged.

I've modified setOutputLevel to only cancel previously schedule gain changes when the sound generator is fully enabled. That seems to fix the bad behavior.

@jessegreenberg - Now that this is changed, you probably want to test and make sure the sounds are behaving the way you want them to. I noticed that if I changed the shape while the music was enabled and then immediately disabled and re-enabled it, it would stop entirely, but if I first disabled the music, then moved the shape, then immediately enabled the music, the sound did play, which seems inconsistent.

[jessegreenberg]

The workaround code in quadrilateral has been removed and sounds have been working well for some time. Thanks for fixing this @jbphet! Closing.