Skip to content

Commit

Permalink
Convert playAreas to not change currentNumber with internal Propertie…
Browse files Browse the repository at this point in the history 
…s, make recreated paper ones not pickable, add guards when setting/reading currentNumberProperty, see #2, #4, #5, #6
  • Loading branch information
@chrisklus
chrisklus committed Nov 11, 2019
1 parent aaf019f commit 92062cd
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 66 deletions.
11 changes: 9 additions & 2 deletions js/common/model/NumberPlayModel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Expand All @@ -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 );
}

/**
Expand All @@ -51,9 +56,11 @@ define( require => {
* @public
*/
reset() {
this.isResettingProperty.value = true;
this.onesPlayArea.reset();
this.objectsPlayArea.reset();
this.currentNumberProperty.reset();
this.isResettingProperty.reset();
}
}

Expand Down
44 changes: 25 additions & 19 deletions js/common/model/ObjectsPlayArea.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
51 changes: 30 additions & 21 deletions js/common/model/OnesPlayArea.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 );
}
} );
}
}
} );
}
Expand Down Expand Up @@ -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 );
}

/**
Expand Down
18 changes: 15 additions & 3 deletions js/common/view/ObjectsPlayAreaNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand All @@ -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 );
Expand Down
49 changes: 28 additions & 21 deletions js/common/view/OnesPlayAreaNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
Expand Down

0 comments on commit 92062cd

Please sign in to comment.