Skip to content

Commit

Permalink
added REVIEW comments, see #426
Browse files Browse the repository at this point in the history
  • Loading branch information
jbphet committed Jun 2, 2020
1 parent 58c646b commit 631f83e
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 1 deletion.
1 change: 1 addition & 0 deletions js/common/view/WaveInterferenceControlPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 );

Expand Down
4 changes: 4 additions & 0 deletions js/common/view/WaveMeterNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions js/waves/view/SineWaveGenerator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 5 additions & 1 deletion js/waves/view/WavesScreenSoundView.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
} );
Expand All @@ -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 );
Expand Down Expand Up @@ -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.
} );
Expand Down Expand Up @@ -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 );
Expand Down

0 comments on commit 631f83e

Please sign in to comment.