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.

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.

[kathy-phet]

@jbphet @brent-phet @zepumph - I don't think we should spend any more resources on this issue. The value add is low and sounds like the cost to fix would be high.

[kathy-phet]

I labeled this deferred. I could be convinced to move it to "won't fix", but didn't want to make that call here.

[zepumph]

Ha! What fun. @jbphet and I believe we have a pretty reasonable fix that I would be fine with him committing. No matter, we both agreed that we wouldn't want it to block MSAB. (and it was only a 20 minute meeting).

[jbphet]

Here is a patch for the proposed fix. We should have this reviewed the thoroughly tested by QA before publishing any sims with this, since it will affect every phet and phet-io brand sim.

Subject: [PATCH] prevent clipped sound when setting phet-io state, see https://github.com/phetsims/tambo/issues/159
---
Index: js/buttons/ResetAllButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/buttons/ResetAllButton.ts b/js/buttons/ResetAllButton.ts
--- a/js/buttons/ResetAllButton.ts	(revision 2d1cb1daeb7c5a998fbc6f7e39542f67bffa1bf0)
+++ b/js/buttons/ResetAllButton.ts	(date 1724801106830)
@@ -24,6 +24,10 @@
 import Property from '../../../axon/js/Property.js';
 import TextKeyNode from '../keyboard/TextKeyNode.js';
 import StringUtils from '../../../phetcommon/js/util/StringUtils.js';
+import isSettingPhetioStateProperty from '../../../tandem/js/isSettingPhetioStateProperty.js';
+import TSoundPlayer from '../../../tambo/js/TSoundPlayer.js';
+import stepTimer from '../../../axon/js/stepTimer.js';
+import soundConstants from '../../../tambo/js/soundConstants.js';
 
 const MARGIN_COEFFICIENT = 5 / SceneryPhetConstants.DEFAULT_BUTTON_RADIUS;
 
@@ -31,7 +35,7 @@
   phetioRestoreScreenStateOnReset?: boolean;
 };
 
-export type ResetAllButtonOptions = SelfOptions & StrictOmit<ResetButtonOptions, 'xMargin' | 'yMargin'>;
+export type ResetAllButtonOptions = SelfOptions & StrictOmit<ResetButtonOptions, 'xMargin' | 'yMargin' | 'soundPlayer'>;
 
 const isResettingAllProperty = new TinyProperty( false );
 
@@ -60,9 +64,6 @@
       tandemNameSuffix: 'ResetAllButton',
       phetioDocumentation: 'The orange, round button that can be used to restore the initial state',
 
-      // sound generation
-      soundPlayer: sharedSoundPlayers.get( 'resetAll' ),
-
       // pdom
       innerContent: SceneryPhetStrings.a11y.resetAll.labelStringProperty,
 
@@ -74,6 +75,12 @@
     assert && assert( options.xMargin === undefined && options.yMargin === undefined, 'resetAllButton sets margins' );
     options.xMargin = options.yMargin = options.radius * MARGIN_COEFFICIENT;
 
+    assert && assert( options.soundPlayer === undefined, 'resetAllButton sets the sound player' );
+    options.soundPlayer = createResetAllSoundPlayer(
+      options.phetioRestoreScreenStateOnReset,
+      () => this.isPhetioInstrumented()
+    );
+
     super( options );
 
     // a11y - When reset all button is fired, disable alerts so that there isn't an excessive stream of alerts while
@@ -170,4 +177,44 @@
   } );
 }
 
+/**
+ * Create a specialized sound player for the Reset All button.  This is needed because this button is basically the only
+ * thing that can and should generate sound during a reset and during the setting of phet-io state.  This makes it a
+ * special case, and it needs special handling.  Specifically, when pressing this button causes the setting of phet-io
+ * state (such as when running in a Standard PhET-iO Wrapper), we need to wait to play the sound until the setting of
+ * state is complete, otherwise it will get partially muted.  See https://github.com/phetsims/tambo/issues/159 for more
+ * detailed history on this if you need it.
+ */
+const createResetAllSoundPlayer = ( phetioRestoreScreenStateOnReset: boolean,
+                                    isPhetioInstrumented: () => boolean ): TSoundPlayer => {
+
+  const baseSoundPlayer = sharedSoundPlayers.get( 'resetAll' );
+  return {
+    play: () => {
+      if ( Tandem.PHET_IO_ENABLED &&
+           phetioRestoreScreenStateOnReset &&
+           isPhetioInstrumented() &&
+           phet.phetio.phetioEngine.phetioStateEngine.mostRecentSavedState ) {
+
+        const playSoundWhenDoneSettingState = ( isSettingPhetioState: boolean ) => {
+          if ( !isSettingPhetioState ) {
+
+            // Play the sound, but delay it enough so that the overall sound output level is fully ramped up.
+            const playDelay = soundConstants.DEFAULT_LINEAR_GAIN_CHANGE_TIME * 1000 * 1.1;
+            stepTimer.setTimeout( () => baseSoundPlayer.play(), playDelay );
+
+            // Unhook the listener.
+            isSettingPhetioStateProperty.unlink( playSoundWhenDoneSettingState );
+          }
+        };
+        isSettingPhetioStateProperty.lazyLink( playSoundWhenDoneSettingState );
+      }
+      else {
+        baseSoundPlayer.play();
+      }
+    },
+    stop: () => baseSoundPlayer.stop()
+  };
+};
+
 sceneryPhet.register( 'ResetAllButton', ResetAllButton );

[kathy-phet]

OK! Well, sounds like you found a fix without much time. That works.