movingInDirection() should be smoothed out

[zepumph]

This will solve #297 and #299. Right now we calculate the velocity every half second as a single opperation, it would be better if we kept track of motion for time X and then determined if it was moving in direction based on multiple points within that time. I'm not yet sure exactly how to do it just yet.

[zepumph]

After a couple of hours of rewriting the movingInDirection() function, I have a new algorithm, one that I like more than the previous one, but unfortunately it doesn't solve either of the bugs linked above. Those are both just behaviors that are in line with the criteria that we established for making the moving sound, but in places where we probably don't want it. I'll keep thinking.

[zepumph]

I don't think I'm going to commit this at this time, but here is my work.

Index: js/common/view/sound/MovingInProportionSoundGenerator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/sound/MovingInProportionSoundGenerator.js b/js/common/view/sound/MovingInProportionSoundGenerator.js
--- a/js/common/view/sound/MovingInProportionSoundGenerator.js	(revision c37374f06fefc84a8a88583322a960e17f661d91)
+++ b/js/common/view/sound/MovingInProportionSoundGenerator.js	(date 1608767744915)
@@ -47,8 +47,7 @@
     this.movingInProportionSoundClip.connect( this.soundSourceDestination );
 
     Property.multilink( [
-      model.ratio.changeInAntecedentProperty,
-      model.ratio.changeInConsequentProperty,
+      model.ratio.tupleProperty,
       model.ratioFitnessProperty
     ], () => {
       if ( model.ratio.movingInDirection() && // only when moving
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 c37374f06fefc84a8a88583322a960e17f661d91)
+++ b/js/common/model/RAPRatio.js	(date 1608766848354)
@@ -64,9 +64,9 @@
     this.changeInAntecedentProperty = new NumberProperty( 0 );
     this.changeInConsequentProperty = new NumberProperty( 0 );
 
-    // @private - keep track of previous values to calculate the change
-    this.previousAntecedentProperty = new NumberProperty( this.antecedentProperty.value );
-    this.previousConsequentProperty = new NumberProperty( this.consequentProperty.value );
+    // @private - keep track of previous values to calculate the change TODO: help me doc
+    this.previousAntecedents = [ this.antecedentProperty.value ];
+    this.previousConsequents = [ this.consequentProperty.value ];
     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
@@ -183,22 +183,70 @@
     this.lockRatioListenerEnabled = true;
   }
 
+
+  /**
+   * @private
+   * @param {Array.<number>} termValues
+   * @returns {boolean}
+   */
+  getDataAboutChangeInTerm( termValues ) {
+    let previousValue = termValues[ 0 ];
+
+    let amountChanged = 0;
+
+    // Keeps tracks of simple 0/1/-1 changes to discover if there was a change
+    let changePresenceAccumulator = 0;
+
+    let timesChangedCount = 0;
+    for ( let i = 0; i < termValues.length; i++ ) {
+      const termValue = termValues[ i ];
+      const currentChange = termValue > previousValue ? 1 :
+                            termValue < previousValue ? -1 :
+                            0;
+      if ( currentChange !== 0 ) {
+        timesChangedCount++;
+      }
+      changePresenceAccumulator += currentChange;
+
+      amountChanged += termValue - previousValue;
+      previousValue = termValue;
+    }
+
+    return {
+      changeAlwaysInSameDirection: Math.abs( changePresenceAccumulator ) === timesChangedCount,
+      timesChanged: timesChangedCount,
+      totalChanged: amountChanged
+    };
+  }
+
   /**
    * Whether or not the two hands are moving fast enough together in the same direction. This indicates a bimodal interaction.
    * @public
    * @returns {boolean}
    */
   movingInDirection() {
-    const bothMoving = this.changeInAntecedentProperty.value !== 0 && this.changeInConsequentProperty.value !== 0;
+
+    const antecedantData = this.getDataAboutChangeInTerm( this.previousAntecedents );
+    const consequentData = this.getDataAboutChangeInTerm( this.previousConsequents );
+
+    console.log( antecedantData, consequentData );
+
+    const changeInAntecedent = antecedantData.totalChanged;
+    const changeInConsequent = consequentData.totalChanged;
+    const bothMoving = changeInAntecedent !== 0 && changeInConsequent !== 0;
 
     // both hands should be moving in the same direction
-    const movingInSameDirection = this.changeInAntecedentProperty.value > 0 === this.changeInConsequentProperty.value > 0;
+    const movingInSameDirection = changeInAntecedent > 0 === changeInConsequent > 0;
+    // antecedantData.changeAlwaysInSameDirection &&
+    // consequentData.changeAlwaysInSameDirection;
 
-    const movingFastEnough = Math.abs( this.changeInAntecedentProperty.value ) > VELOCITY_THRESHOLD && // antecedent past threshold
-                             Math.abs( this.changeInConsequentProperty.value ) > VELOCITY_THRESHOLD; // consequent past threshold
+    const movedMoreThanOneTimeEach = antecedantData.timesChanged >= 2 && consequentData.timesChanged >= 2;
+
+    const movingFastEnough = Math.abs( changeInAntecedent ) > VELOCITY_THRESHOLD && // antecedent past threshold
+                             Math.abs( changeInConsequent ) > VELOCITY_THRESHOLD; // consequent past threshold
 
     // Ignore the speed component when the ratio is locked
-    return bothMoving && movingInSameDirection && ( movingFastEnough || this.lockedProperty.value );
+    return bothMoving && movingInSameDirection && movedMoreThanOneTimeEach && ( movingFastEnough || this.lockedProperty.value );
   }
 
   /**
@@ -215,9 +263,11 @@
    * @param {number} currentValue
    * @param {Property.<number>} currentVelocityProperty
    */
-  calculateCurrentVelocity( previousValueProperty, currentValue, currentVelocityProperty ) {
-    currentVelocityProperty.value = currentValue - previousValueProperty.value;
-    previousValueProperty.value = currentValue;
+  calculateCurrentVelocity( previousValues, currentValue ) {
+    previousValues.push( currentValue );
+    while ( previousValues.length > 12 ) {
+      previousValues.shift();
+    }
   }
 
   /**
@@ -226,10 +276,12 @@
   step() {
 
     // only recalculate every X steps to help smooth out noise
-    if ( ++this.stepCountTracker % 30 === 0 ) {
-      this.calculateCurrentVelocity( this.previousAntecedentProperty, this.antecedentProperty.value, this.changeInAntecedentProperty );
-      this.calculateCurrentVelocity( this.previousConsequentProperty, this.consequentProperty.value, this.changeInConsequentProperty );
+    if ( ++this.stepCountTracker % 5 === 0 ) {
+      this.calculateCurrentVelocity( this.previousAntecedents, this.antecedentProperty.value );
+      this.calculateCurrentVelocity( this.previousConsequents, this.consequentProperty.value );
     }
+
+
   }
 
   /**
@@ -246,10 +298,13 @@
     this.enabledRatioTermsRangeProperty.reset();
     this.changeInAntecedentProperty.reset();
     this.changeInConsequentProperty.reset();
-    this.previousAntecedentProperty.reset();
-    this.previousConsequentProperty.reset();
-    this.stepCountTracker = 0;
     this.lockRatioListenerEnabled = true;
+
+    this.previousAntecedents.length = 0;
+    this.previousAntecedents.push( this.antecedentProperty.value );
+    this.previousConsequents.length = 0;
+    this.previousConsequents.push( this.consequentProperty.value );
+    this.stepCountTracker = 0;
   }
 }
 

[zepumph]

I'm going to close for now. I'll reopen if it seems like it makes sense to work on in the future.

[zepumph]

We tried this again in #338 (comment), and then ran into this annoyance again over in #360

[zepumph]

This was finally done. See #360 (comment) and lower for commits.