phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

Reset sound muted in sim where PhET-iO state was set #724

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

Reported in https://github.com/phetsims/phet-io/issues/1626#issuecomment-1034217060 by @Nancy-Salpepi:

When the reset all button is pressed in the downstream sim, no reset sound is heard. This is seen in all of the sims I tested thus far that have this sound: Balancing Act, BASE, Build an Atom, Faraday's Law, Friction, GFL, GFL:B.

This issue is seen with both Mac + Chrome and Safari.

To reproduce:

  1. Launch the State wrapper for any of the above sims.
  2. Set the State Rate equal to zero.
  3. Move things around in the downstream sim to make noise--sounds should all work
  4. Press the reset-all button in the downstream sim --no sound or a short tap sound is heard instead of the reset sound.

I have some leads, most likely we just need the reset sound to bypass the muting that goes on during state set.

zepumph commented 2 years ago

This is caused because when the state has been set on a sim, setState is called again on reset all (for that screen only). The simplest solution I could think of was that the resetAllButton shouldn't have this enabled control Property. This makes sense since it is a one-shot sound that will NEVER have state to control its firing. Thus, it seems reasonable to opt out of a state-related enabledControlProperty.

This is highly tied to the solution that @jbphet and I worked on over in https://github.com/phetsims/phet-io/issues/1626#issuecomment-1021770433.

@jbphet how does this patch seem to you? It is working well from my testing.

```diff 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 df48fc38788139cf442673f9edcd56036e9aff90) +++ b/js/sound-generators/SoundGenerator.js (date 1645568096005) @@ -52,8 +52,11 @@ // {AudioNode[]} Audio nodes that will be connected in the specified order between the bufferSource and // localGainNode, used to insert things like filters, compressors, etc. - additionalAudioNodes: [] + additionalAudioNodes: [], + // {boolean} When true, an enable-control Property will be added that mutes the sound when setting PhET-iO state. + // Almost all sounds want this to occur, please test thoroughly before turning this off. + phetioStateSettingControlled: true }, options ); options.enableControlProperties.forEach( enableControlProperty => { @@ -157,7 +160,7 @@ this.soundSourceDestination = audioNode; } - if ( Tandem.PHET_IO_ENABLED ) { + if ( Tandem.PHET_IO_ENABLED && options.phetioStateSettingControlled ) { if ( Tandem.launched ) { assert && assert( notSettingPhetioStateProperty, 'Should exist after launch' ); Index: js/shared-sound-players/resetAllSoundPlayer.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/shared-sound-players/resetAllSoundPlayer.js b/js/shared-sound-players/resetAllSoundPlayer.js --- a/js/shared-sound-players/resetAllSoundPlayer.js (revision df48fc38788139cf442673f9edcd56036e9aff90) +++ b/js/shared-sound-players/resetAllSoundPlayer.js (date 1645567910017) @@ -12,7 +12,10 @@ // create the shared sound instance const resetAllSoundPlayer = new SoundClipPlayer( resetAll_mp3, { - soundClipOptions: { initialOutputLevel: 0.39 }, + soundClipOptions: { + initialOutputLevel: 0.39, + phetioStateSettingControlled: false + }, soundManagerOptions: { categoryName: 'user-interface' } } );
zepumph commented 2 years ago

@jbphet and I spoke about this, and it wasn't a 100% yes commit this. We both want to think if there is a better way to control most/all sounds save the reset one.

jbphet commented 2 years ago

Okay, I've given this some thought, and it seems like a reasonable approach to me. I would suggest a different name for the phetioStateSettingControlled option thought, since the sound generator isn't really being controlled in any way by the setting of phet-io state. How about disabledDuringPhetioStateSetting?

zepumph commented 2 years ago

I love it, thanks for thinking on it!

zepumph commented 2 years ago

Alright, I committed, I went with enabledDuringPhetioStateSetting default to false, because I didn't like the double negative at the usage site:

disabledDuringPhetioStateSetting: false

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.

zepumph commented 2 years ago

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!

zepumph commented 2 years ago

I met with @jbphet about this today. I created https://github.com/phetsims/tambo/issues/159, and we will take it from there. Closing