Simplify getNextPositionInDimension

[samreid]

From #398, we are interested in improving getNextPositionInDimension. We almost got this patch kind of working, but something like this:

Subject: [PATCH] Move updateOrderDependentProperties into QuadrilateralShapeModel and add documentation, see https://github.com/phetsims/quadrilateral/issues/398
---
Index: js/quadrilateral/model/QuadrilateralModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/QuadrilateralModel.ts b/js/quadrilateral/model/QuadrilateralModel.ts
--- a/js/quadrilateral/model/QuadrilateralModel.ts	(revision a530d4c658f8981cbca1fcedca1e799459ce877a)
+++ b/js/quadrilateral/model/QuadrilateralModel.ts	(date 1677799759119)
@@ -197,10 +197,13 @@
    * with the model grid. See vertexIntervalProperty for more information about how the intervals of the grid
    * can change.
    */
-  public getClosestGridPosition( ProposedPosition: Vector2 ): Vector2 {
+  public getClosestGridPosition( proposedPosition: Vector2 ): Vector2 {
 
     const interval = this.vertexIntervalProperty.value;
-    return new Vector2( Utils.roundToInterval( ProposedPosition.x, interval ), Utils.roundToInterval( ProposedPosition.y, interval ) );
+    return new Vector2(
+      Utils.roundToInterval( proposedPosition.x, interval ),
+      Utils.roundToInterval( proposedPosition.y, interval )
+    );
   }
 
   /**
@@ -209,17 +212,11 @@
    * closest grid position in both X and Y.
    */
   public getClosestGridPositionInDirection( currentPosition: Vector2, directionVector: Vector2 ): Vector2 {
-    let nextX = currentPosition.x;
-    let nextY = currentPosition.y;
 
-    if ( directionVector.x !== 0 ) {
-      nextX = this.getNextPositionInDimension( currentPosition.x, directionVector.x > 0 );
-    }
-    else if ( directionVector.y !== 0 ) {
-      nextY = this.getNextPositionInDimension( currentPosition.y, directionVector.y > 0 );
-    }
-
-    return new Vector2( nextX, nextY );
+    return new Vector2(
+      this.getNextPositionInDimension( currentPosition.x, directionVector.x ),
+      this.getNextPositionInDimension( currentPosition.y, directionVector.y )
+    );
   }
 
   /**
@@ -227,22 +224,15 @@
    * @param currentVal - Current position in x or y dimensions
    * @param gettingLarger - Are you getting larger in that dimension?
    */
-  private getNextPositionInDimension( currentVal: number, gettingLarger: boolean ): number {
-    const interval = this.vertexIntervalProperty.value;
-    let remainder = Math.abs( currentVal ) % interval;
+  private getNextPositionInDimension( currentVal: number, inputDelta: number ): number {
+    if ( inputDelta === 0 ) {
+      return currentVal;
+    }
+    const interval = inputDelta > 0 ? this.vertexIntervalProperty.value : -this.vertexIntervalProperty.value;
+    const remainder = currentVal % interval;
     const onTheInterval = Utils.equalsEpsilon( remainder, 0, 0.01 );
 
-    if ( currentVal < 0 ) {
-      remainder = interval - remainder;
-    }
-
-    let delta;
-    if ( gettingLarger ) {
-      delta = onTheInterval ? interval : interval - remainder;
-    }
-    else {
-      delta = onTheInterval ? -interval : -remainder;
-    }
+    const delta = onTheInterval ? interval : interval - remainder;
 
     return currentVal + delta;
   }

[jessegreenberg]

OK, I added documentation to getNextPositionInDimension and reorganized some of the logic for readability. I think it is much clearer to see what it is doing. I did not pursue the patch in #402 (comment). It looks like that patch is trying to set both x and y from this function and it is important that only one dimension of movement be set at a time. That might be why it wasn't working well when we last looked at this.

Anyway, I am happy with where this function is now and ready for re-review or close.

[jessegreenberg]

Assigning for async re-review but we might discuss this again together soon.

[samreid]

I appreciate the clear and elaborate documentation, thanks! I see opportunity for simplification and improvement. I tried this implementation, with a comparison to see if it behaves exactly as the prior implementation. Every now and then fuzzing with keyboard fuzzing catches a difference, but I have not been able to catch it in a debugger. The idea is that it rounds to the nearest interval, then adds the delta. Does it seem correct to you?

Subject: [PATCH] Standardize author annotations
---
Index: js/quadrilateral/model/QuadrilateralModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/QuadrilateralModel.ts b/js/quadrilateral/model/QuadrilateralModel.ts
--- a/js/quadrilateral/model/QuadrilateralModel.ts	(revision 3bd1b43b624a4b364c5e6275f3e4268f3a88ff07)
+++ b/js/quadrilateral/model/QuadrilateralModel.ts	(date 1679496701156)
@@ -199,10 +199,31 @@
     return new Vector2( nextX, nextY );
   }
 
-  /**
-   * Returns a new position in x or y dimensions, used by getClosestGridPositionInDirection.
-   */
   private getNextPositionInDimension( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
+    const a = this.getNextPositionInDimensionA( currentPosition, directionVector, dimension );
+    const b = this.getNextPositionInDimensionB( currentPosition, directionVector, dimension );
+    console.log( a === b, a, b );
+    if ( a !== b ) {
+      debugger;
+    }
+    return a;
+  }
+
+  private getNextPositionInDimensionB( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
+    const currentValue = currentPosition[ dimension ];
+    const gettingLarger = directionVector[ dimension ] > 0;
+    const interval = this.vertexIntervalProperty.value;
+
+    const currentValueOnInterval = Utils.roundToInterval( currentValue, interval );
+    const delta = gettingLarger ? interval : -interval;
+
+    return currentValueOnInterval + delta;
+  }
+
+  /**
+   * Returns a new position in x or y dimensions, used by getClosestGridPositionInDirection.
+   */
+  private getNextPositionInDimensionA( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
     const currentValue = currentPosition[ dimension ];
     const gettingLarger = directionVector[ dimension ] > 0;
 

[samreid]

Oh, I discovered you have to click in the sim to get keyboard fuzzing to start. @jessegreenberg and I discussed and identified that the main idea is that unless you are within 0.01 of the next interval, you should move to the next interval (if you are below an interval, and within 0.01, then you need to jump 2x intervals). So I feel there is probably a way to simplify the logic, but the proposed implementation above is not correct.

[samreid]

This seems to be getting closer:

  private getNextPositionInDimensionB( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
    const currentValue = currentPosition[ dimension ];
    const gettingLarger = directionVector[ dimension ] > 0;
    const interval = this.vertexIntervalProperty.value;

    if ( gettingLarger ) {
      return Utils.roundToInterval( currentValue + 0.01 - interval / 2 + interval, interval );
    }
    else {
      return Utils.roundToInterval( currentValue - 0.01 + interval / 2 - interval, interval );
    }
  }

[samreid]

I simplified it a bit and it seems to give the same behavior as the existing method. Here is a patch that compares values:

Subject: [PATCH] Standardize author annotations
---
Index: js/quadrilateral/model/QuadrilateralModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/quadrilateral/model/QuadrilateralModel.ts b/js/quadrilateral/model/QuadrilateralModel.ts
--- a/js/quadrilateral/model/QuadrilateralModel.ts	(revision 3bd1b43b624a4b364c5e6275f3e4268f3a88ff07)
+++ b/js/quadrilateral/model/QuadrilateralModel.ts	(date 1679500880414)
@@ -199,10 +199,30 @@
     return new Vector2( nextX, nextY );
   }
 
-  /**
-   * Returns a new position in x or y dimensions, used by getClosestGridPositionInDirection.
-   */
   private getNextPositionInDimension( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
+    const a = this.getNextPositionInDimensionA( currentPosition, directionVector, dimension );
+    const b = this.getNextPositionInDimensionB( currentPosition, directionVector, dimension );
+    console.log( a === b, a, b );
+    if ( a !== b ) {
+      debugger;
+    }
+    return a;
+  }
+
+  private getNextPositionInDimensionB( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
+    const currentValue = currentPosition[ dimension ];
+    const gettingLarger = directionVector[ dimension ] > 0;
+    const interval = this.vertexIntervalProperty.value;
+
+    const shift = +0.01 - interval / 2 + interval;
+    const sign = gettingLarger ? 1 : -1;
+    return Utils.roundToInterval( currentValue + sign * shift, interval );
+  }
+
+  /**
+   * Returns a new position in x or y dimensions, used by getClosestGridPositionInDirection.
+   */
+  private getNextPositionInDimensionA( currentPosition: Vector2, directionVector: Vector2, dimension: 'x' | 'y' ): number {
     const currentValue = currentPosition[ dimension ];
     const gettingLarger = directionVector[ dimension ] > 0;
 

The main idea is that everything in the curly brace maps to the next interval:

I'm getting this exception, but it seems unrelated:

Uncaught Error: Assertion failed: CornerGuideNodes cannot support angles at or less than zero
    at window.assertions.assertFunction (assert.js:28:13)
    at CornerGuideNode.ts:77:17
    at listener (Multilink.ts:111:11)
    at TinyProperty.emit (TinyEmitter.ts:102:9)
    at Property._notifyListeners (ReadOnlyProperty.ts:312:23)
    at Property.unguardedSet (ReadOnlyProperty.ts:263:14)
    at Property.set (ReadOnlyProperty.ts:247:12)
    at set value [as value] (Property.ts:54:11)
    at QuadrilateralVertex.updateAngle (QuadrilateralVertex.ts:186:29)
    at QuadrilateralShapeModel.ts:412:14

[jessegreenberg]

Thank you @samreid, that is looking great! Confirmed about that issue, looks like it started showing up on CT yesterday morning, Ill look at that.

[samreid]

I did not attempt to add good code comments, since I was relying on the diagram above. But would be good to comment this strategy and why it works.

[jessegreenberg]

OK, thanks @samreid. Here is what I added based on my understanding of how this is working:

  /**
   * Get the next value on the interval in provided dimension. The following diagram demonstrates how this works:
   *                interval
   *           |---------------|
   *              A
   *         |---------|
   * -----C--*-|-*-----|-----*-|-*-----------------
   *                   |---------|
   *                        B
   *  C: currentValue
   *  A: If the value lands in this region, next position should be left side of interval.
   *  B: If value lands in this region, next position should be right side of interval.
   *  *: small offset so if currentValue is very close to the interval, we will round to next interval.
   *
   *  So the length of A (or B) is added to currentValue before rounding to the interval.
   *
   *  See https://github.com/phetsims/quadrilateral/issues/402 for more implementation notes.
   */

I did a lot of testing and confirmed that this new method always produces the same values as before. Closing.