Skip to content

Commit

Permalink
remove preloading and activation of challenges, the performance impro…
Browse files Browse the repository at this point in the history 
…vement is no longer necessary, #78
  • Loading branch information
@pixelzoom
pixelzoom committed Mar 3, 2023
1 parent 6b6c5d3 commit c37d19b
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 109 deletions.
24 changes: 5 additions & 19 deletions js/game/view/ChallengeNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
import Multilink from '../../../../axon/js/Multilink.js';
import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
import Bounds2 from '../../../../dot/js/Bounds2.js';
Expand Down Expand Up @@ -48,7 +47,6 @@ type ChallengeNodeOptions = SelfOptions;
export default class ChallengeNode extends Node {

private readonly checkButtonEnabledProperty: TReadOnlyProperty<boolean>;
private playStateProperty: EnumerationProperty<PlayState> | null; // set by activate()
private readonly playStateObserver: ( playState: PlayState ) => void;
private readonly buttons: GameButtons;
private readonly disposeChallengeNode: () => void;
Expand Down Expand Up @@ -282,10 +280,8 @@ export default class ChallengeNode extends Node {
// Move from "Try Again" to "Check" state when a quantity is changed, see reactants-products-and-leftovers#37.
// Must be disposed.
const answerChangedMultilink = Multilink.lazyMultilinkAny( quantityProperties, () => {
const playStateProperty = this.playStateProperty!;
assert && assert( playStateProperty, 'playStateProperty should have been set by now.' );
if ( playStateProperty.value === PlayState.TRY_AGAIN ) {
playStateProperty.value = PlayState.SECOND_CHECK;
if ( model.playStateProperty.value === PlayState.TRY_AGAIN ) {
model.playStateProperty.value = PlayState.SECOND_CHECK;
}
} );

Expand Down Expand Up @@ -325,6 +321,7 @@ export default class ChallengeNode extends Node {
// switch between spinners and static numbers
quantitiesNode.setInteractive( PlayState.INTERACTIVE_STATES.includes( playState ) );
};
model.playStateProperty.link( playStateObserver );

//------------------------------------------------------------------------------------
// Developer
Expand All @@ -351,8 +348,8 @@ export default class ChallengeNode extends Node {

// Properties
this.checkButtonEnabledProperty.dispose();
if ( this.playStateProperty ) {
this.playStateProperty.unlink( playStateObserver );
if ( model.playStateProperty.hasListener( playStateObserver ) ) {
model.playStateProperty.unlink( playStateObserver );
}

// Nodes
Expand All @@ -363,21 +360,10 @@ export default class ChallengeNode extends Node {
quantitiesNode.dispose();
};

this.playStateProperty = null; // will be set by activate()
this.playStateObserver = playStateObserver;
this.buttons = buttons;
}

/**
* Connects this node to the model. Until this is called, the node is preloaded, but not fully functional.
*/
public activate( playStateProperty: EnumerationProperty<PlayState> ): void {
this.buttons.activate( playStateProperty );
this.playStateProperty = playStateProperty;
// optimization: we're set up in the correct initial state, so wait for state change
this.playStateProperty.lazyLink( this.playStateObserver ); // must be unlinked in dispose
}

public override dispose(): void {
this.disposeChallengeNode();
super.dispose();
Expand Down
19 changes: 3 additions & 16 deletions js/game/view/GameButtons.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* @author Chris Malley (PixelZoom, Inc.)
*/

import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
import optionize, { combineOptions } from '../../../../phet-core/js/optionize.js';
import PickOptional from '../../../../phet-core/js/types/PickOptional.js';
Expand All @@ -29,8 +28,6 @@ type GameButtonOptions = SelfOptions & PickOptional<VBoxOptions, 'maxWidth'>;

export default class GameButtons extends VBox {

private playStateProperty: EnumerationProperty<PlayState> | null;
private playStateObserver: ( ( playState: PlayState ) => void );
private readonly disposeGameButtons: () => void;

public constructor( model: GameModel, checkButtonEnabledProperty: TReadOnlyProperty<boolean>, providedOptions?: GameButtonOptions ) {
Expand Down Expand Up @@ -88,6 +85,7 @@ export default class GameButtons extends VBox {
showAnswerButton && ( showAnswerButton.visible = ( state === PlayState.SHOW_ANSWER ) );
nextButton && ( nextButton.visible = ( state === PlayState.NEXT ) );
};
model.playStateProperty.link( playStateObserver );

this.disposeGameButtons = () => {
checkButton.dispose();
Expand All @@ -97,21 +95,10 @@ export default class GameButtons extends VBox {
if ( checkButtonEnabledProperty.hasListener( checkButtonEnabledObserver ) ) {
checkButtonEnabledProperty.unlink( checkButtonEnabledObserver );
}
if ( this.playStateProperty && this.playStateProperty.hasListener( this.playStateObserver ) ) {
this.playStateProperty.unlink( this.playStateObserver );
if ( model.playStateProperty.hasListener( playStateObserver ) ) {
model.playStateProperty.unlink( playStateObserver );
}
};

this.playStateProperty = null; // will be set by activate()
this.playStateObserver = playStateObserver;
}

/**
* Connects this node to the model. Until this is called, the node is preloaded, but not fully functional.
*/
public activate( playStateProperty: EnumerationProperty<PlayState> ): void {
this.playStateProperty = playStateProperty;
this.playStateProperty.link( this.playStateObserver ); // must be unlinked in dispose
}

public override dispose(): void {
Expand Down
5 changes: 0 additions & 5 deletions js/game/view/GameScreenView.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,6 @@ export default class GameScreenView extends ScreenView {
this.resultsNode.step( dt );
}

// cleanup nodes
if ( this.playNode ) {
this.playNode.step( dt );
}

super.step( dt );
}
}
Expand Down
76 changes: 7 additions & 69 deletions js/game/view/PlayNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,7 @@ export default class PlayNode extends Node {
private readonly model: GameModel;
private readonly layoutBounds: Bounds2;
private readonly audioPlayer: GameAudioPlayer;

private readonly challengeBounds: Bounds2; // challenge will be displayed in the area below the status bar
private readonly disposeNodes: ChallengeNode[]; // nodes in this array are scheduled for disposal
private nextChallengeNode: ChallengeNode | null; // the next challenge, preloaded to improve responsiveness
private stepsSinceDisposal: number; // number of times that step() has been called since a node was schedule for disposal
private stepsSinceUpdate: number; // number of times that step() has been called since the challenge changed

public constructor( model: GameModel, layoutBounds: Bounds2, visibleBoundsProperty: TReadOnlyProperty<Bounds2>, audioPlayer: GameAudioPlayer ) {

Expand Down Expand Up @@ -83,10 +78,6 @@ export default class PlayNode extends Node {
this.challengeBounds = new Bounds2( layoutBounds.left, statusBar.bottom, layoutBounds.right, layoutBounds.bottom );

let currentChallengeNode: ChallengeNode | null = null;
this.disposeNodes = [];
this.nextChallengeNode = null;
this.stepsSinceDisposal = 0;
this.stepsSinceUpdate = 0;

/*
* Displays the current challenge.
Expand All @@ -96,80 +87,27 @@ export default class PlayNode extends Node {

// schedule previous challenge for deletion
if ( currentChallengeNode ) {
this.disposeNodes.push( currentChallengeNode );
currentChallengeNode.visible = false;
currentChallengeNode.dispose(); // handles removeChild
currentChallengeNode = null;
this.stepsSinceDisposal = 0;
}

// activate current challenge
if ( challenge ) { // challenge will be null on startup and 'Reset All'
if ( this.nextChallengeNode ) {
// use preloaded node
currentChallengeNode = this.nextChallengeNode;
this.nextChallengeNode = null;
}
else {
// if a node hasn't been preloaded, create one
currentChallengeNode = new ChallengeNode( model, challenge, this.challengeBounds, audioPlayer );
this.addChild( currentChallengeNode );
}
currentChallengeNode.activate( model.playStateProperty );
currentChallengeNode.visible = true;
this.stepsSinceUpdate = 0;
currentChallengeNode = new ChallengeNode( model, challenge, this.challengeBounds, audioPlayer );
this.addChild( currentChallengeNode );
}
} );

/*
* When we transition away from 'play' phase, schedule the current and preloaded nodes for disposal.
* Unlink is unnecessary because this node exists for the lifetime of the simulation.
* When we transition away from 'play' phase, dispose of the current challengeNode.
*/
model.gamePhaseProperty.link( gamePhase => {
if ( gamePhase !== GamePhase.PLAY ) {
if ( currentChallengeNode ) {
this.disposeNodes.push( currentChallengeNode );
currentChallengeNode = null;
}
if ( this.nextChallengeNode ) {
this.disposeNodes.push( this.nextChallengeNode );
this.nextChallengeNode = null;
}
this.stepsSinceDisposal = 0;
if ( gamePhase !== GamePhase.PLAY && currentChallengeNode ) {
currentChallengeNode.dispose();
currentChallengeNode = null;
}
} );
}

public step( dt: number ): void {

/**
* See issue #17
* To defer the cost of removing a ChallengeNode, handle the removal of the previous ChallengeNode after the current one
* is made visible (2 steps). The user will presumably be distracted processing what they see on the screen, so won't notice
* the brief interruption. this.disposeNodes is an array so that we can remove both the current and next (preloaded)
* ChallengeNodes when we leave the 'play' phase of the game.
*/
this.stepsSinceDisposal++;
if ( this.stepsSinceDisposal >= 2 && this.disposeNodes.length > 0 ) {
for ( let i = 0; i < this.disposeNodes.length; i++ ) {
this.removeChild( this.disposeNodes[ i ] );
this.disposeNodes[ i ].dispose();
}
this.disposeNodes.length = 0;
}

/**
* See issue #17
* To defer the cost of adding a ChallengeNode, preload the new one after the current one is made visible (2 steps).
* The user will presumably be distracted processing what they see on the screen, so won't notice the brief interruption.
*/
this.stepsSinceUpdate++;
const challengeIndex = this.model.challengeIndexProperty.value;
if ( this.stepsSinceUpdate >= 2 && this.visible && !this.nextChallengeNode && challengeIndex < this.model.challenges.length - 1 ) {
this.nextChallengeNode = new ChallengeNode( this.model, this.model.challenges[ challengeIndex + 1 ], this.challengeBounds, this.audioPlayer );
this.nextChallengeNode.visible = false;
this.addChild( this.nextChallengeNode );
}
}
}

reactantsProductsAndLeftovers.register( 'PlayNode', PlayNode );

0 comments on commit c37d19b

Please sign in to comment.