Testing corner cases with ball animation and input events

[matthew-blackman]

Since the sim uses animation on objects that have input enabled, we feel that it will be helpful to test out the following to confirm the desired behavior:

• More than one ball lands in the same stack in rapid succession
• While one or more balls are animating to the top of a stack, the user clicks on that stack
• While one or more balls are animating to the top of a stack, another ball is dragged onto that stack
• While one or more balls are animating to the top of a stack, another ball is dragged past that stack
• The user hits 'clear data' or 'reset all' while a ball is animating

This is a continuation of the work done in #165

[matthew-blackman]

There is some buggy behavior here. Testing with the query params 'slowAnimation' and 'sameSpot' to make it easier to see animation and input issues. Since the code is starting to get complicated with listeners and callbacks, I think we should lay out the desired behavior before discussing a solution @samreid @marlitas.

Here are some of the scenarios we will need to account for, to confirm UX:

• While a ball lands on a stack that already has other balls, its z-index should immediately be moved to the front.
• If a ball lands on a stack that has other balls animating, all animations continue and the balls animate to their correct positions on the stack (currently the animation is cancelled when the ball reaches the top).
• Any click event on a stack should cancel all animations and immediately rearrange the balls in the stack to their final positions
• Dragging a ball onto or over another stack should also cancel all animations and immediately rearrange the balls in the stack to their final positions
• If a ball lands on a stack that is currently being dragged, what should we do?

We may want to set aside some design time to discuss the UX here. Thoughts?

[marlitas]

Here's a patch that has an interesting floating ball bug.

Subject: [PATCH] clearAnimation
---
Index: js/common/model/SoccerBall.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/SoccerBall.ts b/js/common/model/SoccerBall.ts
--- a/js/common/model/SoccerBall.ts	(revision f14888a75f80e32c7ea6cce2980eca109f03ad81)
+++ b/js/common/model/SoccerBall.ts	(date 1684255978237)
@@ -109,6 +109,12 @@
     } );
   }
 
+  public clearAnimation(): void {
+    if ( this.animation ) {
+      this.animation.stop();
+      this.animation = null;
+    }
+  }
   public step( dt: number ): void {
     if ( this.animationModeProperty.value === AnimationMode.FLYING ) {
 
Index: js/common/model/CAVSceneModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/CAVSceneModel.ts b/js/common/model/CAVSceneModel.ts
--- a/js/common/model/CAVSceneModel.ts	(revision f14888a75f80e32c7ea6cce2980eca109f03ad81)
+++ b/js/common/model/CAVSceneModel.ts	(date 1684256111146)
@@ -158,6 +158,7 @@
           const oldStack = this.getStackAtLocation( oldValue );
           if ( oldStack.length > 0 ) {
             this.reorganizeStack( oldStack );
+            this.clearAnimationsInStack( oldStack );
           }
 
           const objectsAtTarget = this.soccerBalls.filter( otherSoccerBall => {
@@ -166,7 +167,9 @@
 
           // Sort from bottom to top, so they can be re-stacked. The specified object will appear at the top.
           const sortedOthers = _.sortBy( objectsAtTarget, object => object.positionProperty.value.y );
-          this.reorganizeStack( [ ...sortedOthers, soccerBall ] );
+          const newStack = [ ...sortedOthers, soccerBall ];
+          this.reorganizeStack( newStack );
+          this.clearAnimationsInStack( newStack );
         }
       } );
 
@@ -298,6 +301,11 @@
     } );
   }
 
+  // Cancel out all animations in the soccer ball stack.
+  private clearAnimationsInStack( stack: SoccerBall[] ): void {
+    stack.forEach( soccerBall => soccerBall.clearAnimation() );
+  }
+
   protected updateDataMeasures(): void {
     const sortedObjects = this.getSortedLandedObjects();
     const medianObjects = CAVSceneModel.getMedianObjectsFromSortedArray( sortedObjects );
@@ -345,13 +353,6 @@
     // collapse the rest of the stack. NOTE: This assumes the radii are the same.
     let position = CAVObjectType.SOCCER_BALL.radius;
     soccerBallStack.forEach( soccerBall => {
-
-      // If a ball was animating to the top of the stack, stop it. This prevents a floating ball if a lower ball
-      // is moved out from underneath
-      if ( soccerBall.animation ) {
-        soccerBall.animation.stop();
-        soccerBall.animation = null;
-      }
       soccerBall.positionProperty.value = new Vector2( soccerBall.valueProperty.value!, position );
       position += CAVObjectType.SOCCER_BALL.radius * 2 * ( 1 - CAVConstants.SOCCER_BALL_OVERLAP );
     } );
@@ -492,7 +493,7 @@
   }
 
   /**
-   * When a ball lands on the ground, animate all other balls that were at this location above the landed ball.
+   * When a ball lands on the ground, animate the ball to the top of the stack.
    */
   private animateSoccerBallStack( soccerBall: SoccerBall, value: number ): void {
     const otherObjectsInStack = this.getActiveSoccerBalls().filter( x => x.valueProperty.value === value && x !== soccerBall );
@@ -505,9 +506,7 @@
     const animationSlowdownFactor = CAVQueryParameters.slowAnimation ? 10 : 1;
     const animationTime = animationSlowdownFactor * 0.06 * this.getStackAtLocation( value ).length;
 
-    if ( soccerBall.animation ) {
-      soccerBall.animation.stop();
-    }
+    soccerBall.clearAnimation();
     soccerBall.animation = new Animation( {
       duration: animationTime,
       targets: [ {

[matthew-blackman]

Pushed commit to wrong issue:
https://github.com/phetsims/center-and-variability/commit/1cb985b56901376376bb439fc7030f1b247a977d

[matthew-blackman]

Improvements to soccerBall animationModeProperty from above commit:

• Renamed ‘animationModeProperty’ to ‘soccerBallPhaseProperty’
• Add enumeration values for INACTIVE, READY and STACKED
• Set soccerBall.isActiveProperty to a DerivedProperty based on whether soccerBallPhaseProperty is anything other than INACTIVE

@samreid and I discussed a plan to centralize the soccer ball “state” into the soccerBallPhaseProperty, to make it more maintainable to find out which part of the motion each soccer ball is currently in.

There are still z-ordering issues to resolve, but this is looking much better!

[matthew-blackman]

Bug report:
Hitting reset while soccer balls are animating can lead to scenarios like this:

[matthew-blackman]

Screen recording showing z-index issues with balls landing out-of-order:
https://github.com/phetsims/center-and-variability/assets/108756567/2686163a-eeee-4380-842d-2e8ebfa33c42

[matthew-blackman]

Patch with working copy changes for fixing z-order rearranging of soccer balls, removing median highligh from SoccerBallNode, adding median highlight to DataPointNode

Subject: [PATCH] Median highlight patch
---
Index: js/common/view/CAVPlotNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CAVPlotNode.ts b/js/common/view/CAVPlotNode.ts
--- a/js/common/view/CAVPlotNode.ts	(revision 5d8177286f7155ebed6b43978fec05cd7dda0acd)
+++ b/js/common/view/CAVPlotNode.ts	(date 1684940016214)
@@ -101,7 +101,7 @@
       // Create the data points for that scene
       scene.soccerBalls.forEach( ( soccerBall, index ) => {
 
-        const dotNode = new DataPointNode( soccerBall, ( model instanceof MeanAndMedianModel ) ? model.isTopMedianVisibleProperty : new BooleanProperty( false ), modelViewTransform, {
+        const dotNode = new DataPointNode( soccerBall, modelViewTransform, {
           tandem: options.tandem.createTandem( `scene${sceneIndex + 1}` ).createTandem( 'dataPointNodes' ).createTandem( 'dataPointNode' + index ),
           fill: options.dataPointFill
         } );
Index: js/common/view/CAVObjectNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/CAVObjectNode.ts b/js/common/view/CAVObjectNode.ts
--- a/js/common/view/CAVObjectNode.ts	(revision 5d8177286f7155ebed6b43978fec05cd7dda0acd)
+++ b/js/common/view/CAVObjectNode.ts	(date 1684940837367)
@@ -13,7 +13,6 @@
 import SoccerBall from '../model/SoccerBall.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
 import { AnimationMode } from '../model/AnimationMode.js';
-import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 import CAVColors from '../CAVColors.js';
 import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
@@ -29,11 +28,7 @@
   & PickRequired<NodeOptions, 'tandem'>;
 
 export default class CAVObjectNode extends Node {
-
-  protected readonly medianHighlight: Circle;
-
   public constructor( public readonly soccerBall: SoccerBall,
-                      isPlayAreaMedianVisibleProperty: TReadOnlyProperty<boolean>,
                       modelViewTransform: ModelViewTransform2,
                       modelRadius: number,
                       providedOptions?: CAVObjectNodeOptions ) {
@@ -43,15 +38,6 @@
     }, providedOptions );
     super( options );
 
-    const viewRadius = modelViewTransform.modelToViewDeltaX( modelRadius );
-
-    // Visibilty controlled by subclass logic. Also this whole node is moved to front when the medianHighlight is shown
-    // so it will appear in front (unless the user drags another object on top of it).
-    this.medianHighlight = new Circle( viewRadius + 1, {
-      fill: CAVColors.medianColorProperty
-    } );
-    this.addChild( this.medianHighlight );
-
     soccerBall.positionProperty.link( position => {
       this.translation = modelViewTransform.modelToViewPosition( position );
     } );
Index: js/common/view/SoccerBallNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/SoccerBallNode.ts b/js/common/view/SoccerBallNode.ts
--- a/js/common/view/SoccerBallNode.ts	(revision 5d8177286f7155ebed6b43978fec05cd7dda0acd)
+++ b/js/common/view/SoccerBallNode.ts	(date 1684940259609)
@@ -25,9 +25,9 @@
 type SelfOptions = EmptySelfOptions;
 type ParentOptions = CAVObjectNodeOptions & AccessibleSliderOptions;
 
-export default class SoccerBallNode extends AccessibleSlider( CAVObjectNode, 4 ) {
+export default class SoccerBallNode extends AccessibleSlider( CAVObjectNode, 3 ) {
 
-  public constructor( soccerBall: SoccerBall, isSceneVisibleProperty: TReadOnlyProperty<boolean>, isPlayAreaMedianVisibleProperty: TReadOnlyProperty<boolean>,
+  public constructor( soccerBall: SoccerBall, isSceneVisibleProperty: TReadOnlyProperty<boolean>,
                       modelViewTransform: ModelViewTransform2, objectNodesInputEnabledProperty: TProperty<boolean>,
                       providedOptions: CAVObjectNodeOptions ) {
 
@@ -57,7 +57,7 @@
       enabledProperty: enabledProperty
     }, providedOptions );
 
-    super( soccerBall, isPlayAreaMedianVisibleProperty, modelViewTransform, CAVObjectType.SOCCER_BALL.radius, options );
+    super( soccerBall, modelViewTransform, CAVObjectType.SOCCER_BALL.radius, options );
 
     // The dark soccer ball is used for when a ball has input disabled.
     const soccerBallNode = new Image( ball_png );
@@ -87,6 +87,7 @@
 
         // if the user presses an object that's animating, allow it to keep animating up in the stack
         soccerBall.dragStartedEmitter.emit();
+        this.moveToFront();
       },
       drag: () => {
         soccerBall.animation && soccerBall.animation.stop();
@@ -150,18 +151,9 @@
       this.visible = isActive && isSceneVisible;
     } );
 
-    // show or hide the median highlight
-    Multilink.multilink(
-      [ soccerBall.isMedianObjectProperty, isPlayAreaMedianVisibleProperty, soccerBall.isAnimationHighlightVisibleProperty ],
-      ( isMedianObject, isPlayAreaMedianVisible, isAnimationHighlightVisible ) => {
-        this.medianHighlight.visible = isPlayAreaMedianVisible && isMedianObject;
-
-        // Median highlights should be in front in z-ordering. Rather than accomplishing this via a different layer,
-        // move this to the front when it is visible.
-        if ( this.medianHighlight.visible ) {
-          this.moveToFront();
-        }
-      } );
+    soccerBall.soccerBallLandedEmitter.addListener( () => {
+      this.moveToFront();
+    } );
 
     this.addLinkedElement( soccerBall, {
       tandem: options.tandem.createTandem( 'soccerBall' )
Index: js/common/view/SceneView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/SceneView.ts b/js/common/view/SceneView.ts
--- a/js/common/view/SceneView.ts	(revision 5d8177286f7155ebed6b43978fec05cd7dda0acd)
+++ b/js/common/view/SceneView.ts	(date 1684939758755)
@@ -46,15 +46,15 @@
 
     // Keep soccer balls in one layer so we can control the focus order
     const frontLayerSoccerBallLayer = new Node();
+    const frontLayerMedianHighlightLayer = new Node( { visibleProperty: model.isPlayAreaMedianVisibleProperty } );
     const frontLayer = new Node( {
-      children: [ frontLayerSoccerBallLayer ]
+      children: [ frontLayerMedianHighlightLayer, frontLayerSoccerBallLayer ]
     } );
 
     sceneModel.soccerBalls.map( ( soccerBall, index ) => {
       const soccerBallNode = new SoccerBallNode(
         soccerBall,
         sceneModel.isVisibleProperty,
-        model.isPlayAreaMedianVisibleProperty,
         modelViewTransform,
         model.objectNodesInputEnabledProperty, {
           tandem: options.tandem.createTandem( 'soccerBallNodes' ).createTandem( `soccerBallNode${index + 1}` )
@@ -125,23 +125,23 @@
       for ( let i = 0; i < stack.length; i++ ) {
 
         const soccerBallNode = soccerBallMap.get( stack[ i ] )!;
-        const selfZIndex = soccerBallNode.parent!.indexOfChild( soccerBallNode );
-
-        let isMisordered = false;
-
-        const lowerNeighborIndex = i - 1;
-        if ( lowerNeighborIndex >= 0 ) {
-          const lowerSoccerBall = soccerBallMap.get( stack[ lowerNeighborIndex ] )!;
-          const otherZIndex = lowerSoccerBall.parent!.indexOfChild( lowerSoccerBall );
-
-          if ( selfZIndex < otherZIndex ) {
-            isMisordered = true;
-          }
-        }
-
-        if ( isMisordered ) {
-          soccerBallNode.moveToFront();
-        }
+        // const selfZIndex = soccerBallNode.parent!.indexOfChild( soccerBallNode );
+        //
+        // let isMisordered = false;
+        //
+        // const lowerNeighborIndex = i - 1;
+        // if ( lowerNeighborIndex >= 0 ) {
+        //   const lowerSoccerBall = soccerBallMap.get( stack[ lowerNeighborIndex ] )!;
+        //   const otherZIndex = lowerSoccerBall.parent!.indexOfChild( lowerSoccerBall );
+        //
+        //   if ( selfZIndex < otherZIndex ) {
+        //     isMisordered = true;
+        //   }
+        // }
+        //
+        // if ( isMisordered ) {
+        //   soccerBallNode.moveToFront();
+        // }
 
         // Only the top ball in each stack is focusable for keyboard navigation
         soccerBallNode.focusable = i === stack.length - 1;
Index: js/common/view/DataPointNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/DataPointNode.ts b/js/common/view/DataPointNode.ts
--- a/js/common/view/DataPointNode.ts	(revision 5d8177286f7155ebed6b43978fec05cd7dda0acd)
+++ b/js/common/view/DataPointNode.ts	(date 1684941014300)
@@ -12,17 +12,26 @@
 import CAVConstants from '../CAVConstants.js';
 import PlotType from '../model/PlotType.js';
 import Multilink from '../../../../axon/js/Multilink.js';
+import CAVColors from '../CAVColors.js';
 
 export default class DataPointNode extends CAVObjectNode {
 
-  public constructor( soccerBall: SoccerBall, isPlayAreaMedianVisibleProperty: TReadOnlyProperty<boolean>,
+  protected readonly medianHighlight: Circle;
+
+  public constructor( soccerBall: SoccerBall,
                       modelViewTransform: ModelViewTransform2,
                       options: CAVObjectNodeOptions & { fill: TColor } ) {
 
-    super( soccerBall, isPlayAreaMedianVisibleProperty, modelViewTransform, CAVObjectType.DATA_POINT.radius, options );
+    super( soccerBall, modelViewTransform, CAVObjectType.DATA_POINT.radius, options );
 
     const viewRadius = modelViewTransform.modelToViewDeltaX( CAVObjectType.DATA_POINT.radius );
 
+    this.medianHighlight = new Circle( viewRadius + 1, {
+      visibleProperty: soccerBall.isAnimationHighlightVisibleProperty,
+      fill: CAVColors.medianColorProperty
+    } );
+    this.addChild( this.medianHighlight );
+
     const circle = new Circle( viewRadius, {
       fill: options.fill,
       center: Vector2.ZERO
@@ -54,19 +63,6 @@
     Multilink.multilink( [ soccerBall.isActiveProperty, soccerBall.valueProperty ], ( isActive, value ) => {
       this.visible = isActive && value !== null;
     } );
-
-    // show or hide the median highlight
-    Multilink.multilink(
-      [ soccerBall.isMedianObjectProperty, isPlayAreaMedianVisibleProperty, soccerBall.isAnimationHighlightVisibleProperty ],
-      ( isMedianObject, isPlayAreaMedianVisible, isAnimationHighlightVisible ) => {
-        this.medianHighlight.visible = isAnimationHighlightVisible;
-
-        // Median highlights should be in front in z-ordering. Rather than accomplishing this via a different layer,
-        // move this to the front when it is visible.
-        if ( this.medianHighlight.visible ) {
-          this.moveToFront();
-        }
-      } );
   }
 }

[matthew-blackman]

The above commits address the scenario of while one or more balls are animating to the top of a stack, the user clicks on that stack.

74fc119 cancels animations in a stack on drag start

06f85bc fixes an issue where the landed balls were occluding click events for the stack, even though they could not be dragged themselves.

[matthew-blackman]

Screen recording: hitting 'reset all' while a ball is animating

reset.mp4

[matthew-blackman]

Commit c8b518d resolves the 'reset' and 'clear' issue in the screen recording above.

[matthew-blackman]

'If a ball lands on a stack that is currently being dragged, what should we do?'

Right now a stack that is being clicked/held behaves like a stack that is not being clicked. If the drag event updates the value of a ball, the animations of the old and new stacks are cancelled as described above. This seems good behavior to me, but warrants a design review to confirm.

[marlitas]

Is this a design question? From the comment above it seems like it might be, so marking it as such.

Just quickly playing with the sim in that use case it seems like good natural behavior.

[samreid]

Today in design meeting, we reviewed the behavior where dragging a ball across an animating stack, and the animations jump to their destinations, and we agreed it seems OK. Nice work everyone, closing.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[matthew-blackman]

I addressed the two remaining TODOs in this issue. The median highlight and all arrows appear to be working nicely. Labeling as ready-for-review.

[samreid]

@matthew-blackman and I reviewed the latter commits and also made another improvement by removing SoccerBall.isActiveProperty. Closing.