phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

White dot may disappear in downstream sim #268

Closed Nancy-Salpepi closed 6 months ago

Nancy-Salpepi commented 6 months ago

Test device MacBook Air M1 chip and iPad 9th generation

Operating System iOS 14.3.1 and iPadOS 17

Browser Safari and chrome

Problem description For https://github.com/phetsims/qa/issues/1060, in the State wrapper on the first 3 screens only-- the white dot to indicate the last projectile launched disappears in the downstream sim with the set state rate at its default value. I don't see this issue, if set state rate = 0 and then I press the Set State Now button. I also don't see this issue on the Sampling Screen.

Steps to reproduce

  1. In the state wrapper, go to any of the first 3 screens
  2. Press the launch button a few times in the upstream sim--the white dot will appear and then disappear in the downstream sim.
  3. In the upstream sim, use the decrement button to select another launched projectile--the white dot will appear and then disappear in the downstream sim.

Visuals

https://github.com/phetsims/projectile-data-lab/assets/87318828/13b48134-6a8b-4420-8b50-8c35647c147a

samreid commented 6 months ago

I can reproduce this problem. The dot gets set correctly on the 1st state set. But on the 2nd state set, it disappears.

samreid commented 6 months ago

This patch works around the problem. It identifies that the root of the problem is reference vs value identity in the selected projectile. Not sure if we should add an equals function or if we need to go upstream fix the reference:

```diff Subject: [PATCH] Only drop circuit elements or tools if the toolbox is visible, see https://github.com/phetsims/circuit-construction-kit-common/issues/999 --- Index: js/common/view/HistogramRepresentationIconNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/HistogramRepresentationIconNode.ts b/js/common/view/HistogramRepresentationIconNode.ts --- a/js/common/view/HistogramRepresentationIconNode.ts (revision 2f3c1652a4f93fe6c5b3241d1fcc7d3307d4ecfc) +++ b/js/common/view/HistogramRepresentationIconNode.ts (date 1711980280477) @@ -11,7 +11,7 @@ import ChartTransform from '../../../../bamboo/js/ChartTransform.js'; import Range from '../../../../dot/js/Range.js'; import ChartCanvasNode from '../../../../bamboo/js/ChartCanvasNode.js'; -import { ColorProperty, Node } from '../../../../scenery/js/imports.js'; +import { ColorProperty, Node, Text } from '../../../../scenery/js/imports.js'; import NumberProperty from '../../../../axon/js/NumberProperty.js'; import { HistogramRepresentation } from '../model/HistogramRepresentation.js'; import Property from '../../../../axon/js/Property.js'; @@ -22,28 +22,28 @@ pickable: true } ); - const chartTransform = new ChartTransform( { - viewWidth: 23, - viewHeight: 23, - modelXRange: new Range( 0, 3 ), - modelYRange: new Range( 0, 3 ) - } ); - const histogramPainter = new HistogramCanvasPainter( - null, - chartTransform, - new NumberProperty( 1 ), - new Property( histogramRepresentation ), - blockFillProperty, blockStrokeProperty - ); - const data = [ { x: 0 }, { x: 0 }, { x: 1 }, { x: 1 }, { x: 1 }, { x: 2 } ]; - histogramPainter.setHistogramData( data, data[ 1 ] ); - const chartCanvasNode = new ChartCanvasNode( chartTransform, [ histogramPainter ] ); + // const chartTransform = new ChartTransform( { + // viewWidth: 23, + // viewHeight: 23, + // modelXRange: new Range( 0, 3 ), + // modelYRange: new Range( 0, 3 ) + // } ); + // const histogramPainter = new HistogramCanvasPainter( + // null, + // chartTransform, + // new NumberProperty( 1 ), + // new Property( histogramRepresentation ), + // blockFillProperty, blockStrokeProperty + // ); + // const data = [ { x: 0 }, { x: 0 }, { x: 1 }, { x: 1 }, { x: 1 }, { x: 2 } ]; + // histogramPainter.setHistogramData( data, data[ 1 ] ); + // const chartCanvasNode = new ChartCanvasNode( chartTransform, [ histogramPainter ] ); - this.addChild( chartCanvasNode ); + this.addChild( new Text( histogramRepresentation ) ); - const area = chartCanvasNode.localBounds.dilatedXY( 2, 10 ); - this.setTouchArea( area ); - this.setMouseArea( area ); + // const area = chartCanvasNode.localBounds.dilatedXY( 2, 10 ); + // this.setTouchArea( area ); + // this.setMouseArea( area ); } } Index: js/common/view/HistogramCanvasPainter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/HistogramCanvasPainter.ts b/js/common/view/HistogramCanvasPainter.ts --- a/js/common/view/HistogramCanvasPainter.ts (revision 2f3c1652a4f93fe6c5b3241d1fcc7d3307d4ecfc) +++ b/js/common/view/HistogramCanvasPainter.ts (date 1711980713007) @@ -44,6 +44,8 @@ public setHistogramData( data: HistogramData[], selectedData: HistogramData | null ): void { this.data = data; this.selectedData = selectedData; + + console.log( 'setHistogramData called, selected Data = ', selectedData ); } public paintCanvas( context: CanvasRenderingContext2D ): void { @@ -81,9 +83,11 @@ let highlightDotX = null; let highlightDotY = null; + console.log( this.data.length ); + for ( let i = 0; i < this.data.length; i++ ) { const projectile = this.data[ i ]; - const isHighlighted = this.selectedData === projectile; + const isHighlighted = this.selectedData && this.selectedData.timeAirborne === projectile.timeAirborne; // Calculate the bin for this value by its lower bound const bin = Math.floor( projectile.x / binWidth ) * binWidth; @@ -102,12 +106,15 @@ } // If the highlighted data block is in the sonified bin, don't draw the dot + console.log( 'isHighlighted = ', isHighlighted, 'isSonifiedBin = ', isSonifiedBin( bin ) ); if ( isHighlighted && !isSonifiedBin( bin ) ) { highlightDotX = x; highlightDotY = y; } } + console.log( histogramRepresentation, highlightDotX, highlightDotY ); + if ( histogramRepresentation === 'bars' ) { for ( const [ bin, count ] of histogram ) { @@ -127,6 +134,8 @@ // draw a white dot in the middle of the highlighted block, in front of the all the data blocks context.beginPath(); context.arc( highlightDotX * scaleFactor + blockWidth * scaleFactor / 2, highlightDotY * scaleFactor + blockHeight * scaleFactor / 2, DOT_RADIUS * scaleFactor, 0, 2 * Math.PI ); + console.log( 'highlightDotX = ', highlightDotX, 'highlightDotY = ', highlightDotY ); + console.log( 'blockWidth = ', blockWidth, 'blockHeight = ', blockHeight ); context.fillStyle = 'white'; context.strokeStyle = this.blockFillProperty.value.toCSS(); context.lineWidth = scaleFactor; ```
samreid commented 6 months ago

I added a workaround to ensure the reference remains valid (by reference and not just by value) after phet-io state set. @matthew-blackman can you please review?

samreid commented 6 months ago

@matthew-blackman and I found a better solution and it is working well in our testing. Closing.