Reset All delayed in sim launched with many kicked balls

[Nancy-Salpepi]

Test device MacBook Air M1 chip

Operating System 14.0

Browser all browsers

Problem description For https://github.com/phetsims/qa/issues/993, launching the sim with all balls kicked for the players on the Variability screen, results in a delay when Reset All is pressed. This was noticeable on my mac with 20 balls kicked for each player. I then tried in on an older computer (Dell with Intel Core i7-6700HQ CPU @ 2.60GHz) and the delay was longer.

Steps to reproduce

1. In Studio, change the number of Max Kicks to 20.
2. On the Variability screen, kick all the balls for all 4 players
3. Press Test
4. In the Standard PhET-iO wrapper, select a different player
5. Press Reset All

Visuals Video using the Dell:

https://github.com/phetsims/center-and-variability/assets/87318828/6773a45c-2359-4d16-96dc-7167778abb77

[marlitas]

According to the performance test it is taking 2.05 seconds when I have maxKicks = 30, and all the balls are kicked across all 4 players.

[marlitas]

Exploring the performance breakdown and also guessing from prior performance issues, I think there's a lot of redrawing happening to state setting... This is another tricky one I'll need help with.

[zepumph]

This is really weird. My performance profiling results were less fruitful than yours. All I got was a "pointer-up" even taking up all 3 seconds.

I'll try restarting my browser to see if that helps. In the mean time I think probably sync pairing would be best.

[samreid]

Current mishmash patch from collaboration with @marlitas

```diff Subject: [PATCH] Rename method, see https://github.com/phetsims/phet-io-wrappers/issues/572 --- Index: center-and-variability/js/variability/model/VariabilitySceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/js/variability/model/VariabilitySceneModel.ts b/center-and-variability/js/variability/model/VariabilitySceneModel.ts --- a/center-and-variability/js/variability/model/VariabilitySceneModel.ts (revision f2e9ad5446601f2ce0afd8689e6997a572f8e0a3) +++ b/center-and-variability/js/variability/model/VariabilitySceneModel.ts (date 1697579310273) @@ -24,6 +24,8 @@ import { KickDistributionStrategySpecification } from '../../../../soccer-common/js/model/KickDistributionStrategy.js'; import Tandem from '../../../../tandem/js/Tandem.js'; import { TColor } from '../../../../scenery/js/imports.js'; +import isResettingProperty from '../../../../soccer-common/js/model/isResettingProperty.js'; +import isSettingPhetioStateProperty from '../../../../tandem/js/isSettingPhetioStateProperty.js'; export default class VariabilitySceneModel extends CAVSoccerSceneModel { @@ -153,6 +155,10 @@ protected override updateDataMeasures(): void { super.updateDataMeasures(); + if ( this.isClearingData || isSettingPhetioStateProperty.value ) { + return; + } + if ( this.initialized ) { const sortedObjects = this.getSortedStackedObjects(); Index: soccer-common/js/model/isResettingProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/soccer-common/js/model/isResettingProperty.ts b/soccer-common/js/model/isResettingProperty.ts --- a/soccer-common/js/model/isResettingProperty.ts (revision dd49cb15a1e750329fccf55980bc4fc1962afab7) +++ b/soccer-common/js/model/isResettingProperty.ts (date 1697578443445) @@ -9,4 +9,6 @@ * @author Marla Schulz (PhET Interactive Simulations) */ const isResettingProperty = new BooleanProperty( false ); + +window.isResettingProperty = isResettingProperty; export default isResettingProperty; \ No newline at end of file Index: soccer-common/js/model/SoccerSceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/soccer-common/js/model/SoccerSceneModel.ts b/soccer-common/js/model/SoccerSceneModel.ts --- a/soccer-common/js/model/SoccerSceneModel.ts (revision dd49cb15a1e750329fccf55980bc4fc1962afab7) +++ b/soccer-common/js/model/SoccerSceneModel.ts (date 1697578892330) @@ -50,6 +50,7 @@ import Tandem from '../../../tandem/js/Tandem.js'; import PickRequired from '../../../phet-core/js/types/PickRequired.js'; import optionize from '../../../phet-core/js/optionize.js'; +import isResettingProperty from './isResettingProperty.js'; type SelfOptions = { isSingleKickerScene?: boolean; @@ -247,7 +248,7 @@ // Signal to listeners that a value changed, but batch notifications during reset const guardedEmit = () => { - if ( !this.isClearingData ) { + if ( !this.isClearingData && !isResettingProperty.value ) { this.objectChangedEmitter.emit(); } }; @@ -341,6 +342,15 @@ maxKicksProperty.lazyLink( () => this.clearData() ); regionAndCultureProperty.lazyLink( () => this.clearData() ); + + // isResettingProperty.lazyLink( isResetting => { + // if ( !isResetting ) { + // this.objectChangedEmitter.emit(); + // } + // } ); + this.objectChangedEmitter.addListener( () => { + console.log( 'object changed' ); + } ); } // Cancel out all animations in the soccer ball stack. @@ -356,7 +366,7 @@ } protected updateDataMeasures(): void { - if ( this.isClearingData ) { + if ( this.isClearingData || isResettingProperty.value ) { return; } Index: scenery-phet/js/buttons/ResetAllButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/scenery-phet/js/buttons/ResetAllButton.ts b/scenery-phet/js/buttons/ResetAllButton.ts --- a/scenery-phet/js/buttons/ResetAllButton.ts (revision 1d4ce8d107782f4ece45b3fb8ca8e5a44594f3cd) +++ b/scenery-phet/js/buttons/ResetAllButton.ts (date 1697578443461) @@ -90,7 +90,9 @@ // even though this is Tandem.REQUIRED, still be graceful if not yet instrumented this.isPhetioInstrumented() ) { + window.isResettingProperty.value = true; phet.phetio.phetioEngine.phetioStateEngine.restoreStateForScreen( options.tandem ); + window.isResettingProperty.value = false; } // restore the enabled state to each utteranceQueue after resetting Index: center-and-variability/js/common/model/CAVSoccerSceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/js/common/model/CAVSoccerSceneModel.ts b/center-and-variability/js/common/model/CAVSoccerSceneModel.ts --- a/center-and-variability/js/common/model/CAVSoccerSceneModel.ts (revision f2e9ad5446601f2ce0afd8689e6997a572f8e0a3) +++ b/center-and-variability/js/common/model/CAVSoccerSceneModel.ts (date 1697579053659) @@ -20,6 +20,7 @@ import RegionAndCulturePortrayal from '../../../../joist/js/preferences/RegionAndCulturePortrayal.js'; import { KickDistributionStrategySpecification } from '../../../../soccer-common/js/model/KickDistributionStrategy.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import isResettingProperty from '../../../../soccer-common/js/model/isResettingProperty.js'; export default class CAVSoccerSceneModel extends SoccerSceneModel { @@ -61,7 +62,7 @@ super.updateDataMeasures(); - if ( this.isClearingData ) { + if ( this.isClearingData || isResettingProperty.value ) { return; } ```

[marlitas]

I worked on this a bit more and found that the patch above introduced a bug into the MAD calculations in the state wrapper. This to me signals that there is some inconsistency throughout the sim as to when and how these data updates are triggered, but I think that is way beyond our scope at this point. This patch seems to be working well, and I was also able to cut the time it took clearData to run by half. I would like to go over it one more time with @samreid though before committing.

```diff Subject: [PATCH] Reset performance enhancement --- Index: center-and-variability/js/variability/model/VariabilitySceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/js/variability/model/VariabilitySceneModel.ts b/center-and-variability/js/variability/model/VariabilitySceneModel.ts --- a/center-and-variability/js/variability/model/VariabilitySceneModel.ts (revision 4eabcb8578167cbff48ca1a4c377c86ab4ba4da9) +++ b/center-and-variability/js/variability/model/VariabilitySceneModel.ts (date 1697584627058) @@ -24,6 +24,7 @@ import { KickDistributionStrategySpecification } from '../../../../soccer-common/js/model/KickDistributionStrategy.js'; import Tandem from '../../../../tandem/js/Tandem.js'; import { TColor } from '../../../../scenery/js/imports.js'; +import isSettingPhetioStateProperty from '../../../../tandem/js/isSettingPhetioStateProperty.js'; export default class VariabilitySceneModel extends CAVSoccerSceneModel { @@ -134,6 +135,10 @@ this.updateDataMeasures(); this.initialized = true; + + isSettingPhetioStateProperty.lazyLink( isSettingState => { + !isSettingState && this.updateDataMeasures(); + } ); } public getDeviationForBallValue( value: number ): number { @@ -153,6 +158,10 @@ protected override updateDataMeasures(): void { super.updateDataMeasures(); + if ( this.isClearingData || isSettingPhetioStateProperty.value ) { + return; + } + if ( this.initialized ) { const sortedObjects = this.getSortedStackedObjects(); Index: center-and-variability/js/common/model/CAVSoccerSceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/center-and-variability/js/common/model/CAVSoccerSceneModel.ts b/center-and-variability/js/common/model/CAVSoccerSceneModel.ts --- a/center-and-variability/js/common/model/CAVSoccerSceneModel.ts (revision 4eabcb8578167cbff48ca1a4c377c86ab4ba4da9) +++ b/center-and-variability/js/common/model/CAVSoccerSceneModel.ts (date 1697583530396) @@ -20,6 +20,8 @@ import RegionAndCulturePortrayal from '../../../../joist/js/preferences/RegionAndCulturePortrayal.js'; import { KickDistributionStrategySpecification } from '../../../../soccer-common/js/model/KickDistributionStrategy.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import isResettingProperty from '../../../../soccer-common/js/model/isResettingProperty.js'; +import isSettingPhetioStateProperty from '../../../../tandem/js/isSettingPhetioStateProperty.js'; export default class CAVSoccerSceneModel extends SoccerSceneModel { @@ -61,7 +63,7 @@ super.updateDataMeasures(); - if ( this.isClearingData ) { + if ( this.isClearingData || isResettingProperty.value || isSettingPhetioStateProperty.value ) { return; } Index: soccer-common/js/model/SoccerSceneModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/soccer-common/js/model/SoccerSceneModel.ts b/soccer-common/js/model/SoccerSceneModel.ts --- a/soccer-common/js/model/SoccerSceneModel.ts (revision db6984b5c0089834b6ca38dd377514cc231b5691) +++ b/soccer-common/js/model/SoccerSceneModel.ts (date 1697584479295) @@ -50,6 +50,8 @@ import Tandem from '../../../tandem/js/Tandem.js'; import PickRequired from '../../../phet-core/js/types/PickRequired.js'; import optionize from '../../../phet-core/js/optionize.js'; +import isResettingProperty from './isResettingProperty.js'; +import isSettingPhetioStateProperty from '../../../tandem/js/isSettingPhetioStateProperty.js'; type SelfOptions = { isSingleKickerScene?: boolean; @@ -247,7 +249,7 @@ // Signal to listeners that a value changed, but batch notifications during reset const guardedEmit = () => { - if ( !this.isClearingData ) { + if ( !this.isClearingData && !isResettingProperty.value && !isSettingPhetioStateProperty.value ) { this.objectChangedEmitter.emit(); } }; @@ -325,7 +327,7 @@ soccerBall.valueProperty.lazyLink( () => { // update data measures if the ball is being dragged/moved to a new position after landing/stacking - if ( soccerBall.soccerBallPhaseProperty.value === SoccerBallPhase.STACKED ) { + if ( soccerBall.soccerBallPhaseProperty.value === SoccerBallPhase.STACKED && !isSettingPhetioStateProperty.value ) { updateDataMeasures(); } } ); @@ -333,7 +335,7 @@ // update data measures when the ball finished its stacking animation or if a ball has been removed from // the data set through state setting. - if ( newPhase === SoccerBallPhase.STACKED || oldPhase === SoccerBallPhase.STACKED ) { + if ( ( newPhase === SoccerBallPhase.STACKED || oldPhase === SoccerBallPhase.STACKED ) && !isSettingPhetioStateProperty.value ) { updateDataMeasures(); } } ); @@ -356,7 +358,7 @@ } protected updateDataMeasures(): void { - if ( this.isClearingData ) { + if ( this.isClearingData || isResettingProperty.value || isSettingPhetioStateProperty.value ) { return; } @@ -439,7 +441,6 @@ this.activeKickIndexProperty.reset(); this.isClearingData = false; - this.updateDataMeasures(); // This emitter was suppressed during isClearingData, so we must synchronize listeners now this.objectChangedEmitter.emit(); ```

[Nancy-Salpepi]

I'm not seeing a delay in rc.3 with MacBook/Dell.