When dragging any soccer ball in the stack, it should really drag the top ball

[Nancy-Salpepi]

Test device
MacBook Air (m1 chip)

Operating System
12.3.1

Browser
Safari

Problem description
For phetsims/qa#795, when the last ball kicked lands on a number that is already occupied: If I move the "last ball" kicked, it doesn't change the value of the last card. Instead, it changes the value of first card that had that number. Shouldn't the last ball to get that value be the "top" ball?

Steps to reproduce

1. Press the kick 5 button 3 times (if the last ball kicked doesn't fall on an already occupied number, press reset and try again).
2. Move the ball underneath the cueing arrow.

Visuals

CardBallmatch.mp4

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Center and Variability‬ URL: https://phet-dev.colorado.edu/html/center-and-variability/1.0.0-rc.2/phet/center-and-variability_all_phet.html Version: 1.0.0-rc.2 2022-04-06 18:08:39 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.75 Safari/537.36 Language: en-US Window: 1431x690 Pixel Ratio: 2/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}

[samreid]

I see a comment in the code that says:

When a ball lands on the ground, animate all other balls that were at this location above the landed ball.

So it seems likely that the last ball landed at the bottom of the stack, and the other balls moved above it. @Nancy-Salpepi would this explain the behavior you reported? @amanda-phet I see how this could be confusing for users. How should we proceed for the milestone and for 1.1?

[Nancy-Salpepi]

Yes @samreid, it explains the behavior I reported. I do think it can be confusing though.

[amanda-phet]

@chrisklus and I noticed another implication of having balls land at the "bottom" of the stack when I was testing out a PhET-iO example. (If I make the first 14 balls disabled and the 15th enabled, the 15th ball gets kicked and shows up at the bottom of the stack and is very obvious because of its appearance.)

We will either need to accept this weirdness, or think of another way to animate the balls landing.

[samreid]

Should we change the rule so the ball that just landed is the only one that animates to the top, since that's what it looks like?

[catherinecarter]

I agree it might be confusing to see a different card move when it's seemingly tracking itself to a specific ball. Here's a video where I put a ball on a stack and you can see which card is moving. As long as I didn't let go of my mouse, the same card changed. But as soon as I let go of the mouse button and went back to the same stack, a different card moved. I expected the same card to move, but it didn't.

Screen.Recording.2023-04-24.at.11.10.14.AM.mov

Is there a way to attach a ball to its card to they're connected (so the same card moves when the same ball is moved)?

[samreid]

In a stack of 3 soccer balls, A,B,C (with soccer ball A at the top), if you click and drag soccer ball B, what is the expected behavior? The user drags ball B and A falls? (it looks like that is happening in the video above) Or ball A drags? Will we need to consider keyboard accessibility #162 to decide this?

[samreid]

In today's design meeting, we agreed that the top ball is always the one that is dragged from the stack. It makes no sense to drag a ball out of the center or bottom of the stack. This also makes sense for a11y--when we implement tab navigation in #162 we only want to visit/drag the topmost soccer balls. Fixing this may also solve #82

[matthew-blackman]

Thoughts on how to fix this:

Option 1: Only the top ball on each stack has an active drag listener, and the size of the hit box stretches to overlap the other balls under it in a stack.

Option 2: CAVSceneModel has a single drag listener, which is triggered by a press event on each soccer ball. The scene model handles the drag event and connects it to the top ball in the clicked stack.

Option 3: ?

@samreid / @marlitas ?

[matthew-blackman]

@samreid and I discussed this, and agree to try a solution close to Option 2 above, where the 'down' event from any soccer balls lower in the stack is forwarded to the top ball in the stack.

[samreid]

I wrote in slack:

we are wondering if we could try making the topmost ball in each stack pickable: true, and the lower soccer balls pickable: false. Or if it is really important to have the lower soccer balls pickable.

[samreid]

@catherinecarter replied:

I don’t think it makes sense to have any of the lower balls pickable. The top one is the only one that needs to be pickable, so if someone grabs any of them in the stack (meaning puts their cursor on, say, the 3rd one down), the top one is the one that moves. But I don’t think if a user wants to drag the top ball but accidentally chooses on not on the top, nothing happens. In other words, if a user touches any part of a stack, the top one should move.

I said:

Do you want to see a cursor: hand on the lower part of the stack?

@catherinecarter replied:

Maybe (?) Would it be weird to see that?

I said:

The sim already has that

@catherinecarter said:

Oh, then yes, cursor.

[samreid]

@amanda-phet said:

I think the whole stack should have a hand cursor.

[samreid]

@catherinecarter said:

I still think it’s ok to make the whole stack have a hand cursor. When a user touches any ball in the stack currently, it appears as if the top ball is moving since the stack adjusts instantly.
Currently, when a user drags any ball in the stack right now, that exact ball is moving even though it looks like the top ball is moving. This is the reason it’s confusing on the median screen when an unexpected card changes value when a ball from a single stack is moved (yes?). When a user moves any ball to a new stack, it becomes the top one. Then, if a user touches a teeny bit too low and grabs the 2nd ball down, it visually looks like they’re moving the same ball to a new stack, but in the code, it’s moving the second ball down (i.e. a different ball than was just placed there). That means a different card’s value changes when the user was expecting to see the same card they just saw change, change. If we make the entire stack the cursor and only move the top ball, the same card will change as the one just placed on top.
Am I think of thinking of this correctly? Does that make sense?

[matthew-blackman]

Patch from work @samreid and I did today on trying to get the top ball to be the only one in each stack that is pickable

Subject: [PATCH] https://github.com/phetsims/center-and-variability/issues/125
---
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 f5f4e9299e516c82db7d044f9f2a8c72db3d017e)
+++ b/js/common/view/CAVObjectNode.ts	(date 1683313273176)
@@ -33,7 +33,7 @@
 
   protected readonly medianHighlight: Circle;
 
-  public constructor( soccerBall: SoccerBall, isShowingPlayAreaMedianProperty: TReadOnlyProperty<boolean>,
+  public constructor( public readonly soccerBall: SoccerBall, isShowingPlayAreaMedianProperty: TReadOnlyProperty<boolean>,
                       modelViewTransform: ModelViewTransform2,
                       modelRadius: number,
                       providedOptions?: CAVObjectNodeOptions ) {
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 f5f4e9299e516c82db7d044f9f2a8c72db3d017e)
+++ b/js/common/view/SceneView.ts	(date 1683315823894)
@@ -18,6 +18,7 @@
 import VariabilitySceneModel from '../../variability/model/VariabilitySceneModel.js';
 import VariabilityModel from '../../variability/model/VariabilityModel.js';
 import Multilink from '../../../../axon/js/Multilink.js';
+import SoccerBall from '../model/SoccerBall.js';
 
 export default class SceneView {
 
@@ -77,7 +78,9 @@
 
     model.soccerBallHasBeenDraggedProperty.link( updateDragIndictatorVisible );
 
-    sceneModel.soccerBalls.forEach( ( soccerBall, index ) => {
+    const soccerBallMap = new Map<SoccerBall, SoccerBallNode>();
+
+    const soccerBallNodes = sceneModel.soccerBalls.map( ( soccerBall, index ) => {
       const soccerBallNode = new SoccerBallNode(
         soccerBall,
         sceneModel.isVisibleProperty,
@@ -112,6 +115,29 @@
           model.soccerBallHasBeenDraggedProperty.value = true;
         }
       } );
+
+      soccerBallMap.set( soccerBall, soccerBallNode );
+
+      return soccerBallNode;
+    } );
+
+    soccerBallNodes.forEach( soccerBallNode => {
+      soccerBallNode.soccerBall.valueProperty.lazyLink( ( value, oldValue ) => {
+
+        soccerBallNodes.forEach( soccerBallNode => {
+          soccerBallNode.pickable = false;
+        } );
+
+        if ( oldValue !== null ) {
+          const oldStack = sceneModel.getStackAtLocation( oldValue );
+          soccerBallMap.get( oldStack[ oldStack.length - 1 ] )!.pickable = true;
+        }
+
+        if ( value !== null ) {
+          const newStack = sceneModel.getStackAtLocation( value );
+          soccerBallMap.get( newStack[ newStack.length - 1] )!.pickable = true;
+        }
+      } );
     } );
 
     const playAreaMedianIndicatorNode = new PlayAreaMedianIndicatorNode();
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 f5f4e9299e516c82db7d044f9f2a8c72db3d017e)
+++ b/js/common/model/CAVSceneModel.ts	(date 1683315787092)
@@ -242,6 +242,16 @@
     } );
   }
 
+  /**
+   * Returns all objects at the target location
+   */
+  public getStackAtLocation( location: number ): SoccerBall[] {
+    return this.getActiveSoccerBalls().filter( ( o: SoccerBall ) => {
+      return o.valueProperty.value === location;
+    } ).sort( ( objectA, objectB ) =>
+      objectA.positionProperty.value.y - objectB.positionProperty.value.y );
+  }
+
   /**
    * Set the position of the parameter object to be on top of the other objects at that target position.
    */
@@ -292,7 +302,7 @@
   public getSortedLandedObjects(): SoccerBall[] {
     return _.sortBy( this.getActiveSoccerBalls().filter( soccerBall => soccerBall.valueProperty.value !== null ),
 
-      // The numerical value takes predence for sorting
+      // The numerical value takes precedence for sorting
       soccerBall => soccerBall.valueProperty.value,
 
       // Then consider the height within the stack

[samreid]

@amanda-phet and @catherinecarter don't worry about the preceding comment. Just a stepping stone to get toward the functionality you described.

[samreid]

Here is some more progress in that direction, still not working well though:

Subject: [PATCH] Add TODO reference, see https://github.com/phetsims/center-and-variability/issues/45
---
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 96e0542851d4706e1bb33dd31f6e56fb2f6d9d54)
+++ b/js/common/model/SoccerBall.ts	(date 1683317599748)
@@ -37,6 +37,8 @@
   & PhetioObjectOptions
   & PickRequired<PhetioObjectOptions, 'tandem'>;
 
+let count = 0;
+
 export default class SoccerBall {
 
   // Continuous value for the drag listener. When dragging, the object snaps to each tickmark
@@ -63,6 +65,9 @@
   public readonly isActiveProperty: BooleanProperty;
   public soccerPlayer: SoccerPlayer | null = null;
 
+  // Global index for debugging
+  public readonly index = count++;
+
   public constructor( providedOptions: CAVObjectOptions ) {
 
     const options = optionize<CAVObjectOptions, SelfOptions, PhetioObjectOptions>()( {
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 96e0542851d4706e1bb33dd31f6e56fb2f6d9d54)
+++ b/js/common/view/SceneView.ts	(date 1683318219080)
@@ -18,6 +18,9 @@
 import VariabilitySceneModel from '../../variability/model/VariabilitySceneModel.js';
 import VariabilityModel from '../../variability/model/VariabilityModel.js';
 import Multilink from '../../../../axon/js/Multilink.js';
+import SoccerBall from '../model/SoccerBall.js';
+import { Shape } from '../../../../kite/js/imports.js';
+import Bounds2 from '../../../../dot/js/Bounds2.js';
 
 export default class SceneView {
 
@@ -77,7 +80,9 @@
 
     model.soccerBallHasBeenDraggedProperty.link( updateDragIndictatorVisible );
 
-    sceneModel.soccerBalls.forEach( ( soccerBall, index ) => {
+    const soccerBallMap = new Map<SoccerBall, SoccerBallNode>();
+
+    const soccerBallNodes = sceneModel.soccerBalls.map( ( soccerBall, index ) => {
       const soccerBallNode = new SoccerBallNode(
         soccerBall,
         sceneModel.isVisibleProperty,
@@ -112,6 +117,51 @@
           model.soccerBallHasBeenDraggedProperty.value = true;
         }
       } );
+
+      soccerBallMap.set( soccerBall, soccerBallNode );
+
+      return soccerBallNode;
+    } );
+
+    const updateStackPickability = ( stack: SoccerBall[] ) => {
+      // TODO: https://github.com/phetsims/center-and-variability/issues/125 remove debug code
+      // console.log( 'visiting stack at: ' + stack.map( soccerBall => soccerBall.valueProperty.value ) );
+      // console.log( 'stack = ' + stack.map( soccerBall => soccerBall.index ) );
+
+      let bounds: Bounds2 | null = null;
+
+      for ( let i = 0; i < stack.length; i++ ) {
+
+        const soccerBallNode = soccerBallMap.get( stack[ i ] )!;
+        soccerBallNode.pickable = i === stack.length - 1;
+
+        if ( i === 0 ) {
+          bounds = soccerBallNode.globalBounds;
+        }
+        else {
+          bounds!.includeBounds( soccerBallNode.globalBounds );
+        }
+
+        if ( i === stack.length - 1 ) {
+          soccerBallNode.mouseArea = Shape.bounds( soccerBallNode.globalToLocalBounds( bounds! ) );
+          soccerBallNode.touchArea = Shape.bounds( soccerBallNode.globalToLocalBounds( bounds! ) );
+        }
+        else {
+          soccerBallNode.mouseArea = Shape.rectangle( 0, 0, 0, 0 );
+          soccerBallNode.touchArea = Shape.rectangle( 0, 0, 0, 0 );
+        }
+      }
+    };
+
+    soccerBallNodes.forEach( soccerBallNode => {
+      soccerBallNode.soccerBall.valueProperty.lazyLink( () => {
+
+        setTimeout( () => {
+          for ( let i = 0; i <= 15; i++ ) {
+            updateStackPickability( sceneModel.getStackAtLocation( i ) );
+          }
+        }, 0 );
+      } );
     } );
 
     const playAreaMedianIndicatorNode = new PlayAreaMedianIndicatorNode();
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 96e0542851d4706e1bb33dd31f6e56fb2f6d9d54)
+++ b/js/common/model/CAVSceneModel.ts	(date 1683316070732)
@@ -242,6 +242,16 @@
     } );
   }
 
+  /**
+   * Returns all objects at the target location
+   */
+  public getStackAtLocation( location: number ): SoccerBall[] {
+    return this.getActiveSoccerBalls().filter( ( o: SoccerBall ) => {
+      return o.valueProperty.value === location;
+    } ).sort( ( objectA, objectB ) =>
+      objectA.positionProperty.value.y - objectB.positionProperty.value.y );
+  }
+
   /**
    * Set the position of the parameter object to be on top of the other objects at that target position.
    */
@@ -292,7 +302,7 @@
   public getSortedLandedObjects(): SoccerBall[] {
     return _.sortBy( this.getActiveSoccerBalls().filter( soccerBall => soccerBall.valueProperty.value !== null ),
 
-      // The numerical value takes predence for sorting
+      // The numerical value takes precedence for sorting
       soccerBall => soccerBall.valueProperty.value,
 
       // Then consider the height within the stack
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 96e0542851d4706e1bb33dd31f6e56fb2f6d9d54)
+++ b/js/common/view/CAVObjectNode.ts	(date 1683317543983)
@@ -26,14 +26,11 @@
   & StrictOmit<NodeOptions, 'inputEnabledProperty'>
   & PickRequired<NodeOptions, 'tandem'>;
 
-// for debugging with ?dev
-let index = 0;
-
 export default abstract class CAVObjectNode extends Node {
 
   protected readonly medianHighlight: Circle;
 
-  public constructor( soccerBall: SoccerBall, isShowingPlayAreaMedianProperty: TReadOnlyProperty<boolean>,
+  public constructor( public readonly soccerBall: SoccerBall, isShowingPlayAreaMedianProperty: TReadOnlyProperty<boolean>,
                       modelViewTransform: ModelViewTransform2,
                       modelRadius: number,
                       providedOptions?: CAVObjectNodeOptions ) {
@@ -65,7 +62,7 @@
 
     // Show index when debugging with ?dev
     if ( phet.chipper.queryParameters.dev ) {
-      this.addChild( new Text( index++ + '', {
+      this.addChild( new Text( soccerBall.index + '', {
         font: new PhetFont( 14 ),
         fill: 'red',
         x: this.width / 2 + 1

[samreid]

Fixed and ready for review. @matthew-blackman can you please review since you are more familiar with the issue?

It looks like this now:

And keep in mind #82 is still buggy independently of this issue.

[matthew-blackman]

I tested the solution @samreid committed above and the behavior looks correct. Nice job! The median highlight and ball animations also appear to be working correctly.

Looking through the above commits, the implementation seems to make sense. I have the following questions/feedback:

• I noticed that 'restackWithTopBall' was being called very frequently upon drag events, even when the dragged ball's position was not changing. I was able to reduce the number of calls by adding the following guard in the constructor of CAVSceneModel.ts, line 108:

soccerBall.dragPositionProperty.lazyLink( ( dragPosition: Vector2 ) => {
        const oldValue = soccerBall.valueProperty.value;
        const newValue = Utils.roundSymmetric( CAVConstants.PHYSICAL_RANGE.constrainValue( dragPosition.x ) );
        if ( oldValue !== newValue ) {
          soccerBall.valueProperty.value = newValue;
          this.restackWithTopBall( soccerBall );
        }
      } );

• I am not sure if the following code is needed in CAVSceneModel.ts. When I comment it out the behavior of the sim does not appear to change.

soccerBall.valueProperty.lazyLink( ( value, oldValue ) => {
        if ( value !== null && !phet.joist.sim.isSettingPhetioStateProperty.value && oldValue !== null ) {
          const stack = this.getStackAtLocation( oldValue );
          if ( stack.length > 0 ) {
            this.restackWithTopBall( stack[ stack.length - 1 ] );
          }
        }
      } );

Since stackChangedEmitter is already causing updateStackPointerAreas to check each stack and update the pointer areas based on the ball y-position, I think this additional call to restackWithTopBall may be redundant. Seeing isSettingPhetioStateProperty is making me wonder if this is a PhET-iO consideration. Thoughts @samreid?

[matthew-blackman]

Question for PhET-iO design meeting:

If the position of a ball is changed via PhET-iO, do we want to move that ball to the top of the stack (the same as if it was dragged there)?

[matthew-blackman]

@samreid I noticed some redundancy in how often functions like updateStackPointerAreas are being called. In SceneView, the following listeners are applied:

soccerBallNodes.forEach( soccerBallNode => soccerBallNode.soccerBall.valueProperty.lazyLink( updateStackPointerAreas ) );
sceneModel.stackChangedEmitter.addListener( updateStackPointerAreas );

It looks like the stackChangedEmitter is being used to update the pointer area when the stacking animation has finished, but wouldn't the pointer area update as soon as the ball lands anyway, because its valueProperty had updated?

The above commit removes one extra call of updateStackPointerAreas, but I think this can be optimized further. Could it be related to #186 via performance issues? @samreid @marlitas

[samreid]

@matthew-blackman and I made commits in #186 to reduce redundancy and improve performance.

[samreid]

I also have a patch in-progress that makes the stackChangedEmitter take a stack that currently changed, instead of redrawing all of them.

[samreid]

@matthew-blackman and I worked out the stack variable in stackChangedEmitter as part of #175. @matthew-blackman can you please re-evaluate this issue and see what remains to do?

[matthew-blackman]

Grabbing and moving the top soccer ball in each stack appears to be working well, and the code changes make sense to me. This seems ready to close. We can continue dealing with corner-cases in #188

[marlitas]

Closing as completed.