From 276a5884c1be03c70dfd4a7774fc7df94ecb01db Mon Sep 17 00:00:00 2001 From: matthewblackman Date: Wed, 24 May 2023 11:22:26 -0400 Subject: [PATCH] Refactor soccer ball z-index reordering, move median highlight into DataPointNode and remove median highlight from SoccerBallNode - see https://github.com/phetsims/center-and-variability/issues/188 --- js/common/view/CAVObjectNode.ts | 17 +---------------- js/common/view/CAVPlotNode.ts | 2 +- js/common/view/DataPointNode.ts | 27 +++++++++++---------------- js/common/view/SceneView.ts | 21 ++------------------- js/common/view/SoccerBallNode.ts | 22 +++++++--------------- 5 files changed, 22 insertions(+), 67 deletions(-) diff --git a/js/common/view/CAVObjectNode.ts b/js/common/view/CAVObjectNode.ts index 0f56c9bc..07d0fcbe 100644 --- 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; export default class CAVObjectNode extends Node { - - protected readonly medianHighlight: Circle; - public constructor( public readonly soccerBall: SoccerBall, - isPlayAreaMedianVisibleProperty: TReadOnlyProperty, 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 index a7978d6f..1db30541 100644 --- 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 index 5eee877b..62b47cc9 100644 --- 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, + 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 index 85594e0f..506a73aa 100644 --- 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 index b53b227a..b3d7b31b 100644 --- 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, isPlayAreaMedianVisibleProperty: TReadOnlyProperty, + public constructor( soccerBall: SoccerBall, isSceneVisibleProperty: TReadOnlyProperty, modelViewTransform: ModelViewTransform2, objectNodesInputEnabledProperty: TProperty, 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' )