From 92062cd457444ea8ed221f05ed2d037bb066d931 Mon Sep 17 00:00:00 2001 From: chrisklus Date: Sun, 10 Nov 2019 23:22:56 -0500 Subject: [PATCH] Convert playAreas to not change currentNumber with internal Properties, make recreated paper ones not pickable, add guards when setting/reading currentNumberProperty, see #2, #4, #5, #6 --- js/common/model/NumberPlayModel.js | 11 ++++-- js/common/model/ObjectsPlayArea.js | 44 +++++++++++++---------- js/common/model/OnesPlayArea.js | 51 ++++++++++++++++----------- js/common/view/ObjectsPlayAreaNode.js | 18 ++++++++-- js/common/view/OnesPlayAreaNode.js | 49 ++++++++++++++----------- 5 files changed, 107 insertions(+), 66 deletions(-) diff --git a/js/common/model/NumberPlayModel.js b/js/common/model/NumberPlayModel.js index 40f1f504..c9bf9d2c 100644 --- a/js/common/model/NumberPlayModel.js +++ b/js/common/model/NumberPlayModel.js @@ -9,6 +9,7 @@ define( require => { 'use strict'; // modules + const BooleanProperty = require( 'AXON/BooleanProperty' ); const numberPlay = require( 'NUMBER_PLAY/numberPlay' ); const ObjectsPlayArea = require( 'NUMBER_PLAY/common/model/ObjectsPlayArea' ); const OnesPlayArea = require( 'NUMBER_PLAY/common/model/OnesPlayArea' ); @@ -31,11 +32,15 @@ define( require => { range: new Range( 0, highestCount ) } ); + // @public {BooleanProperty} - true when the sim is being reset. this is used so that playAreas don't return things + // to their buckets the normal way (with animations), but instead with a different reset case (no animations). + this.isResettingProperty = new BooleanProperty( false ); + // @public (read-only) - the model for managing the play area in the OnesAccordionBox - this.onesPlayArea = new OnesPlayArea( this.currentNumberProperty, paperNumberOrigin ); + this.onesPlayArea = new OnesPlayArea( this.currentNumberProperty, paperNumberOrigin, this.isResettingProperty ); // @public (read-only) - the model for managing the play area in the ObjectsAccordionBox - this.objectsPlayArea = new ObjectsPlayArea( this.currentNumberProperty, objectMaxScale, organizedObjectPadding ); + this.objectsPlayArea = new ObjectsPlayArea( this.currentNumberProperty, objectMaxScale, organizedObjectPadding, this.isResettingProperty ); } /** @@ -51,9 +56,11 @@ define( require => { * @public */ reset() { + this.isResettingProperty.value = true; this.onesPlayArea.reset(); this.objectsPlayArea.reset(); this.currentNumberProperty.reset(); + this.isResettingProperty.reset(); } } diff --git a/js/common/model/ObjectsPlayArea.js b/js/common/model/ObjectsPlayArea.js index 4b864c77..0438a102 100644 --- a/js/common/model/ObjectsPlayArea.js +++ b/js/common/model/ObjectsPlayArea.js @@ -47,12 +47,13 @@ define( require => { * @param {NumberProperty} currentNumberProperty * @param {number} objectMaxScale - see PlayObject for doc * @param {Vector2} organizedObjectPadding - see calculateOrganizedPlayObjectSpots for doc + * @param {BooleanProperty} isResetting */ - constructor( currentNumberProperty, objectMaxScale, organizedObjectPadding ) { + constructor( currentNumberProperty, objectMaxScale, organizedObjectPadding, isResettingProperty ) { assert && assert( currentNumberProperty.range, `Range is required: ${currentNumberProperty.range}` ); - // @private + // @public this.currentNumberProperty = currentNumberProperty; // @public (read-only) @@ -93,29 +94,34 @@ define( require => { // @private - all playObjects that are currently in the play area this.playObjectsInPlayArea = new ObservableArray(); + // @public {boolean} whether the view of this play area is controlling the current number + this.isControllingCurrentNumber = false; + // if the current number changes, add or remove playObjects from the play area currentNumberProperty.link( ( currentNumber, previousNumber ) => { - if ( currentNumber < this.playObjectsInPlayArea.lengthProperty.value ) { - _.times( previousNumber - currentNumber, () => { - this.findPlayObjectToReturnToBucket(); - } ); - } - else if ( currentNumber > this.playObjectsInPlayArea.lengthProperty.value ) { - _.times( currentNumber - previousNumber, () => { - this.findPlayObjectToAddToPlayArea(); - } ); + if ( !this.isControllingCurrentNumber && !isResettingProperty.value ) { + if ( currentNumber < previousNumber ) { + _.times( previousNumber - currentNumber, () => { + + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.playObjectsInPlayArea.lengthProperty.value > currentNumberProperty.range.min ) { + this.findPlayObjectToReturnToBucket(); + } + } ); + } + else if ( currentNumber > previousNumber ) { + _.times( currentNumber - previousNumber, () => { + + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.playObjectsInPlayArea.lengthProperty.value < currentNumberProperty.range.max ) { + this.findPlayObjectToAddToPlayArea(); + } + } ); + } } } ); } - /** - * Updates the value of currentNumberProperty to be the number of playObjects in the play area. Should only be - * called when user-controlled state of one of the playObjects changes. - */ - updateCurrentNumberProperty() { - this.currentNumberProperty.value = this.playObjectsInPlayArea.lengthProperty.value; - } - /** * Animates the given playObject back into the bucket and removes it from playObjectsInPlayArea. If the given * playObject is already in the bucket, that's okay - it just animates back to its initial position if need be. diff --git a/js/common/model/OnesPlayArea.js b/js/common/model/OnesPlayArea.js index 89f6d8e9..2c366930 100644 --- a/js/common/model/OnesPlayArea.js +++ b/js/common/model/OnesPlayArea.js @@ -38,13 +38,14 @@ define( require => { * @param {NumberProperty} currentNumberProperty * @param {Vector2} paperNumberOrigin - origin of where paper numbers are created, but only used when the model is * responding to changes in currentNumberProperty, not when a user drags a paperNumber out of the bucket + * @param {BooleanProperty} isResetting * TODO: paperNumberOrigin is a band-aid since paperNumberNodes don't use MVT */ - constructor( currentNumberProperty, paperNumberOrigin ) { + constructor( currentNumberProperty, paperNumberOrigin, isResettingProperty ) { assert && assert( currentNumberProperty.range, `Range is required: ${currentNumberProperty.range}` ); - // @private + // @public this.currentNumberProperty = currentNumberProperty; // @public {NumberProperty} - The total sum of the current numbers @@ -74,17 +75,33 @@ define( require => { size: NumberPlayConstants.BUCKET_SIZE } ); + // @public {boolean} whether the view of this play area is controlling the current number + this.isControllingCurrentNumber = false; + // if the current number changes, add or remove paperNumbers from the play area currentNumberProperty.link( ( currentNumber, previousNumber ) => { - if ( currentNumber < this.sumProperty.value ) { - _.times( previousNumber - currentNumber, () => { - this.returnPaperNumberToBucket( paperNumberOrigin ); - } ); - } - else if ( currentNumber > this.sumProperty.value ) { - _.times( currentNumber - previousNumber, () => { - this.createPaperNumberFromBucket( paperNumberOrigin ); - } ); + if ( !this.isControllingCurrentNumber && !isResettingProperty.value ) { + if ( previousNumber !== this.sumProperty.value ) { + // debugger; + } + // console.log( `paper numbers are being changed. current number: ${currentNumber}, current sum: ${this.sumProperty.value}, target change: ${currentNumber - previousNumber}` ); + if ( currentNumber < previousNumber ) { + _.times( previousNumber - currentNumber, () => { + console.log( currentNumber, previousNumber ); + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.sumProperty.value > currentNumberProperty.range.min ) { + this.returnPaperNumberToBucket( paperNumberOrigin ); + } + } ); + } + else if ( currentNumber > previousNumber ) { + _.times( currentNumber - previousNumber, () => { + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.sumProperty.value < currentNumberProperty.range.max ) { + this.createPaperNumberFromBucket( paperNumberOrigin ); + } + } ); + } } } ); } @@ -176,20 +193,12 @@ define( require => { this.addPaperNumber( paperNumberToReturn ); } - // send it back to its origin and set its value to 0. paperNumbers aren't removed from paperNumbers until they get + // set its value to 0 and send it back to its origin. paperNumbers aren't removed from the playArea until they get // back to the bucket, but we don't want them to count towards the sum while they're on their way to the bucket. // this allows for multiple paperNumbers to be returning to the bucket at the same time instead of only one at a // time. - paperNumberToReturn.setDestination( paperNumberOrigin, true ); paperNumberToReturn.numberValueProperty.value = 0; - } - - /** - * Updates the value of currentNumberProperty to be the sum of paperNumbers in the play area. Should only be - * called when user-controlled state of one of the paperNumbers changes. - */ - updateCurrentNumberProperty() { - this.currentNumberProperty.value = this.sumProperty.value; + paperNumberToReturn.setDestination( paperNumberOrigin, true ); } /** diff --git a/js/common/view/ObjectsPlayAreaNode.js b/js/common/view/ObjectsPlayAreaNode.js index 80abc914..17da321d 100644 --- a/js/common/view/ObjectsPlayAreaNode.js +++ b/js/common/view/ObjectsPlayAreaNode.js @@ -58,7 +58,13 @@ define( require => { playObjectsStorageLayer.addChild( playObjectNode ); } playArea.returnPlayObjectToBucket( playObject ); - playArea.updateCurrentNumberProperty(); + + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( playArea.currentNumberProperty.value > playArea.currentNumberProperty.range.min ) { + playArea.isControllingCurrentNumber = true; + playArea.currentNumberProperty.value--; + playArea.isControllingCurrentNumber = false; + } } else { playArea.checkIfCoveringPlayObject( playObject, 0 ); @@ -72,14 +78,20 @@ define( require => { if ( bucketFrontNode.bounds.intersectsBounds( playObjectNode.bounds ) || bucketHole.bounds.intersectsBounds( playObjectNode.bounds ) ) { playArea.addPlayObjectToPlayArea( playObject ); - playArea.updateCurrentNumberProperty(); + + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( playArea.currentNumberProperty.value < playArea.currentNumberProperty.range.max ) { + playArea.isControllingCurrentNumber = true; + playArea.currentNumberProperty.value++; + playArea.isControllingCurrentNumber = false; + } } } } ); playObject.positionProperty.link( () => { if ( ( bucketFrontNode.bounds.intersectsBounds( playObjectNode.bounds ) || - bucketHole.bounds.intersectsBounds( playObjectNode.bounds ) ) && !playObject.userControlledProperty.value ) { + bucketHole.bounds.intersectsBounds( playObjectNode.bounds ) ) && !playObject.userControlledProperty.value ) { if ( playObjectsPlayAreaLayer.hasChild( playObjectNode ) ) { playObjectsPlayAreaLayer.removeChild( playObjectNode ); playObjectsStorageLayer.addChild( playObjectNode ); diff --git a/js/common/view/OnesPlayAreaNode.js b/js/common/view/OnesPlayAreaNode.js index 519d1138..4bae3233 100644 --- a/js/common/view/OnesPlayAreaNode.js +++ b/js/common/view/OnesPlayAreaNode.js @@ -118,10 +118,17 @@ define( require => { // Add it and lookup the related node. this.playArea.addPaperNumber( paperNumber ); const paperNumberNode = this.findPaperNumberNode( paperNumber ); - paperNumberNode.startSyntheticDrag( event ); - // a user grabbed a new number, so update the sim's currentNumberProperty - this.playArea.updateCurrentNumberProperty(); + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.playArea.currentNumberProperty.value < this.playArea.currentNumberProperty.range.max ) { + + // a user grabbed a new number, so update the sim's currentNumberProperty + this.playArea.isControllingCurrentNumber = true; + this.playArea.currentNumberProperty.value++; + this.playArea.isControllingCurrentNumber = false; + } + + paperNumberNode.startSyntheticDrag( event ); } /** @@ -137,7 +144,10 @@ define( require => { // if a number's value is set to 0, make it's corresponding node not pickable (since it's on its way to the bucket) paperNumber.numberValueProperty.link( numberValue => { - paperNumberNode.pickable = numberValue > 0; + if ( numberValue < 1 ) { + paperNumberNode.pickable = false; + paperNumberNode.interruptSubtreeInput(); + } } ); this.paperNumberNodeMap[ paperNumberNode.paperNumber.id ] = paperNumberNode; @@ -308,28 +318,25 @@ define( require => { // Return it to the panel if it's been dropped in the panel. if ( this.isNumberInReturnZone( paperNumber ) ) { - // Remove the original paper number (as we have are about to add its components to return). - this.playArea.removePaperNumber( paperNumber ); + const paperNumberValue = paperNumber.numberValueProperty.value; + paperNumber.numberValueProperty.value = 0; - // a user returned a number in play, so update the sim's currentNumberProperty - this.playArea.updateCurrentNumberProperty(); + // Set its destination to the proper target (with the offset so that it will disappear once centered). + let targetPosition = this.onesCreatorNode.getOriginLocation(); - const baseNumbers = paperNumber.baseNumbers; + // TODO: the ternary below is a hack that shouldn't be needed once https://github.com/phetsims/number-play/issues/6 is fixed. + const paperCenterOffset = new PaperNumber( paperNumberValue > 0 ? paperNumberValue : 1, new Vector2( 0, 0 ) ).getLocalBounds().center; + targetPosition = targetPosition.minus( paperCenterOffset ); + paperNumber.setDestination( targetPosition, true ); - // Split it into a PaperNumber for each of its base numbers, and animate them to their targets in the - // explore panel. - for ( let i = baseNumbers.length - 1; i >= 0; i-- ) { - const baseNumber = baseNumbers[ i ]; - const basePaperNumber = new PaperNumber( baseNumber.numberValue, paperNumber.positionProperty.value ); - // Set its destination to the proper target (with the offset so that it will disappear once centered). - let targetPosition = this.onesCreatorNode.getOriginLocation(); - const paperCenterOffset = new PaperNumber( baseNumber.numberValue, new Vector2( 0, 0 ) ).getLocalBounds().center; - targetPosition = targetPosition.minus( paperCenterOffset ); - basePaperNumber.setDestination( targetPosition, true ); + // TODO: the need for this guard means that the play areas are not in sync, and should be eliminated when https://github.com/phetsims/number-play/issues/6 is fixed. + if ( this.playArea.currentNumberProperty.value > this.playArea.currentNumberProperty.range.min ) { - // Add the new base paper number - this.playArea.addPaperNumber( basePaperNumber ); + // a user returned a number, so update the sim's currentNumberProperty + this.playArea.isControllingCurrentNumber = true; + this.playArea.currentNumberProperty.value = this.playArea.currentNumberProperty.value - paperNumberValue; + this.playArea.isControllingCurrentNumber = false; } } }