Better KeyboardDragListener behavior and API for dragVelocity and downDelta

[jessegreenberg]

KeyboardDragListener has two options with behavior that could be improved downDelta, which controls how far the object should move on initial press and dragVelocity which controls how far the object should move in view coordinates per second while keys are pressed down. dragVelocity supports smooth (game-like) motion while downDelta is used for app press-and-hold behavior where motion is in discrete steps.

The problem is that they are used together which can be confusing, create buggy situations, and has a bad API at usages. Example of a bug caused by this is phetsims/quadrilateral#186. Example of how using them together is confusing/bad API is
https://github.com/phetsims/quadrilateral/blob/35f930e33fa5e6ac0a0f20bbce73a8c9f4db7ee8/js/quadrilateral/view/SideNode.ts#L169-L173

The intent in this example is to move the object by viewDragDelta on initial press and then continue to move by that much every frame. I shouldn't have to use both options in this case. I shouldn't have to multiply viewDragDelta by 60 for dragVelocity "assuming 60 fps" to get continuous motion.

I think it would be better to make dragVelocity and downDelta mutually exclusive. You cannot use both. KeyboardDragListener will either support smooth game-like motion or discrete steps with press and hold.

[jessegreenberg]

I reviewed the proposal and current usages of KeyboardDragListener with @zepumph who agreed that this seems like a reasonable change. Ill give it a try.

[jessegreenberg]

Almost there, here is a patch I will come back to next time:

Index: js/listeners/KeyboardDragListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/listeners/KeyboardDragListener.ts b/js/listeners/KeyboardDragListener.ts
--- a/js/listeners/KeyboardDragListener.ts	(revision 9509fc901e4345e3a93e6e070eba4797bfe671ef)
+++ b/js/listeners/KeyboardDragListener.ts	(date 1659740286425)
@@ -35,6 +35,7 @@
 import optionize from '../../../phet-core/js/optionize.js';
 import IReadOnlyProperty from '../../../axon/js/IReadOnlyProperty.js';
 import IEmitter from '../../../axon/js/IEmitter.js';
+import assertMutuallyExclusiveOptions from '../../../phet-core/js/assertMutuallyExclusiveOptions.js';
 
 type PressedKeyTiming = {
 
@@ -59,13 +60,29 @@
 
 type SelfOptions = {
 
-  // While direction key is down, this will be the 1D velocity for movement. The position will
-  // change this much in view coordinates every second.
+  // How much the position Property will change in view coordinates every moveOnHoldInterval. Object will
+  // move in discrete steps at this interval. If you would like smoother "animated" motion use dragVelocity
+  // instead. downDelta produces a UX that is more typical for applications but dragVelocity is better for video
+  // game-like components. downDelta and dragVelocity are mutually exclusive options.
+  downDelta?: number;
+
+  // How much the PositionProperty will change in view coordinates every moveOnHoldInterval while the shift modifier
+  // key is pressed. Shift modifier should produce more fine-grained motion so this value needs to be less than
+  // downDelta if provided. Object will move in discrete steps. If you would like smoother "animated" motion use
+  // dragVelocity options instead. downDelta options produce a UX that is more typical for applications but dragVelocity
+  // is better for game-like components. downDelta and dragVelocity are mutually exclusive options.
+  shiftDownDelta?: number;
+
+  // While a direction key is held down, the target will move by this amount in view coordinates every second.
+  // This is an alternative way to control motion with keyboard than downDelta and produces smoother motion for
+  // the object. dragVelocity and downDelta options are mutually exclusive. See downDelta for more information.
   dragVelocity?: number;
 
-  // If shift key down while pressing direction key, this will be the 1D delta for movement in view
-  // coordinates every second. Must be less than or equal to dragVelocity, and it is intended to provide user with
-  // more fine-grained control of motion.
+  // While a direction key is held down with the shift modifier key, the target will move by this amount in view
+  // coordinates every second. Shift modifier should produce more fine-grained motion so this value needs to be less
+  // than dragVelocity if provided. This is an alternative way to control motion with keyboard than downDelta and
+  // produces smoother motion for the object. dragVelocity and downDelta options are mutually exclusive. See downDelta
+  // for more information.
   shiftDragVelocity?: number;
 
   // If provided, it will be synchronized with the drag position in the model frame, applying provided transforms as
@@ -95,14 +112,6 @@
   // Time interval at which the object will change position while the arrow key is held down, in ms
   moveOnHoldInterval?: number;
 
-  // On initial key press, how much the position Property will change in view coordinates, generally only needed when
-  // there is a moveOnHoldDelay or moveOnHoldInterval. In ms.
-  downDelta?: number;
-
-  // The amount PositionProperty changes in view coordinates, generally only needed when there is a moveOnHoldDelay or
-  // moveOnHoldInterval. In ms.
-  shiftDownDelta?: number;
-
   // Time interval at which holding down a hotkey group will trigger an associated listener, in ms
   hotkeyHoldInterval?: number;
 
@@ -180,11 +189,22 @@
   // A reference to the Pointer during a drag operation so that we can add/remove the _pointerListener.
   private _pointer: PDOMPointer | null;
 
+  // Whether we are using a velocity implementation or delta implementation for dragging. See options
+  // downDelta and dragVelocity for more information.
+  private readonly useDragVelocity: boolean;
+
   public constructor( providedOptions?: KeyboardDragListenerOptions ) {
 
+    // Use either dragVelocity or downDelta, cannot use both at the same time.
+    assert && assertMutuallyExclusiveOptions( providedOptions, [ 'dragVelocity', 'shiftDragVelocity' ], [ 'downDelta', 'shiftDownDelta' ] );
+
     const options = optionize<KeyboardDragListenerOptions, SelfOptions, EnabledComponentOptions>()( {
-      dragVelocity: 600,
-      shiftDragVelocity: 300,
+
+      // default moves the object roughly 600 view coordinates every second, assuming 60 fps
+      downDelta: 10,
+      shiftDownDelta: 5,
+      dragVelocity: 0,
+      shiftDragVelocity: 0,
       positionProperty: null,
       transform: null,
       dragBoundsProperty: null,
@@ -193,8 +213,6 @@
       end: null,
       moveOnHoldDelay: 0,
       moveOnHoldInterval: 0,
-      downDelta: 0,
-      shiftDownDelta: 0,
       hotkeyHoldInterval: 800,
       phetioEnabledPropertyInstrumented: false,
       tandem: Tandem.REQUIRED,
@@ -204,6 +222,7 @@
     }, providedOptions );
 
     assert && assert( options.shiftDragVelocity <= options.dragVelocity, 'shiftDragVelocity should be less than or equal to shiftDragVelocity, it is intended to provide more fine-grained control' );
+    assert && assert( options.shiftDownDelta <= options.downDelta, 'shiftDownDelta should be less than or equal to downDelta, it is intended to provide more fine-grained control' );
 
     super( options );
 
@@ -231,6 +250,10 @@
     // while holding the hotkey should result in a delay of this much. in ms
     this.hotkeyHoldIntervalCounter = this._hotkeyHoldInterval;
 
+    // for readability - since dragVelocity and downDelta are mutually exclusive, a value for either one of these
+    // indicates dragging implementation should use velocity
+    this.useDragVelocity = options.dragVelocity > 0 || options.shiftDragVelocity > 0;
+
     this.moveOnHoldDelayCounter = 0;
     this.moveOnHoldIntervalCounter = 0;
 
@@ -261,9 +284,13 @@
         }
       }
 
-      // move object on first down before a delay
-      const positionDelta = this.shiftKeyDown() ? this._shiftDownDelta : this._downDelta;
-      this.updatePosition( positionDelta );
+      // initial movement on down should only be used for downDelta implementation
+      if ( !this.useDragVelocity ) {
+
+        // move object on first down before a delay
+        const positionDelta = this.shiftKeyDown() ? this._shiftDownDelta : this._downDelta;
+        this.updatePosition( positionDelta );
+      }
     }, {
       parameters: [ { name: 'event', phetioType: SceneryEvent.SceneryEventIO } ],
       tandem: options.tandem.createTandem( 'dragStartAction' ),
@@ -569,19 +596,33 @@
         this.hotkeyHoldIntervalCounter += ms;
       }
 
-      // calculate change in position from time step
-      const positionVelocitySeconds = this.shiftKeyDown() ? this._shiftDragVelocity : this._dragVelocity;
-      const positionVelocityMilliseconds = positionVelocitySeconds / 1000;
-      const positionDelta = ms * positionVelocityMilliseconds;
+      let positionDelta = 0;
+
+      if ( this.useDragVelocity ) {
+
+        // calculate change in position from time step
+        const positionVelocitySeconds = this.shiftKeyDown() ? this._shiftDragVelocity : this._dragVelocity;
+        const positionVelocityMilliseconds = positionVelocitySeconds / 1000;
+        positionDelta = ms * positionVelocityMilliseconds;
+      }
+      else {
+        positionDelta = this.shiftKeyDown() ? this._shiftDownDelta : this._downDelta;
+      }
 
-      if ( this.moveOnHoldDelayCounter >= this._moveOnHoldDelay && !this.delayComplete ) {
+      if ( positionDelta > 0 ) {
         this.updatePosition( positionDelta );
-        this.delayComplete = true;
       }
 
-      if ( this.delayComplete && this.moveOnHoldIntervalCounter >= this._moveOnHoldInterval ) {
-        this.updatePosition( positionDelta );
-      }
+      // TODO: For next time, put these checks into the downDelta implementation, they don't matter for
+      // dragVelocity implementation
+      // if ( this.moveOnHoldDelayCounter >= this._moveOnHoldDelay && !this.delayComplete ) {
+      //   this.updatePosition( positionDelta );
+      //   this.delayComplete = true;
+      // }
+      //
+      // if ( this.delayComplete && this.moveOnHoldIntervalCounter >= this._moveOnHoldInterval ) {
+      //   this.updatePosition( positionDelta );
+      // }
     }
   }
 

[jessegreenberg]

The behavior is changed as described with the above commits. I ran through perennial/data/interactive-description sims that use KeyboardDragListener and fixed usages so that they run with the new assertion. I spot checked each and saw the dragging behavior looking OK.

Option downDelta would be better named as dragDelta to match the other option dragVelocity and because this delta is no longer just for the initial down. Ill do that rename now.

[jessegreenberg]

Rename complete in the above commits.

@zepumph can you please review this? Most important commit is 85f7b3d. The rest update usages and rename options.

[zepumph]

This is great! Thanks!

[jessegreenberg]

Thanks, closing.

[zepumph]

Looks like we greatly changed the default dragging here, as found in phetsims/friction#321. Do you think there are any other usages that should be updated with this work?

[jessegreenberg]

Sorry about that, I missed it in #1437 (comment). I believe this change impacted sims that were using the default dragVelocity options.

Sims that use KeyboardDragListener:

• ph-scale - uses custom dragVelocity
• area-model-common sims - uses custom dragDelta
• BASE - uses custom dragVelocity
• fourier-making-waves - uses custom dragVelocity
• geometric-optics - uses custom dragVelocity
• ISLC sims - uses custom dragDelta
• quadrialteral - uses dragDelta
• friction - done after phetsims/friction@446cb77
• faradays-law - may be a problem!

In general it looks like the commits above correct for this change in sims, but Friction and faradays-law were missed.

[jessegreenberg]

I just checked faradays law and published sim feels identical to master. I feel OK closing again.

[zepumph]

Just noting here from phetsims/friction#322 that in phetsims/friction#321 we actually should have used dragVelocity for the continuous movement in friction.