Refactor soccer ball z-index reordering, move median highlight into D…

…ataPointNode and remove median highlight from SoccerBallNode - see #188
phetsims · May 24, 2023 · 276a588 · 276a588
1 parent 5d81772
commit 276a588
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 67 deletions.
diff --git a/js/common/view/CAVObjectNode.ts b/js/common/view/CAVObjectNode.ts
@@ -9,12 +9,10 @@
 
 import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
 import centerAndVariability from '../../centerAndVariability.js';
-import { Circle, Node, NodeOptions, Text } from '../../../../scenery/js/imports.js';
+import { Node, NodeOptions, Text } from '../../../../scenery/js/imports.js';
 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';
 import Multilink from '../../../../axon/js/Multilink.js';
@@ -29,11 +27,7 @@ export type CAVObjectNodeOptions =
   & 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 +37,6 @@ export default class CAVObjectNode extends Node {
     }, 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 );
     } );

diff --git a/js/common/view/CAVPlotNode.ts b/js/common/view/CAVPlotNode.ts
@@ -101,7 +101,7 @@ export default class CAVPlotNode extends Node {
       // 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
         } );

diff --git a/js/common/view/DataPointNode.ts b/js/common/view/DataPointNode.ts
@@ -3,7 +3,6 @@
 import CAVObjectNode, { CAVObjectNodeOptions } from './CAVObjectNode.js';
 import centerAndVariability from '../../centerAndVariability.js';
 import SoccerBall from '../model/SoccerBall.js';
-import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
 import CAVObjectType from '../model/CAVObjectType.js';
 import { Circle, Node, Path, TColor } from '../../../../scenery/js/imports.js';
@@ -12,17 +11,26 @@ import timesSolidShape from '../../../../sherpa/js/fontawesome-5/timesSolidShape
 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 +62,6 @@ export default class DataPointNode extends CAVObjectNode {
     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();
-        }
-      } );
   }
 }
 

diff --git a/js/common/view/SceneView.ts b/js/common/view/SceneView.ts
@@ -46,15 +46,15 @@ export default class SceneView {
 
     // 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,6 @@ export default class SceneView {
       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();
-        }
 
         // Only the top ball in each stack is focusable for keyboard navigation
         soccerBallNode.focusable = i === stack.length - 1;

diff --git a/js/common/view/SoccerBallNode.ts b/js/common/view/SoccerBallNode.ts
@@ -25,9 +25,9 @@ import NumberTone from '../model/NumberTone.js';
 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 @@ export default class SoccerBallNode extends AccessibleSlider( CAVObjectNode, 4 )
       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 @@ export default class SoccerBallNode extends AccessibleSlider( CAVObjectNode, 4 )
 
         // 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 @@ export default class SoccerBallNode extends AccessibleSlider( CAVObjectNode, 4 )
       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' )