How should we go about adding sound to common components?

[jbphet]

The sound design team has been working on coming up with sounds for common UI components, the first items being:

• checkbox
• push button
• radio button
• play/pause/step buttons
• reset button
• screen selection (including home screen)

So far, I have not been adding the sounds to the common code components themselves, I've been creating and adding code that monitors the item that is controlled (e.g. the property controlled by the checkbox) within the sim and generating the sound there. Since sound associated with common UI components is going to be used in several simulations relatively soon, it's time to plan out the details of how the code to produce the sounds should behave and how it should be added to the common-code components.

I'd like to approach this by putting my initial thoughts in this GitHub issue and then soliciting feedback from the other developers by marking this for developer meeting discussion. If others think that we should have a separate meeting dedicated to this, I'd be fine with that alternative.

Here is a list of the questions that immediately come to mind for me, and I fully expect that the discussion will raise some others.

1. How should sound be controlled via the options to the UI component constructor? I'm assuming that we'd like to do this as consistently as possible for all components, and I'm also assuming that sound would be on by default. This means that we would start hearing the sound for the given UI component as soon as sound is enabled in the sim. I have a question about whether we want to just have a flag, something like soundEnabled in the options, or something that allows the client to provide an alternative sound that is played at the proper time, something like:

   {
   soundEnabled: true,
   soundToPlay: squeekyDuckSound
   }

2. In a number of cases, we want to prevent the generation of sound during a reset. In the prototypes created to date, this is done by providing the sound generator with an axon Property the indicates whether a reset is occurring, and to using this to prevent sound generation during reset. It seems ugly to me to suddenly have to start passing instances of this Property into sound-generating UI components, but I'm not sure of the best alternative. It could be that the soundManager has a resetInProgress property that it can use to mute most sounds, and the sound generators have a property that indicates whether they should be muted during reset, but this is exactly the sort of thing I'd like to kick around with the dev team.
3. Adding sound will increase the footprint of all components regardless of whether sound is enabled. We could do something clever, like set up the sound.js plugin to fill in all sounds with a small empty sound if sound production is not enabled for the sim. Should we? (It may not be easy to answer this without more information, so an investigation of how much to component grows may be part of the answer).
4. I'm thinking that once the major questions above have been resolved, it would be good to do a trial implementation of a couple of components, such as Checkbox.js and ResetAllButton.js and have those reviewed before doing the rest. Sound reasonable?

[samreid]

Good questions @jbphet, some partial brainstorming here:

• Maybe soundToPlay: null would be a reasonable alternative to soundEnabled:false?
• Passing resetAllProperty once to soundManager sound much nicer than passing it to N user interface components

[jbphet]

Maybe soundToPlay: null would be a reasonable alternative to soundEnabled:false?

I had thought about that, since it would be nice to consolidate it into a single parameter rather than two, but it would mean that the component would use the default sound if soundToPlay was undefined, put would use no sound if the value were explicitly set to null. That struck me as a bit subtle from the client's point of view, but I wouldn't have strong objections to it if the rest of the team preferred this approach. @samreid - do you (or anyone else) know of precedents that have been used in similar situations? It seems like this is a pattern we would have encountered before.

[samreid]

it would be nice to consolidate it into a single parameter rather than two

I agree, that sounds nice.

it would mean that the component would use the default sound if soundToPlay was undefined, put would use no sound if the value were explicitly set to null

Clarifying:

new RadioButton({soundToPlay:null}); // no sound
new RadioButton({});//default sound
new RadioButton({soundToPlay:undefined}); // no sound

The reason for the last line is lodash behavior works like so:

_.extend({a:'hello'},{a:undefined}) // returns {a: undefined}

do you (or anyone else) know of precedents that have been used in similar situations

Perhaps nodes that have a default fill or stroke, but can be passed a fill: null to go unfilled?

[jonathanolson]

do you (or anyone else) know of precedents that have been used in similar situations

Perhaps nodes that have a default fill or stroke, but can be passed a fill: null to go unfilled?

If it's a larger combination of values where an explicit "default" is needed, it sounds most similar to pickable, which defaults to null, but can be true or false. If it's just using null to override a default, Text nodes have a default stroke which can be overridden with stroke: null which sounds similar.

[jbphet]

It also occurs to me that if a common component uses a sound clip, and there are several of the common code components in a sim (like a bunch of checkboxes), the code should be set up such that the sound only has to be decoded once, and not on every construction of the component.

[jbphet]

I have taken a shot at adding code to a couple of common code components, and I'd like to get some input from other devs so that this can be pretty worked out before much of it goes into master. Marking for dev meeting to discuss.

[pixelzoom]

@jbphet after you made commit https://github.com/phetsims/scenery-phet/commit/c6e249a83c7ac6a31397af96dfbd154b1b03c9b0, I'm now seeing the warning shown below in all sims at startup, once for each ResetAllButton instance. For example, it occurs 4 times at startup for Gas Properties.

Is this intended? If so, do we really need to print warnings to the console?

[jbphet]

The warning message will go away shortly, since I'm going to move the code to a branch.

[pixelzoom]

Going forward, as sound is added to UI components, I think that we should avoid creating sound generators when sound is off. They presumably take time to create, and require resources (memory, processor).

I'm also wondering whether it's appropriate to create default sound generators for UI component. E.g. wondering about the impact (startup, performance, memory,...) of having a sound generator by default for every Checkbox, or whether that's even desirable. Perhaps we should have default sound generators that are available for the client to instantiate and attach to UI components, but not have sound generators associated with UI components by default.

For example, if we had CheckboxSoundGenerator that is intended to be used with Checkbox, the client would add a sound generator to the Checkboxes for which sound is desired, e.g.:

const showVectorsCheckbox = new Checkbox( ..., {
  soundGenerator: new CheckboxSoundGenerator(...)
} );

... instead of adding a sound generator to all Checkboxes by default, like:

class Checkbox ... {
  constructor( ..., options ) {
    options = _.extend( {
      soundGenerator: new CheckboxSoundGenerator(...)
      ...
    }, options );
    ...
  }
}

[jbphet]

I brought this up for discussion in the developer meeting today, here are a few notes:

• There is a general question at this point about whether all of these sounds should be on by default, which is what I had been assuming until this point. I'll raise it with the sonification group and get the leaders involved in the decision on how to proceed with this.
• It was suggested that sounds are not loaded at all when sound is not enabled for the sim. This will save load time and memory.
• It was suggested that we should explicitly add the sound generator to the components rather than have them be there by default, but after some discussion it was decided that if they are supposed to be there by default we shouldn't require the developers to add them.
• It should also be possible to assign the sound for a given common component to be only turned on with "enhanced" sounds, and if the sim is launched without enhanced sounds supported, the should should not be loaded or added to the sound manager (to save time and memory).
• When ready, we should do some of the meeting like the ones that were done when we were first getting started on tambo, where ideas were presented and reviewed for 1/2 to 1 hours on Thursdays

[pixelzoom]

@jbphet said:

It was suggested that sounds are not loaded at all when sound is enabled for the sims.

Did you mean "when sound is disabled"?

--> Yep. Edited (by @jbphet)

[jbphet]

Another thing that we may want to do is modify the build process to track whether sound is enabled for a sim and, if not, don't include the base64 versions of the sound in the built file. I'll make some measurements to compare how much having the common code sounds adds to the built file size once we have some more of them so we can decide if we want to handle this in the build process.

[jbphet]

I presented some prototype code at a review meeting today (8/22/2019) that used a strategy pattern for generating sounds in common UI components. I used the strategy pattern because it was similar to the way in which appearance and content are handled for RectangularButtonView and RoundButtonView, so I thought it would be good to be consistent. Also, a strategy pattern is pretty versatile, and would make it possible to support some significant variations in how sound is produced in the future.

Based on the discussion and feedback that occurred during the meeting, I think I'll move away from using a strategy and towards a simpler approach. Most of the development group felt that requiring a strategy here was overkill when all that we are likely to ever need is the ability to play a sound when the button fires. With this in mind, I'll change the current implementation on the branches where this is being explored to just require an instance with a play() method (or null to turn off sound generation) for the buttons.

As part of this discussion, we decided to try having radio button groups delegate the playing of sounds to the individual buttons instead of what I'm doing now, which is to have the RadioButtonGroup be responsible for playing sounds. I'll give this a try, and will likely create a type that uses a single SoundClip instance but supports playing it a different speeds so that a separate instance of SoundClip doesn't need to be instantiated for every radio button.

[jbphet]

We had a technical review meeting of some of the work that has been done on this today. I presented code in PlayPauseButton that looked like this:

    // generate sounds when the state changes, but only if the state change is due to direct user interaction
    var playSounds = function( running ) {
      if ( self.buttonModel.overProperty.value ) {
        running ? playSound.play() : pauseSound.play();
      }
    };
    isPlayingProperty.lazyLink( playSounds );

The general feedback on this was that the implementation was looking at the model state, which seems incorrect, and that it should be instead listening more “purely” to the user interactions. We discussed adding an input listener, but there are complications with this based on order dependencies and not being able to access state information that is needed. It was pointed out that, in spite of the fact that button models are called “models”, they are actually representing the state of the view, so it actually seems reasonable to access information in this model when presenting information to the user and playing sounds. Ultimately, we decided to try a scheme where the button model base class (ButtonModel) will have an emitter that signals that it is time to play whatever sounds the button may be supposed to play. The various view components will monitor this emitter and can monitor internal state of the button model in order to play different sounds if necessary (such as in the case of the play/pause button). The various sub-types of ButtonModel will fire this emitter at the appropriate times, and will always do so after any button model and other internal state changes resulting from the user action are complete. @jbphet will work on this and report back.

[jbphet]

Based on feedback received in the 9/26/2019 technical discussion meeting, here are some of the next steps:

• Sound production should be moved out of RoundButtonView and RectangularButtonView and into their first level descendant classes. In the process, code that can be common should be consolidated so that there isn't a bunch of duplication.
• In order to minimize the size increase for built sims that results from using common sounds, the shared sounds should be separated out from the commonSoundPlayers singleton into a separate file per sound, so that the sims only get the sounds that are associated with the UI controls that they actually use.

[samreid]

There is a problem with the existing implementation. One of the goals was to make it so no common sound decoding is done if that sound is not needed. I tested this by running the following patch:

```diff Index: tambo/js/AutoRegisteringSoundClipProxy.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- tambo/js/AutoRegisteringSoundClipProxy.js (revision 6e3aa080cebcb4f36350c997dae8931404b241f6) +++ tambo/js/AutoRegisteringSoundClipProxy.js (date 1571435638031) @@ -25,6 +25,7 @@ // create the sound clip if sound is enabled for this sim, otherwise don't if ( phet.joist.sim.supportsSound ) { + console.log( 'new sound clip' ); // @private {SoundClip} this.soundClip = new SoundClip( soundInfo, options ); Index: sun/js/demo/DemosScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- sun/js/demo/DemosScreenView.js (revision f0820dd27e2e2d45820cb2b13a2e7981cc28d820) +++ sun/js/demo/DemosScreenView.js (date 1571435626295) @@ -90,16 +90,16 @@ // Combo box for selecting which component to view const selectedDemoProperty = new Property( selectedDemo ); - const comboBox = new ComboBox( comboBoxItems, selectedDemoProperty, listParent, { - buttonFill: 'rgb( 218, 236, 255 )', - cornerRadius: options.comboBoxCornerRadius, - xMargin: options.comboBoxItemXMargin, - yMargin: options.comboBoxItemYMargin, - top: options.comboBoxLocation.x, - left: options.comboBoxLocation.y, - tandem: options.tandem.createTandem( 'comboBox' ) - } ); - this.addChild( comboBox ); + // const comboBox = new ComboBox( comboBoxItems, selectedDemoProperty, listParent, { + // buttonFill: 'rgb( 218, 236, 255 )', + // cornerRadius: options.comboBoxCornerRadius, + // xMargin: options.comboBoxItemXMargin, + // yMargin: options.comboBoxItemYMargin, + // top: options.comboBoxLocation.x, + // left: options.comboBoxLocation.y, + // tandem: options.tandem.createTandem( 'comboBox' ) + // } ); + // this.addChild( comboBox ); // Make the selected demo visible selectedDemoProperty.link( function( demo, oldDemo ) { Index: sun/js/demo/ComponentsScreenView.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- sun/js/demo/ComponentsScreenView.js (revision f0820dd27e2e2d45820cb2b13a2e7981cc28d820) +++ sun/js/demo/ComponentsScreenView.js (date 1571435653899) @@ -146,15 +146,17 @@ const checkbox = new Checkbox( new Text( 'My Awesome Checkbox', { font: new PhetFont( 30 ) } ), property, { - enabledProperty: enabledProperty + enabledProperty: enabledProperty, + checkedSoundPlayer: null, + uncheckedSoundPlayer: null } ); - const enabledCheckbox = new Checkbox( new Text( 'enabled', { - font: new PhetFont( 20 ) - } ), enabledProperty ); + // const enabledCheckbox = new Checkbox( new Text( 'enabled', { + // font: new PhetFont( 20 ) + // } ), enabledProperty ); return new VBox( { - children: [ checkbox, enabledCheckbox ], + children: [ checkbox ], spacing: 30, center: layoutBounds.center } ); Index: sun/js/Checkbox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- sun/js/Checkbox.js (revision f0820dd27e2e2d45820cb2b13a2e7981cc28d820) +++ sun/js/Checkbox.js (date 1571435791834) @@ -67,6 +67,7 @@ appendDescription: true }, options ); + Node.call( this ); PhetioObject.mergePhetioComponentOptions( { visibleProperty: { phetioFeatured: true } }, options ); ```

Then running http://localhost/sun/sun_en.html?brand=phet&ea&supportsSound&screens=2&component=Checkbox

You will see that no sound is produced because null is specified for the checkbox sounds, but it still decodes the audio clips in AutoRegisteringSoundClipProxy.

I believe this problem could be worked around with 2 ingredients:

1. Add caching to soundInfoDecoder
2. Use factory functions as the default values, like so:

// sound options, can replace with a custom sound player or set to null to disable sound production
checkedSoundPlayer: () => new SoundClip( checkboxCheckedSoundInfo ),
uncheckedSoundPlayer: () => new SoundClip( checkboxCheckedSoundInfo ),

And invoke them if they are functions:

if ( typeof options.checkedSoundPlayer === 'function' ) { options.checkedSoundPlayer = options.checkedSoundPlayer(); }
if ( typeof options.uncheckedSoundPlayer === 'function' ) { options.uncheckedSoundPlayer = options.uncheckedSoundPlayer(); }

Part (1) assumes that SoundClip is pretty lightweight and the heavy part is the decoded audio. And that the decoded audio could be shared.

[jbphet]

There is a problem with the existing implementation. One of the goals was to make it so no common sound decoding is done if that sound is not needed.

Thanks for looking at this, but the way that I'm currently planning to prevent decoding of unused common UI sound clips is not through the use of AutoRegisteringSoundClipProxy, it's by having the common sounds in separate files. That way, not only is the sound not decoded if it isn't used, it's also not included in the build.

Let's back up for a second. The three goals I'm trying to address are:

• There is only one copy of each shared UI sound loaded and decoded, thus saving memory and load time
• The shared UI sound is not loaded nor is it included in the build if the associated UI component is not used in the sim (i.e. if no checkboxes are used, the checkbox sounds are not included in the build)
• The shared UI sounds are loaded prior to their first use (otherwise there's an audible delay)

At this point, I'm experimenting with addressing these constraints by having each shared sound in a separate file that is essentially a singleton that constructs and decodes the sound clip the first time it is requested. You can see an example of this by checking out the sounds-in-common-code branch of sun and looking at shared-sound-players/checkboxCheckedSound.js, and then looking at how this shared sound is used in Checkbox.js.

There are a few things that bother me about this implementation. One is that the shared sound is an object with a method invocation that is required to obtain the sound player, as opposed to just the player itself. However, I can't construct the sound clip at RequireJS time because the global that indicates whether sound is supported in the sim isn't yet available. I can't wait for the first invocation of play() to decode it lazily because it causes a noticeable delay (I tried it).

I'm setting up a meeting to discuss this with @samreid and @pixelzoom to brainstorm some alternatives. Some of things that I've been kicking around in my head are:

• Caching all decoded sounds, so that once a sound file is decoded, any other references to it don't cause another decode. Then all sound clips will essentially be shared
• Auto-registering all sounds (this doesn't address much of the problem, but simplifies things in general)
• Making the plugin do some of the lifting, but I'm a bit stymied by the fact that phet.joist.sim.supportsSound isn't available to the plugin

[samreid]

Is it a goal that if the sim has one Checkbox with a pair of custom sound effects, that the default sound effects are not loaded and/or decoded?

[pixelzoom]

@samreid in partial answer to your question... In 10/18/19 dev meeting notes, I asked about this, and @jbphet started to reply but didn't finish:

CM: Do you care whether default sounds that are never actually used are loaded? Do you care whether sounds that are not used are bundled into the .html file? (Ideally, both of those should be avoided.) JB: Yes, I’m trying to avoid both of those situations, and have

[pixelzoom]

Self-assigning for 10/21/19 discussion with @jbphet and @samreid.

[jbphet]

Is it a goal that if the sim has one Checkbox with a pair of custom sound effects, that the default sound effects are not loaded and/or decoded?

Thus far no - it seems like a rare enough case with small enough consequences that I haven't been targeting this. We can discuss if you disagree.

[pixelzoom]

@jbphet This is the opposite of what you said in 10/18/19 dev meeting notes, see https://github.com/phetsims/tambo/issues/73#issuecomment-544064562. Please clarify.

[jbphet]

The distinction is subtle, I admit. I'm thinking of "default sounds that are never used" as the default sound for, say, a checkbox being checked when there are no checkboxes in the sim. The case Sam is describing is one in which the checkbox is used in the sim but the default sound is overridden in all cases.

[pixelzoom]

I was thinking of the same case as Sam. That is "default sounds" are the default values for Checkbox options checkedSoundPlayer and uncheckedSoundPlayer:

      // sound options, can replace with a custom sound player or set to null to disable sound production
      checkedSoundPlayer: checkboxCheckedSound.soundClip,
      uncheckedSoundPlayer: checkboxUncheckedSound.soundClip,

If you're not targeting this case (or allowing for it in the design), then you should have strong evidence that the vast majority of UI components will use their default sounds.

[pixelzoom]

Here are my notes after reviewing Checkbox.js and checkboxCheckedSound.js, as requested in https://github.com/phetsims/tambo/issues/73#issuecomment-544010160.

(EDIT: @jbphet has added responses inline)

• I was surprised to find that the checkbox sound is an MP3 file. I thought we were trying to avoid sound files and use generated sounds where possible (to decrease payload size), and this seems like a reasonable place to use a generated sound. Please clarify. And is this discussion the same for sound files vs generated sounds?

  • (@jbphet): In the prototyping and interviewing that has been done thus far, we have learned that people much prefer decent quality recorded sounds to generated sounds, particularly if the sound is very long. It's true that this will increase sim size, so we're trying to used sounds that are compressed as much as possible in order to minimize the impact.

• tambo/sounds/ is in master, and tambo is a dependency of all sims in build.json. How are we currently preventing these mp3 files from being bundled into the .html deliverable?

  • (@jbphet): The sound.js plugin does this, so only the sounds that are pulled in via require statements will be included in the sim's build.

• Problems with sound file names, example: const checkboxCheckedSoundInfo = require( 'sound!TAMBO/check-box-checked.mp3' );

  • 'check-box' violates bad-text.js, should be 'checkbox'
  • (@jbphet): agreed, will do
  • Use names that will allow us to enforce conventions for require names, ala strings.js. check-box-checked.mp3 should be checkboxChecked.mp3, then tack on suffix for require name
  • (@jbphet): Yes, I'd like to do this, and have the suffix be SoundInfo. Does that seem reasonable?

• SharedSoundClip is a misleading name. It doesn't extend SoundClip. And it made me confused about the (equally confusing) name of checkboxCheckedSound.js. I thought it should be renamed checkboxCheckedSoundClip (since it returns a SharedSoundClip, not a "Sound") but apparently it's not actually a SoundClip.

  • (@jbphet): I do need to refine the terminology, and will do so once we decide if we can configure a sim for whether it supports sound in package.json, in which case some of this will go away. In the mean time, I'll work on better names for these items.

• sound.js says "for loading sound clips", but we now have a different meaning for "sound clip". Should that doc be changed to "for loading sound files"?

  • (@jbphet): Good observation, I've updated the documentation.

• Loading of default sounds that are not used could be avoided like this. We would need to conditionally call require, which is something we have avoided in the past. (EDIT: How would build process deal with this?)

options = merge( {
  checkedSoundPlayer: null, // doc
  uncheckedSoundPlayer: null, // doc
}, options );

if ( !options.checkedSoundPlayer ) {
  options.checkedSoundPlayer = require( 'TAMBO/shared-sound-players/checkboxCheckedSound' );
}
if ( !options.uncheckedSoundPlayer ) {
  options.uncheckedSoundPlayer = require( 'TAMBO/shared-sound-players/checkboxUncheckedSound' );
}

• (@jbphet): We discussed this today, and @samreid reminded us that conditional require statements are hard for the build process to deal with, so we probably want to avoid this.

Related to your questions from https://github.com/phetsims/tambo/issues/73#issuecomment-544010160.

• Caching all decoded sounds, so that once a sound file is decoded, any other references to it don't cause another decode. Then all sound clips will essentially be shared

Sounds like a good idea.

• Auto-registering all sounds (this doesn't address much of the problem, but simplifies things in general)

Sounds like a good idea. Auto-register when loaded by plugin? Or did you have something else in mind?

Making the plugin do some of the lifting, but I'm a bit stymied by the fact that phet.joist.sim.supportsSound isn't available to the plugin

stringTest is available to string.js, do something similar?

[samreid]

Here are my notes from today's meeting. We were agreed on everything except I don't think we had enough time to conclude on the last bullet point:

• Auto-register sound clips with soundManager
• Move supportsSound to package.json so it can be used statically.
• Add a cache in soundInfoDecoder.js
• Delete SharedSoundClip.js
• Delete AutoRegisteringSoundClipProxy.js
• Eliminate checkboxCheckedSound.js and instead require 'sound!TAMBO/check-box-checked.mp3' in Checkbox directly, making a static shared SoundClip for it.

[jbphet]

Assigning to just me for followup.

[samreid]

Auto-register sound clips with soundManager

If sounds are auto-registered, we will need another way to specify associatedViewNode

// Suppress the tone when another screen is selected
soundManager.addSoundGenerator( sineWavePlayer, {
  associatedViewNode: view
} );

Perhaps one way would be for associatedViewNode to be a SoundGenerator option.

[jbphet]

I just made the following PSA on Slack, recording here for posterity:

PSA - I am starting on the steps to add support for common UI sounds to master, so any release branches started after today will include this code. Sound will be off by default, so it shouldn't change the default behavior of the sim in any way. My goal is to keep master stable and to minimize the impact on built sim size, initialization time, and run-time memory usage, but there will of course be some effects (I'll quantify them to some degree and let everyone know what I find out). Please let me know if you see anything in sim behavior that may be caused by this work.

[jbphet]

The common code sounds have been added and have been in use for a while now. It seems to be working reasonably well, so I'll close this issue and log more specific ones for subsequent changes.