Sound may be briefly interrupted after starting

[KatieWoe]

Test device
Dell
Operating System
Win 10
Browser
Firefox
Problem description
For phetsims/qa#601.
Sometimes, the metronome-like sound that indicates that hands are close to the ratio will start, seem to be interrupted and pause with no input, then start up again. I haven't fully determined what causes it, so I will record the settings at the time.
Steps to reproduce (Possibly)

1. Occurred on second screen
2. Happened with a few ratios, including 1:2
3. Enhanced sound on
4. Range was 0 -20
5. Ratio locked before using keyboard nav
6. Use keyboard nav to move hands up and down while locked.
7. Use number pad to unlock ratio and move hands between different levels that produce sound.
8. May be caused by jumping while in the middle of old sound?

Visuals

Image.from.iOS.MOV

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Ratio and Proportion‬
URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.0.0-rc.1/phet/ratio-and-proportion_all_phet.html
Version: 1.0.0-rc.1 2021-01-26 23:41:16 UTC
Features missing: touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0
Language: en-US
Window: 1280x687
Pixel Ratio: 1.5/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0
Vendor: Mozilla (Mozilla)
Vertex: attribs: 16 varying: 30 uniform: 4096
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 32767x32767
OES_texture_float: true
Dependencies JSON: {}

[zepumph]

This is basically the same underlying cause as #299. Tagging @BLFiedler and @terracoda about this so that they are in the loop.

In this issue, you jump both hands down to 1, and the model says that it is "moving in direction", and because it is with small values, you are actually in-proportion when moving in direction. Thus, for 30 frames (~1/2 second, the granularity of how often moving-in-direction is computed).

This makes me want to try to again get a solution by changing the moving in direction algorithm. I think that if it changed so that it just had to check changes twice instead of once, it would make more sense.

[zepumph]

This change set successfully updates the "granularity" of moving in direction such that you need 3 data points of change before it considers you moving in direction. I want to discuss to see if this is a possible solution to this and #299. The main point of hesitation is that you need to change the value 2 times (3 different values of the ratio) within a given time frame. And the moving-in-proportion sound will be delayed by how long that time frame is. The current value is 30 frames (1/2 second), and mouse behaves well with it, but it seems too quick for keyboard (since you need 3 presses in half a second. I tried 1 second, and for mouse it seemed really delayed, but it was a bit nicer for keyboard, although kinda fast still.

Here is the patch, let's discuss this tomorrow.

Index: js/common/model/RAPRatio.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/RAPRatio.js b/js/common/model/RAPRatio.js
--- a/js/common/model/RAPRatio.js	(revision ddb8145464d26608aaa1144d1b2b3198b3a6f52a)
+++ b/js/common/model/RAPRatio.js	(date 1612836616291)
@@ -21,6 +21,7 @@
 
 // How often (in frames) to capture the change in ratio values for the ratio's "velocity"
 const STEP_FRAME_GRANULARITY = 30;
+const VELOCITY_MEMORY = 3; // TODO
 
 const DEFAULT_TERM_VALUE_RANGE = RAPConstants.TOTAL_RATIO_TERM_VALUE_RANGE;
 
@@ -55,8 +56,8 @@
     this.changeInConsequentProperty = new NumberProperty( 0 );
 
     // @private - keep track of previous values to calculate the change
-    this.previousAntecedentProperty = new NumberProperty( this.tupleProperty.value.antecedent );
-    this.previousConsequentProperty = new NumberProperty( this.tupleProperty.value.consequent );
+    this.previousAntecedents = [];
+    this.previousConsequents = [];
     this.stepCountTracker = 0; // Used for keeping track of how often dVelocity is checked.
 
     // @public - when true, moving one ratio value will maintain the current ratio by updating the other value Property
@@ -201,25 +202,40 @@
 
   /**
    * @private
-   * @param {Property.<number>} previousValueProperty
-   * @param {number} currentValue
+   * @param {number[]} previousValues
    * @param {Property.<number>} currentVelocityProperty
    */
-  calculateCurrentVelocity( previousValueProperty, currentValue, currentVelocityProperty ) {
-    currentVelocityProperty.value = currentValue - previousValueProperty.value;
-    previousValueProperty.value = currentValue;
+  calculateCurrentVelocity( previousValues, currentVelocityProperty ) {
+    if ( previousValues.length === VELOCITY_MEMORY && _.uniq( previousValues ).length >= VELOCITY_MEMORY ) {
+      currentVelocityProperty.value = previousValues[ previousValues.length - 1 ] - previousValues[ 0 ];
+    }
+    else {
+      currentVelocityProperty.value = 0;
+    }
   }
 
   /**
    * @public
    */
   step() {
+    this.stepCountTracker++;
+    const currentTuple = this.tupleProperty.value;
+    if ( this.stepCountTracker % Math.floor( STEP_FRAME_GRANULARITY / VELOCITY_MEMORY ) === 0 ) {
+      this.previousAntecedents.push( currentTuple.antecedent );
+      while ( this.previousAntecedents.length > VELOCITY_MEMORY ) {
+        this.previousAntecedents.shift();
+      }
+
+      this.previousConsequents.push( currentTuple.consequent );
+      while ( this.previousConsequents.length > VELOCITY_MEMORY ) {
+        this.previousConsequents.shift();
+      }
+    }
 
     // only recalculate every X steps to help smooth out noise
-    if ( ++this.stepCountTracker % STEP_FRAME_GRANULARITY === 0 ) {
-      const currentTuple = this.tupleProperty.value;
-      this.calculateCurrentVelocity( this.previousAntecedentProperty, currentTuple.antecedent, this.changeInAntecedentProperty );
-      this.calculateCurrentVelocity( this.previousConsequentProperty, currentTuple.consequent, this.changeInConsequentProperty );
+    if ( this.stepCountTracker % STEP_FRAME_GRANULARITY === 0 ) {
+      this.calculateCurrentVelocity( this.previousAntecedents, this.changeInAntecedentProperty );
+      this.calculateCurrentVelocity( this.previousConsequents, this.changeInConsequentProperty );
     }
   }
 
@@ -236,8 +252,8 @@
     this.enabledRatioTermsRangeProperty.reset();
     this.changeInAntecedentProperty.reset();
     this.changeInConsequentProperty.reset();
-    this.previousAntecedentProperty.reset();
-    this.previousConsequentProperty.reset();
+    this.previousAntecedents.length = 0;
+    this.previousConsequents.length = 0;
     this.stepCountTracker = 0;
     this.lockRatioListenerEnabled = true;
   }

[terracoda]

I can still get the choir sound, but I know the challenge ratio, so I have an advantage :-)

[terracoda]

Oh, this issue is not about the choir sound :-)

[terracoda]

I can reproduce. We'll discuss at meeting.

[zepumph]

At design meeting today, @terracoda, @BLFiedler and I decided that this bug wasn't worth the complexity of the moving-in-direction algorithm change above.

We were already able to excuse the weird behavior over in #299, and here, this bug is just a bit of a pause, and not the negative to the UX.

We will not do anything further here. Closing