Sonification code review

[samreid]

We may still make changes during design meeting, but it seems the existing implementation for sonification is stable and ready for code review. @jbphet are you available to take a look? SineWaveGenerator.js was already reviewed so I think the remaining work to review is just WavesScreenSoundView.js

Also note I marked some TODOs in the file for discussion with the reviewer.

[jbphet]

Overall I'd say it looks like it's on the right track. Here are some comments:

• Why is this a static type with an init function and not just a class with a constructor like WavesScreenView?
• The name of the file seems a bit odd since it ends with the word View and it is about producing sounds. How about WavesScreenSounds or something similar? I have generally just put the sounds into the view file, not in their own separate file, so I haven't run into this yet (but I don't object to having the sounds separated out like this).
• There is a comment in the file that says, "TODO (for the reviewer): is that last constraint about having to wait for the next cycle to hear change OK?". I played with the sim to test this, and it does seem a little odd to me, since the position of the speaker adjusts instantly to the amplitude setting, so it seems like the sound should too. I'd suggest monitoring the amplitude setting and just directly adjusting the output level for the sound based on this.
• The note about the fullSonification query parameter should be marked with a TODO.
• There are a couple of comments that are labeled with TODO: @jbphet that relate to using disabling the sound in some circumstances. I created an issue to investigate, see Using enabled properties and the associatedViewNode option seem to cut volume tambo#74. Can you please take a look at the issue and make sure that I've correctly described the problem that was being seen?
• There is a recently added class in tambo called MultiClip that might be a better choice for the water drop sounds. It uses less resources than creating all the sound clips, and I think would be easier to work with in the client code. It assumes that there don't need to be unique volume settings for the different sounds, but it doesn't appear that you're doing that, so it should work well.
• I made a minor change to the working in the header comment, please make sure you're cool with it
• There are a number of violations of the 120 columns guideline that seem unnecessary IMO.

[samreid]

Why is this a static type with an init function and not just a class with a constructor like WavesScreenView?

It was initially new WavesScreenSoundView() but that was a lint error since it was never assigned. It seemed inappropriate to assign it somewhere if it was never referenced. Let me know what you recommend.

The name of the file seems a bit odd since it ends with the word View and it is about producing sounds. How about WavesScreenSounds or something similar?

I was using "view" as opposed to "model" but maybe that is unnecessary? Should I go with WavesScreenSounds?

I'd suggest monitoring the amplitude setting and just directly adjusting the output level for the sound based on this.

I haven't yet looked into how difficult this would be to implement, but in further testing, this effect did seem minor and transient, so I'm not convinced it would be worth a lot of effort to change. But would you expect it to work properly if I change the playback rate and output level while playing? I'm not sure if there will be artifacts or other concerns to deal with.

The note about the fullSonification query parameter should be marked with a TODO.

done

Can you please take a look at the issue and make sure that I've correctly described the problem that was being seen?

Yes, that's the problem that I saw. I didn't try to replicate the problem recently--let me know if you are able to replicate the problem or not.

There is a recently added class in tambo called MultiClip that might be a better choice for the water drop sounds.

I'll give it a try, thanks!

I made a minor change to the working in the header comment, please make sure you're cool with it

Looks good, thanks!

There are a number of violations of the 120 columns guideline that seem unnecessary IMO.

Addressed all but one in the above commit.

[samreid]

There is a recently added class in tambo called MultiClip that might be a better choice for the water drop sounds.

I tried this but was unable to call MultiClip.setPlaybackRate. Should this be added, or should I continue using an array of SoundClip?

UPDATE: here is my patch in case I return to it:

Index: js/waves/view/WavesScreenSoundView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/waves/view/WavesScreenSoundView.js	(revision 653a7106b151fc0c1a13b4805f07df5e3996464a)
+++ js/waves/view/WavesScreenSoundView.js	(date 1565814397883)
@@ -10,6 +10,7 @@
 
   // modules
   const DerivedProperty = require( 'AXON/DerivedProperty' );
+  const MultiClip = require( 'TAMBO/sound-generators/MultiClip' );
   const Property = require( 'AXON/Property' );
   const ResetAllSoundGenerator = require( 'TAMBO/sound-generators/ResetAllSoundGenerator' );
   const SineWaveGenerator = require( 'WAVE_INTERFERENCE/waves/view/SineWaveGenerator' );
@@ -67,23 +68,18 @@
           initialOutputLevel: 0.7
         } ) );
         if ( model.waterScene ) {
-          const waterDropSoundClip0 = new SoundClip( waterDropSound0 );
-          const waterDropSoundClip1 = new SoundClip( waterDropSound1 );
-          const waterDropSoundClip2 = new SoundClip( waterDropSound2 );
-          const waterDropSoundClip3 = new SoundClip( waterDropSound3 );
-          soundManager.addSoundGenerator( waterDropSoundClip0 );
-          soundManager.addSoundGenerator( waterDropSoundClip1 );
-          soundManager.addSoundGenerator( waterDropSoundClip2 );
-          soundManager.addSoundGenerator( waterDropSoundClip3 );
-          const soundClips = [
-            waterDropSoundClip0,
-            waterDropSoundClip1,
-            waterDropSoundClip2,
-            waterDropSoundClip3
+          const multiClipData = [
+            { value: 0, soundInfo: waterDropSound0 },
+            { value: 1, soundInfo: waterDropSound1 },
+            { value: 2, soundInfo: waterDropSound2 },
+            { value: 3, soundInfo: waterDropSound3 }
           ];
+          const waterDropMultiClip = new MultiClip( multiClipData );
+          soundManager.addSoundGenerator( waterDropMultiClip );
+          const soundClipValues = multiClipData.map( v => v.value );
 
-          // The water drop SoundClip that was most recently played, to avoid repeats
-          let lastPlayedWaterDropSoundClip = null;
+          // {number} The water drop SoundClip that was most recently played, to avoid repeats
+          let lastWaterDropValue = -1;
 
           // When a water drop is absorbed, play a water drop sound.
           model.waterScene.waterDropAbsorbedEmitter.addListener( waterDrop => {
@@ -96,10 +92,11 @@
             );
 
             // Select water drop sounds randomly, but do not let the same sound go twice in a row
-            const availableClips = _.without( soundClips, lastPlayedWaterDropSoundClip );
-            lastPlayedWaterDropSoundClip = phet.joist.random.sample( availableClips );
-            lastPlayedWaterDropSoundClip.setPlaybackRate( amplitude );
-            lastPlayedWaterDropSoundClip.play();
+            const availableValues = _.without( soundClipValues, lastWaterDropValue );
+            lastWaterDropValue = phet.joist.random.sample( availableValues );
+            waterDropMultiClip.setPlaybackRate( amplitude );
+            console.log( lastWaterDropValue );
+            waterDropMultiClip.play( lastWaterDropValue );
           } );
         }
 

[jbphet]

Ah, I hadn't thought about the fact that playback rate changes are needed. How about leave it as is, i.e. don't use MultiClip, and I'll give this some thought.

[jbphet]

It was initially new WavesScreenSoundView() but that was a lint error since it was never assigned. It seemed inappropriate to assign it somewhere if it was never referenced. Let me know what you recommend.

I've been creating sounds in the view type, since I think of it as "presentation" and not just view. If we decide that we'd like to separate out the "sound view" into a separate class, we should really add support for this to the framework, specifically to the Screen type so that an optional "sound view" can be provided. It would then be instantiated in the same way that ScreenView classes are.

I think this is the only outstanding comment, assign back to me as needed if not or if there is more to discuss on this.

[emily-phet]

@samreid - is this still relevant, or perhaps a new issue for a new review? Not sure what the typical practice is on this sort of thing...but just noticed this seemed old and possibly out-of-date.

[samreid]

I'll need to make another cleanup pass once the sound design is finalized. Then review with or by @jbphet would be good. Leaving open and on hold until then.

[samreid]

@jbphet would you like to review this sonification code before we go to dev testing? Please be aware of phetsims/tambo#74 which is under discussion. It's ready if you are.

For the scope of the review, the sonification parts of these files:

• WaveMeterNode.js
• WavesScreenSoundView.js
• WaveInterferenceSlider.js
• WavesScreenView.js
• SineWaveGenerator.js

Perhaps also skim the files checked in to sounds/ and make sure their metadata (mono vs stereo/bitrate/etc) is appropriate.

Anything else you can think of that should be reviewed for sonification?

[jbphet]

Review complete, see //REVIEW markers in the code.

Also, on the topic of WavesScreenSoundView having a static init function, this doesn't seem very consistent with how we handle visual views. Here is a suggestion: Consider adding an field to the options parameter in Screen.js named something like createSoundView, similar to the required createModel and createView parameters, and have it be created there. We could even step it if it has a step function like we do for the model and view, which would be quite useful in cases where sound generators need it.

[samreid]

Thanks for the code review, @jbphet, good suggestions! I addressed them in the commits. Ready for your re-review.

[jbphet]

@samreid - most of the commits look good. One question: In #426 (comment), as part of this review, I suggested a different way to handle the initialization of the sound view rather that using an init function that is called from WavesScreenView. It looks like that part wasn't done. Was this considered and rejected, or was it just missed?

[samreid]

Thanks, I neglected to comment on that part. I don't like the existing pattern:

WavesScreenSoundView.init( model, this, options );

The proposal from the comment is to add a new Screen.js method like Screen.createSoundView, so that the constructor would change from

function Screen( createModel, createView, options ) {

to

function Screen( createModel, createView, createSoundView, options ) {

I do not agree with this proposal. The model and view parts of Screen.js indicate the model and view parts from an MVC pattern, and createView should create all parts of the view (including audio), not just the visual part of the view. This isn't perfectly symmetric at the moment because createView creates a scenery Node.

One way forward would be to have createView create a composite that has a visual Node and an optional audio component. But that seems like a lot of work for a minor improvement. For now, it seems suitable to have the audio view be an optional component of the visual view.

For WavesScreenView, would it be better to do:

      // @private
      this.wavesScreenSoundView = new WavesScreenSoundView( model, this, options );

?

[jbphet]

@samreid - what you responded to is not quite what I proposed. I proposed an option called createSoundView, not a parameter. This option would be set to null and unused by default but, if set, the function would be called and the created object stepped if it has a step method. So let's be clear: when you say "I do not agree with this proposal", do you also disagree with there being an option that allows creation of a sound view?

If so, I'd say go ahead with assigning this.wavesScreenSoundView. It will probably be flagged in the IDE as unused, but it still strikes me as a better pattern than having a static init method.

[samreid]

Thanks for the correction, I did write my comment based on a required parameter, but all of the same comments apply equally well to an option. To be clear, I do not think there should be an option to create a sound view, and that createView should create the root of the view, which contains all view components, not just the visual one. I'll go ahead with assigning this.wavesScreenSoundView and based on the last paragraph in the preceding comment, it sounds ready to close after that.

[samreid]

I converted WavesScreenSoundView from static init to constructor, @jbphet can you please review and check the last few comments. Anything else to do here?

[jbphet]

Looks good to me. I think we're done here. Closing.