From 631f83e1a3c584478486642de923e7baba4e5039 Mon Sep 17 00:00:00 2001
From: jbphet <john.blanco@colorado.edu>
Date: Tue, 2 Jun 2020 16:50:47 -0600
Subject: [PATCH] added REVIEW comments, see
 https://github.com/phetsims/wave-interference/issues/426

---
 js/common/view/WaveInterferenceControlPanel.js | 1 +
 js/common/view/WaveMeterNode.js                | 4 ++++
 js/waves/view/SineWaveGenerator.js             | 1 +
 js/waves/view/WavesScreenSoundView.js          | 6 +++++-
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/js/common/view/WaveInterferenceControlPanel.js b/js/common/view/WaveInterferenceControlPanel.js
index a50e5927..915d2870 100644
--- a/js/common/view/WaveInterferenceControlPanel.js
+++ b/js/common/view/WaveInterferenceControlPanel.js
@@ -47,6 +47,7 @@ class WaveInterferenceControlPanel extends WaveInterferencePanel {
       yMargin: 4,
       showSceneRadioButtons: true,
       showPlaySoundControl: false,
+      //REVIEW - consider another name, perhaps audioEnabled (used elsewhere in this code) to avoid confusion with Sim options
       supportsSound: true
     }, options );
 
diff --git a/js/common/view/WaveMeterNode.js b/js/common/view/WaveMeterNode.js
index fc7c3fe7..52ae7f37 100644
--- a/js/common/view/WaveMeterNode.js
+++ b/js/common/view/WaveMeterNode.js
@@ -227,8 +227,11 @@ class WaveMeterNode extends Node {
               soundManager.addSoundGenerator( soundClip, { associatedViewNode: this } );
             }
 
+            //REVIEW - why such a precise number for the multiplier?
             const outputLevel = getWaveMeterNodeOutputLevel( value ) * 8.912509381337454 * seriesVolume;
 
+            //REVIEW - lots of lines wider than 120 that go off of my screen, suggest breaking up
+
             // "Play Tone" takes precedence over the wave meter node sounds, because it is meant to be used briefly
             const duckFactor = ( model.sceneProperty.value === model.soundScene && model.soundScene.isTonePlayingProperty.value ) ? 0.2 : 1;
 
@@ -244,6 +247,7 @@ class WaveMeterNode extends Node {
 
             const basePlaybackRate = lowProperty.value * playbackRateProperty.value;
             if ( value > 0 ) {
+              //REVIEW - This is a fairly complex calculation with lots of unexplained number, and could use some documentation.
               soundClip.setPlaybackRate( basePlaybackRate * ( intervalProperty.value === 5 ? 329.63 / 220 : intervalProperty.value === 4 ? 293.66 / 220 : 277.18 / 220 ) ); // 5th  (SR #1 pref)
             }
             else {
diff --git a/js/waves/view/SineWaveGenerator.js b/js/waves/view/SineWaveGenerator.js
index f4bc32e9..ac23e15f 100644
--- a/js/waves/view/SineWaveGenerator.js
+++ b/js/waves/view/SineWaveGenerator.js
@@ -53,6 +53,7 @@ class SineWaveGenerator extends SoundGenerator {
       }
       else if ( !fullyEnabled && this.oscillator !== null ) {
 
+        //REVIEW - The comment below seems a little out of context.  Cut and paste error?  If not, please explain what's fading out and how this matches the timing.
         // The parent fades out, we schedule a stop to coincide with the end of the fade out time.
         this.oscillator.stop( this.audioContext.currentTime + soundConstants.DEFAULT_LINEAR_GAIN_CHANGE_TIME );
         this.oscillator = null;
diff --git a/js/waves/view/WavesScreenSoundView.js b/js/waves/view/WavesScreenSoundView.js
index 6c85ea9f..8ed189da 100644
--- a/js/waves/view/WavesScreenSoundView.js
+++ b/js/waves/view/WavesScreenSoundView.js
@@ -42,11 +42,13 @@ class WavesScreenSoundView {
             model.soundScene.isTonePlayingProperty,
             model.soundScene.button1PressedProperty,
             model.isRunningProperty,
+            //REVIEW: There is a BooleanInvertedProperty for the case below in tambo (and perhaps it should go in axon)
             new DerivedProperty( [ model.isResettingProperty ], isResetting => !isResetting )
           ]
         } );
 
       // Suppress the tone when another screen is selected
+      //REVIEW - Are both of the below really needed?  Seems like they would have the same effect.
       soundManager.addSoundGenerator( sineWavePlayer, {
         associatedViewNode: view
       } );
@@ -56,6 +58,7 @@ class WavesScreenSoundView {
 
     if ( model.waterScene ) {
       const waterDropOptions = { initialOutputLevel: 1.5 };
+      //REVIEW - consider putting the sounds (e.g. waterDropSound0) and having a loop here to do all of this to avoid code duplication
       const waterDropSoundClip0 = new SoundClip( waterDropSound0, waterDropOptions );
       const waterDropSoundClip1 = new SoundClip( waterDropSound1, waterDropOptions );
       const waterDropSoundClip2 = new SoundClip( waterDropSound2, waterDropOptions );
@@ -99,7 +102,7 @@ class WavesScreenSoundView {
     if ( model.soundScene ) {
       const speakerMembraneSoundClip = new SoundClip( speakerPulseSound, {
 
-        // The sound repeats, so the waveform should not be clipped
+        // The sound repeats, so the waveform should not be trimmed
         trimSilence: false,
         initialOutputLevel: 0 // The speaker pulse plays when the speaker membrane oscillates.  The outputLevel is set below.
       } );
@@ -149,6 +152,7 @@ class WavesScreenSoundView {
         loop: true
       } );
 
+      //REVIEW - Do we still want this TODO here?  It's on the radar to work on initial audio volume issues, but will likely be a while.
       // TODO: the following line was causing odd audio volume behavior, but this should now be addressed.  @samreid -
       // TODO: please try using it again, and report findings in https://github.com/phetsims/tambo/issues/74.
       // lightBeamLoopSoundClip.addEnableControlProperty( model.lightScene.soundEffectEnabledProperty );