Clipping in the middle of reset all sound when setting PhET-iO state on reset

[zepumph]

Originally this was reported in phetsims/scenery-phet#724 (comment):

I wanted to touch base about one more item I noticed while testing this. It seems like there is a bit of clipping occurring in the middle of the resetAllButton sound. I presume that it is because enabledControlProperties are turning sounds off, but the shutoff switch is set on before any changes that would cause sounds occur, so I don't quite understand why this is like this, or how we'd fix it. Perhaps it is because of ramping down to muting items. I'll investigate a bit.

I'm a bit out of my depths on this one. I may need to solicit @jbphet for some assistance. If nothing else, he can let me know how serious the problem is. @jbphet will you please reach out when you are back and we can schedule even a short 15 minute meeting to discuss. Thanks!

After a 20 minute discussion with @jbphet, it seemed possible that this could be challenging to solve, and potentially not worth it. So I will create another issue to track progress.

[zepumph]

To reproduce:

• Launch the State wrapper for any sim with sound enabled (reset sound is in common code).
• Set the State Rate equal to zero after 2 seconds.
• Press the reset-all button in the downstream sim, hear clipping, especially when pressing quickly.

We were able to consistently get the clip on chrome, even pressing the reset all button once every 5 seconds. We only got the clipping in Firefox when spamming the reset all button repeatedly.

[zepumph]

Investigation:

We noticed that quite of bit of work is done if a sound isn't playing it its fullyEnabledProperty changes to false:

tambo/js/sound-generators/SoundGenerator.js

Lines 133 to 149 in ddff764

	// turn down the gain to zero when not fully enabled, or up to the current output level when becoming fully enabled
	this.fullyEnabledProperty.link( fullyEnabled => {
	const previousGainSetting = fullyEnabled ? 0 : this._outputLevel;
	const newGainSetting = fullyEnabled ? this._outputLevel : 0;
	const now = this.audioContext.currentTime;
	// For the linear ramp to work consistently on all browsers, the gain must be explicitly set to what it is
	// supposed to be before making any changes. Otherwise, it may extrapolate from the most recent previous event.
	this.masterGainNode.gain.setValueAtTime( previousGainSetting, now );
	// ramp the gain to the new level
	this.masterGainNode.gain.linearRampToValueAtTime(
	newGainSetting,
	this.audioContext.currentTime + soundConstants.DEFAULT_LINEAR_GAIN_CHANGE_TIME
	);
	} );

tambo/js/sound-generators/SoundClip.js

Lines 133 to 137 in 6e3d6d8

	this.fullyEnabledProperty.lazyLink( fullyEnabled => {
	if ( !this.loop && !fullyEnabled ) {
	this.stop();
	}
	} );

So this patch had two possible solutions, and neither seemed to help much. One, don't call "stop" unless you are playing, and two immediately silence sounds if setting PhET-iO state when no longer fullyEnabled.

Details

Index: js/sound-generators/SoundGenerator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sound-generators/SoundGenerator.js b/js/sound-generators/SoundGenerator.js
--- a/js/sound-generators/SoundGenerator.js	(revision ddff76443c2404104c955cd54b52c587d08d74dc)
+++ b/js/sound-generators/SoundGenerator.js	(date 1646353635877)
@@ -141,11 +141,12 @@
       // supposed to be before making any changes.  Otherwise, it may extrapolate from the most recent previous event.
       this.masterGainNode.gain.setValueAtTime( previousGainSetting, now );
 
+      // If setting PhET-iO state, immediately mute it.
+      const timeToNewGain = notSettingPhetioStateProperty && !notSettingPhetioStateProperty.value ? 0 :
+                            this.audioContext.currentTime + soundConstants.DEFAULT_LINEAR_GAIN_CHANGE_TIME;
+
       // ramp the gain to the new level
-      this.masterGainNode.gain.linearRampToValueAtTime(
-        newGainSetting,
-        this.audioContext.currentTime + soundConstants.DEFAULT_LINEAR_GAIN_CHANGE_TIME
-      );
+      this.masterGainNode.gain.linearRampToValueAtTime( newGainSetting, timeToNewGain );
     } );
 
     // @protected {AudioNode} - The audio node to which the sound sources will connect, analogous to
Index: js/sound-generators/SoundClip.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sound-generators/SoundClip.js b/js/sound-generators/SoundClip.js
--- a/js/sound-generators/SoundClip.js	(revision ddff76443c2404104c955cd54b52c587d08d74dc)
+++ b/js/sound-generators/SoundClip.js	(date 1646353661293)
@@ -131,7 +131,7 @@
 
     // listen to the Property that indicates whether we are fully enabled and stop one-shot sounds when it goes false
     this.fullyEnabledProperty.lazyLink( fullyEnabled => {
-      if ( !this.loop && !fullyEnabled ) {
+      if ( !this.loop && !fullyEnabled && this.isPlaying ) {
         this.stop();
       }
     } );

---

The primary thought at this point is that setting every single sound to silent while setting PhET-iO state is overloading the audio thread, and it is a bit of a performance issue. I don't think there is an easy fix for this, but @jbphet felt like he would need a few hours to know for sure.

We are at the point where we want to ask higher ups if this is even worth the hassle. It doesn't really seem like it to me, especially given our other PhET-iO priorities. @kathy-phet, would you like us to spend more time on this issue at this time?

[jbphet]

This has come up again due to phetsims/mean-share-and-balance#353, so I've done a bit more investigation. I don't think the patches proposed above really address the problem, at least not as it exists at the time of this writing.

After adding some debug code and trying a few experiments, I think the problem is that the reset button is being pressed, then the sound is started, then the listener for the reset button sets isSettingPhetioStateProperty.value to true and restores the initial phet-io state, which (intentionally) disables sound production, and then isSettingPhetioStateProperty.value is set to false, and we hear the rest of the Reset All sound.

One possible way to fix this would be to detect the situation where the pressing of the Reset All button is going to restore phet-io state and delay the playing of the sound until that completes. I'll investigate this a bit, but again, I'm not sure it is worth it.

[jbphet]

I'll continue to investigate this a little more, but there's a lingering question that should be addressed. About 2.5 years ago @zepumph asked @kathy-phet:

[W]ould you like us to spend more time on this issue at this time?

He's basically saying, "This isn't easy to solve. Should we just live with the fact that the Reset All sound is a bit wonky in sims that are launched from Studio?". Since @kathy-phet hasn't been able to answer that, I'm going to assign @brent-phet instead, since he is now the product manager. He can ping me if he'd like to meet to hear the differences and come to a resolution on this.