From 5ceaf56a2019bd3304b3fb068ad9f14aafd85c4a Mon Sep 17 00:00:00 2001 From: jbphet Date: Wed, 7 Jun 2017 14:27:35 -0600 Subject: [PATCH] addressed several REVIEW comments, see #88 --- js/game/view/EECollectionAreaNode.js | 43 ++++----------- js/game/view/EEGameIconNode.js | 55 +++++++++---------- js/game/view/EEGameLevelIconFactory.js | 71 ++++++++----------------- js/game/view/EEGameLevelView.js | 73 +++++++++++--------------- js/game/view/EEGameScreenView.js | 4 +- 5 files changed, 91 insertions(+), 155 deletions(-) diff --git a/js/game/view/EECollectionAreaNode.js b/js/game/view/EECollectionAreaNode.js index 15b8f2dd..f734c3a9 100644 --- a/js/game/view/EECollectionAreaNode.js +++ b/js/game/view/EECollectionAreaNode.js @@ -25,38 +25,22 @@ define( function( require ) { Node.call( this ); // create the 'halo' that will turn on as a hint that the user can drop something into the collection area - //REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY - var halo = new Rectangle( - 0, - 0, - collectionArea.bounds.width, - collectionArea.bounds.height, - CORNER_RADIUS, - CORNER_RADIUS, - { - lineWidth: 9, - stroke: '#66FF33' - } - ); + var halo = Rectangle.bounds( collectionArea.bounds, { + lineWidth: 9, + stroke: '#66FF33', + cornerRadius: CORNER_RADIUS + } ); this.addChild( halo ); // control halo visibility collectionArea.haloActiveProperty.linkAttribute( halo, 'visible' ); // create the basic rectangular background - //REVIEW: Rectangle.bounds( collectionArea.bounds, { cornerRadius: CORNER_RADIUS, ... } ) if we don't translate by the bounds minX/minY - var collectionAreaRectangle = new Rectangle( - 0, - 0, - collectionArea.bounds.width, - collectionArea.bounds.height, - CORNER_RADIUS, - CORNER_RADIUS, - { - fill: 'white', - stroke: 'black' - } - ); + var collectionAreaRectangle = Rectangle.bounds( collectionArea.bounds, { + fill: 'white', + stroke: 'black', + cornerRadius: CORNER_RADIUS + } ); this.addChild( collectionAreaRectangle ); // add the expression description representation, which will update if the expression description changes @@ -66,13 +50,11 @@ define( function( require ) { // remove the previous expression description node, if present if ( expressionDescriptionNode ) { self.removeChild( expressionDescriptionNode ); - //REVIEW: Usually with this pattern, the expressionDescriptionNode would be set to null? Otherwise if the - // description goes to null and back, things error? + expressionDescriptionNode = null; } // add the description node for the new expression if ( expressionDescription ) { - //REVIEW: If we don't translate by the bounds minX/minY (as suggested above), just use things relative to the bounds instead expressionDescriptionNode = new ExpressionDescriptionNode( expressionDescription, collectionArea.viewMode, @@ -81,9 +63,6 @@ define( function( require ) { self.addChild( expressionDescriptionNode ); } } ); - - //REVIEW: Recommend removing this, as it makes the Rectangle constructors a lot more verbose (and is an extra line) - this.setTranslation( collectionArea.bounds.minX, collectionArea.bounds.minY ); } expressionExchange.register( 'EECollectionAreaNode', EECollectionAreaNode ); diff --git a/js/game/view/EEGameIconNode.js b/js/game/view/EEGameIconNode.js index 5eb022d4..25eaa083 100644 --- a/js/game/view/EEGameIconNode.js +++ b/js/game/view/EEGameIconNode.js @@ -14,8 +14,9 @@ define( function( require ) { var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' ); var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' ); var FaceNode = require( 'SCENERY_PHET/FaceNode' ); + var HBox = require( 'SCENERY/nodes/HBox' ); + var HStrut = require( 'SCENERY/nodes/HStrut' ); var inherit = require( 'PHET_CORE/inherit' ); - var Node = require( 'SCENERY/nodes/Node' ); var PhetFont = require( 'SCENERY_PHET/PhetFont' ); var Rectangle = require( 'SCENERY/nodes/Rectangle' ); var Screen = require( 'JOIST/Screen' ); @@ -26,6 +27,7 @@ define( function( require ) { var ICON_SIZE = Screen.MINIMUM_HOME_SCREEN_ICON_SIZE; var COIN_SPACING = ICON_SIZE.width * 0.02; // empirically determined var TEXT_FONT = new PhetFont( 50 ); + var PLUS_SIGN_X_MARGIN = 10; /** * @constructor @@ -33,70 +35,61 @@ define( function( require ) { function EEGameIconNode() { // create the background - //REVIEW: Rectangle.dimension( ICON_SIZE, { fill: BACKGROUND_COLOR } ) Rectangle.call( this, 0, 0, ICON_SIZE.width, ICON_SIZE.height, { fill: BACKGROUND_COLOR } ); // created a rounded rectangle that looks like a card - //REVIEW: Recommend cornerRadius (duplicated) specified once in options object - var cardBackground = new Rectangle( 0, 0, ICON_SIZE.width / 2, ICON_SIZE.height / 2, 20, 20, { + var cardBackground = new Rectangle( 0, 0, ICON_SIZE.width / 2, ICON_SIZE.height / 2, { x: ICON_SIZE.width * 0.1, y: ICON_SIZE.height * 0.1, fill: 'white', stroke: 'black', - lineWidth: 2 + lineWidth: 2, + cornerRadius: 20 } ); this.addChild( cardBackground ); // create a "coin equation" that includes coins and numbers - var coinEquation = new Node(); - coinEquation.addChild( new Text( '2', { font: TEXT_FONT } ) ); var coinRadius = ICON_SIZE.width * 0.04; // empirically determined - //REVIEW: Can an HBox be used here? looks like anti-pattern with incrementing left based on extisting width. - coinEquation.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, { - left: coinEquation.width, - centerY: coinEquation.centerY - } ) ); - coinEquation.addChild( new Text( '+', { - font: TEXT_FONT, - left: coinEquation.width + 10 - } ) ); - coinEquation.addChild( new Text( '3', { - font: TEXT_FONT, - left: coinEquation.width + 10 - } ) ); - coinEquation.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, { - left: coinEquation.width, - centerY: coinEquation.centerY - } ) ); + var coinEquation = new HBox( { + children: [ + new Text( '2', { font: TEXT_FONT } ), + CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius ), + new HStrut( PLUS_SIGN_X_MARGIN ), + new Text( '+', { font: TEXT_FONT } ), + new HStrut( PLUS_SIGN_X_MARGIN ), + new Text( '3', { font: TEXT_FONT } ), + CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius ) + ] + } ); // add the coin equation to the card - //REVIEW: coinEquation.center = cardBackground.localBounds.center? coinEquation.centerX = cardBackground.width / 2; coinEquation.centerY = cardBackground.height / 2; cardBackground.addChild( coinEquation ); // create and add coins next to the card - //REVIEW: Has some values that could be factored out (and named for improved readability) + var topCoinRowCenterY = cardBackground.top + cardBackground.height * 0.3; + var secondCoinRowCenterY = cardBackground.top + cardBackground.height * 0.7; this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, { left: cardBackground.right + COIN_SPACING, - centerY: cardBackground.top + cardBackground.height * 0.3 + centerY: topCoinRowCenterY } ) ); this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.X, coinRadius, { left: cardBackground.right + coinRadius * 2 + COIN_SPACING * 2, - centerY: cardBackground.top + cardBackground.height * 0.3 + centerY: topCoinRowCenterY } ) ); this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, { left: cardBackground.right + COIN_SPACING, - centerY: cardBackground.top + cardBackground.height * 0.7 + centerY: secondCoinRowCenterY } ) ); this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, { left: cardBackground.right + coinRadius * 2 + COIN_SPACING * 2, - centerY: cardBackground.top + cardBackground.height * 0.7 + centerY: secondCoinRowCenterY } ) ); this.addChild( CoinNodeFactory.createIconNode( CoinTermTypeID.Y, coinRadius, { left: cardBackground.right + coinRadius * 4 + COIN_SPACING * 3, - centerY: cardBackground.top + cardBackground.height * 0.7 + centerY: secondCoinRowCenterY } ) ); this.addChild( new FaceNode( ICON_SIZE.width * 0.35, { diff --git a/js/game/view/EEGameLevelIconFactory.js b/js/game/view/EEGameLevelIconFactory.js index b6bb47cd..ae525389 100644 --- a/js/game/view/EEGameLevelIconFactory.js +++ b/js/game/view/EEGameLevelIconFactory.js @@ -8,73 +8,50 @@ define( function( require ) { 'use strict'; // modules + var CoinNodeFactory = require( 'EXPRESSION_EXCHANGE/common/view/CoinNodeFactory' ); var CoinTermTypeID = require( 'EXPRESSION_EXCHANGE/common/enum/CoinTermTypeID' ); var EESharedConstants = require( 'EXPRESSION_EXCHANGE/common/EESharedConstants' ); var expressionExchange = require( 'EXPRESSION_EXCHANGE/expressionExchange' ); - var Image = require( 'SCENERY/nodes/Image' ); var Node = require( 'SCENERY/nodes/Node' ); var PhetFont = require( 'SCENERY_PHET/PhetFont' ); var Rectangle = require( 'SCENERY/nodes/Rectangle' ); var Text = require( 'SCENERY/nodes/Text' ); - // images - var coinXTimesYFrontImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-xy-back.png' ); - var coinYBackImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-y-back.png' ); - var coinYSquaredBackImage = require( 'mipmap!EXPRESSION_EXCHANGE/coin-y-squared-back.png' ); - // constants var CARD_CORNER_ROUNDING = 4; var NUMBER_LABEL_FONT = new PhetFont( 26 ); // size empirically determined var CARD_STAGGER_OFFSET = 1.5; // empirically determined, same in x and y directions var ICON_WIDTH = 40; var CARD_ICON_HEIGHT = 40; + var COIN_RADIUS = 20; // empirically determined - // helper function for creating coin-image-based icons - //REVIEW Type docs generally helpful when reading this type of function + /** + * helper function for creating coin-image-based icons + * @param {CoinTermTypeID} coinTermTypeID - type of coin image to use + * @param {number} value - value that will appear on the face of the coin + * @returns {Node} + */ function createCoinIcon( coinTermTypeID, value ) { - var rootNode = new Node(); - var imageNode; - // create the coin image node - //REVIEW: These images are also used in similar switch statements in CoinNodeFactory. This could be potentially - // factored out into CoinNodeFactory? - switch( coinTermTypeID ) { - case CoinTermTypeID.X: - imageNode = new Image( coinXTimesYFrontImage ); - break; - - case CoinTermTypeID.Y: - imageNode = new Image( coinYBackImage ); - break; - - case CoinTermTypeID.Z: - imageNode = new Image( coinYSquaredBackImage ); - break; - - default: - assert && assert( false, 'handling does not exist for coin term type: ' + coinTermTypeID ); - break; - } - - imageNode.setScaleMagnitude( ICON_WIDTH / imageNode.width ); - rootNode.addChild( imageNode ); + var imageNode = CoinNodeFactory.createImageNode( coinTermTypeID, COIN_RADIUS, false ); // add the label - rootNode.addChild( new Text( value, { + var label = new Text( value, { font: NUMBER_LABEL_FONT, centerX: imageNode.width / 2, centerY: imageNode.height / 2 - } ) ); - - //REVIEW: Easier to - // return new Node( { children: [ imageNode, the_text_thing ] } ) rather than having an additional declaration and return. + } ); - return rootNode; + return new Node( { children: [ imageNode, label ] } ); } - // helper function for creating the icons that look like stacks of cards - //REVIEW Type docs generally helpful when reading this type of function + /** + * helper function for creating the icons that look like a stack of cards + * @param {number} numberOnStack + * @param {number} numberOfAdditionalCards + * @returns {Node} + */ function createCardStackIcon( numberOnStack, numberOfAdditionalCards ) { var rootNode = new Node(); var cardWidth = ICON_WIDTH - numberOfAdditionalCards * CARD_STAGGER_OFFSET; @@ -100,9 +77,7 @@ define( function( require ) { } ) ); // add the cards to the root node - //REVIEW: cards.reverse() returns the reversed array, so that can be chained with the forEach directly - cards.reverse(); - cards.forEach( function( card ) { + cards.reverse().forEach( function( card ) { rootNode.addChild( card ); } ); @@ -125,20 +100,18 @@ define( function( require ) { var icon; - //REVIEW: If helpful, I've found it nice to have different objects for each level that handle creation of icons. - //REVIEW: No problem to leave as-is if this is the most convenient. switch( gameLevel ) { case 0: - icon = createCoinIcon( CoinTermTypeID.X, 1 ); + icon = createCoinIcon( CoinTermTypeID.X_TIMES_Y, 1 ); break; case 1: - icon = createCoinIcon( CoinTermTypeID.Y, 2 ); + icon = createCoinIcon( CoinTermTypeID.X, 2 ); break; case 2: - icon = createCoinIcon( CoinTermTypeID.Z, 3 ); + icon = createCoinIcon( CoinTermTypeID.Y_SQUARED, 3 ); break; case 3: diff --git a/js/game/view/EEGameLevelView.js b/js/game/view/EEGameLevelView.js index 76059cb3..e87872f9 100644 --- a/js/game/view/EEGameLevelView.js +++ b/js/game/view/EEGameLevelView.js @@ -40,9 +40,10 @@ define( function( require ) { * @param {EEGameLevel} levelModel - model for the level depicted by this view object * @param {Bounds2} screenLayoutBounds * @param {Property.} visibleBoundsProperty + * @param {GameAudioPlayer} gameAudioPlayer * @constructor */ - function EEGameLevelView( gameModel, levelModel, screenLayoutBounds, visibleBoundsProperty ) { + function EEGameLevelView( gameModel, levelModel, screenLayoutBounds, visibleBoundsProperty, gameAudioPlayer ) { var self = this; @@ -54,15 +55,20 @@ define( function( require ) { // performance issues, and this property can be used to only clock when this is in the viewport. this.inViewportProperty = new Property( false ); - // add an invisible background rectangle so that bounds are correct - //REVIEW: Shouldn't need a reference to this with proper layering? + // add an invisible background rectangle so that bounds are correct, this is needed for animation of game level views var background = new Rectangle( screenLayoutBounds, { - //REVIEW: if for debugging, why still here? - //REVIEW: Why have something that's "essentially invisible" instead of a null or 'transparent' fill? - stroke: 'rgba( 0, 0, 200, 0.01 )' // increase opacity to make the outline visible if desired (for debugging) + stroke: 'transparent' // increase opacity to make the outline visible if desired (for debugging) } ); this.addChild( background ); + // layer where everything else should appear + var middleLayer = new Node(); + this.addChild( middleLayer ); + + // layer where the coin term nodes live + var coinTermLayer = new Node(); + this.addChild( coinTermLayer ); + // set the bounds for coin term retrieval in the model levelModel.setCoinTermRetrievalBounds( screenLayoutBounds ); @@ -75,17 +81,15 @@ define( function( require ) { top: 20 } ); - this.addChild( title ); + middleLayer.addChild( title ); // add the back button var backButton = new BackButton( { - //REVIEW: easier to read with screenLayoutBounds.left and screenLayoutBounds.top - //REVIEW: Or leftTop: screenLayoutBounds.eroded( 30 ).leftTop - left: screenLayoutBounds.minX + 30, - top: screenLayoutBounds.minY + 30, + left: screenLayoutBounds.left + 30, + top: screenLayoutBounds.top + 30, listener: gameModel.returnToLevelSelection.bind( gameModel ) } ); - this.addChild( backButton ); + middleLayer.addChild( backButton ); // add the refresh button var refreshButton = new RectangularPushButton( { @@ -97,20 +101,21 @@ define( function( require ) { left: backButton.left, top: backButton.bottom + 8 } ); - this.addChild( refreshButton ); + middleLayer.addChild( refreshButton ); - // create the expression manipulation view, added later for correct layering + // create the expression manipulation view var expressionManipulationView = new ExpressionManipulationView( levelModel, visibleBoundsProperty, { coinTermBreakApartButtonMode: 'inverted' } ); + coinTermLayer.addChild( expressionManipulationView ); // add the coin term creator box var coinTermCreatorBox = null; levelModel.currentChallengeProperty.link( function( currentChallenge ) { if ( coinTermCreatorBox ) { - self.removeChild( coinTermCreatorBox ); + middleLayer.removeChild( coinTermCreatorBox ); } coinTermCreatorBox = CoinTermCreatorBoxFactory.createGameScreenCreatorBox( currentChallenge, @@ -118,42 +123,37 @@ define( function( require ) { expressionManipulationView, { centerX: title.centerX, bottom: screenLayoutBounds.bottom - 40 } ); - self.addChild( coinTermCreatorBox ); - //REVIEW: Seems like an anti-pattern to moveToBack here. Use layers instead? - coinTermCreatorBox.moveToBack(); // needs to be behind coin term and other layers... - background.moveToBack(); // ...except for the background + middleLayer.addChild( coinTermCreatorBox ); // let the model know where the creator box is so that it knows when the user returns coin terms levelModel.creatorBoxBounds = coinTermCreatorBox.bounds; } ); // add the check box that allows expressions with negative values to be simplified - //REVIEW: Third collection area seems easy to break in future refactoring. Pick last collection area instead? - var boundsOfLowestCollectionArea = levelModel.collectionAreas[ 2 ].bounds; + var boundsOfLowestCollectionArea = _.last( levelModel.collectionAreas ).bounds; var showSubtractionCheckbox = new CheckBox( new ShowSubtractionIcon(), levelModel.simplifyNegativesProperty, { - //REVIEW: for readability, use bounds.left and bounds.bottom? - left: boundsOfLowestCollectionArea.minX, - top: boundsOfLowestCollectionArea.maxY + 20, + left: boundsOfLowestCollectionArea.left, + top: boundsOfLowestCollectionArea.bottom + 20, maxWidth: boundsOfLowestCollectionArea.minX } ); - this.addChild( showSubtractionCheckbox ); + middleLayer.addChild( showSubtractionCheckbox ); // add the node for moving to the next level, only shown when all challenges on this level have been answered this.nextLevelNode = new NextLevelNode( gameModel.nextLevel.bind( gameModel ), { centerX: title.centerX, centerY: screenLayoutBounds.height * 0.33 // empirically determined } ); - this.addChild( this.nextLevelNode ); + middleLayer.addChild( this.nextLevelNode ); this.allLevelsCompletedDialog = new AllLevelsCompletedDialog( gameModel.returnToLevelSelection.bind( gameModel ), { centerX: title.centerX, centerY: screenLayoutBounds.height * 0.4 // empirically determined } ); - this.addChild( this.allLevelsCompletedDialog ); + middleLayer.addChild( this.allLevelsCompletedDialog ); // only show the checkbox for simplifying expressions with negative values if some are present in the challenge levelModel.currentChallengeProperty.link( function( currentChallenge ) { @@ -168,14 +168,6 @@ define( function( require ) { showSubtractionCheckbox.visible = negativesExist; } ); - // add the view area where the user will interact with coin terms and expressions - this.addChild( expressionManipulationView ); - - // create the audio player - //REVIEW: Presumably this one is used, the one in EEGameScreenView is not? - // TODO: consider passing this in to save memory - var gameAudioPlayer = new GameAudioPlayer( levelModel.soundEnabledProperty ); - // show the appropriate dialog and reward node based on the score levelModel.scoreProperty.link( function( score, previousScore ) { @@ -200,13 +192,11 @@ define( function( require ) { self.nextLevelNode.visible = true; } - // show the reward node if warranted + // show the reward node if warranted - this is created lazily to save time on startup and memory if ( allLevelsCompleted || EEQueryParameters.showRewardNodeEveryLevel ) { if ( !self.rewardNode ) { - //REVIEW: Can we create this on startup, so the extra logic isn't needed? self.rewardNode = new EERewardNode(); background.addChild( self.rewardNode ); - self.rewardNode.moveToBack(); //REVIEW: Why immediately move to back? why not insert it at the correct place? } self.rewardNode.visible = true; } @@ -225,11 +215,12 @@ define( function( require ) { return inherit( Node, EEGameLevelView, { - // @public - //REVIEW: docs? + /** + * @param {number} dt + * @public + */ step: function( dt ) { if ( this.inViewportProperty.get() && this.rewardNode && this.rewardNode.visible ) { - //REVIEW: Generally preferred to cap DT at the top level. Is the cap only needed for RewardNode? this.rewardNode.step( Math.min( dt, 1 ) ); } }, diff --git a/js/game/view/EEGameScreenView.js b/js/game/view/EEGameScreenView.js index 92f15361..ce5f0768 100644 --- a/js/game/view/EEGameScreenView.js +++ b/js/game/view/EEGameScreenView.js @@ -35,7 +35,6 @@ define( function( require ) { // hook up the audio player to the sound settings //REVIEW: visibility docs - //REVIEW: Wait, this isn't used? EEGameLevelView uses a different instance of GameAudioPlayer? this.gameAudioPlayer = new GameAudioPlayer( gameModel.soundEnabledProperty ); // consolidate the level scores into an array for the level selection node @@ -91,7 +90,8 @@ define( function( require ) { gameModel, levelModel, self.layoutBounds, - self.visibleBoundsProperty + self.visibleBoundsProperty, + self.gameAudioPlayer ); gameLevelView.visible = false; // will be made visible when the corresponding level is activated self.gameLevelViews.push( gameLevelView );