Skip to content

Commit

Permalink
More cleanup and review notes, see #88
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonathanolson
jonathanolson committed May 24, 2017
1 parent 13cc5ad commit 2785ac2
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 63 deletions.
3 changes: 2 additions & 1 deletion doc/implementation-notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ The main "big picture" common code types are the ExpressionManipulationModel and
of the first three screens has one of each of these classes, and the game screen has one of each of these for each
game level. These classes handle the basics of creating and working with expressions.

There are two primary low-level classes that are used withing the model: CoinTerm and Expression.
There are two primary low-level classes that are used withing the model: CoinTerm and Expression.

A CoinTerm is a fundamental model element, and it represents a thing that can either appear to be a coin or a
mathematical term. The name is used a lot throughout the code, so it's important to have a clear idea of what it is.
Expand All @@ -40,6 +40,7 @@ Some of this code had to be hooked to properties, while some of it was implement
can be complicated, so it will be important to have a good understanding of such interactions if some of these
behavioral rules need to be changed or fixed.

REVIEW: Don't understand how a coin term can overlap (be a member of?) multiple expressions
A decision was made that if a coin term overlaps multiple expressions, on the one with the most overlap should have its
hints activated. This means that the main model has to compare them and decide which one to activate, which means that
this decision must be centralized in the main model rather than leaving it to the coin terms or expressions.
Expand Down
27 changes: 15 additions & 12 deletions js/common/model/CoinTerm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ define( function( require ) {
//REVIEW: Here's an example where type documentation really helps. No idea (in initial read through) what type it can hold.
this.inProgressAnimationProperty = new Property( null );

// @public (read-only) - total number of coins/terms combined into this one, can be negative
// @public {Property.<number>} (read-only) - total number of coins/terms combined into this one, can be negative
this.totalCountProperty = new Property( options.initialCount );

// @public (read-write) - flag that controls whether breaking apart is allowed
Expand All @@ -92,13 +92,15 @@ define( function( require ) {
// having it available on the model element after being set by the view worked out to be the best approach.
this.relativeViewBoundsProperty = new Property( null );

// @public (read only) - ranges from 1 to 0, used primarily for fading out of a coin term when cancellation occurs
// @public {Property.<number>} (read only) - ranges from 1 to 0, used primarily for fading out of a coin term when
// cancellation occurs
this.existenceStrengthProperty = new Property( 1 );

// @public, determines the opacity of the card on which the coin term can reside
this.cardOpacityProperty = new Property( options.initiallyOnCard ? 1.0 : 0 );
// @public {Property.<number>}, determines the opacity of the card on which the coin term can reside
this.cardOpacityProperty = new Property( options.initiallyOnCard ? 1 : 0 );

// @public, used by view to make the coin terms appear smaller if necessary when put in collection areas (game only)
// @public {Property.<number>}, used by view to make the coin terms appear smaller if necessary when put in
// collection areas (game only)
this.scaleProperty = new Property( 1 );

//------------------------------------------------------------------------
Expand Down Expand Up @@ -131,23 +133,24 @@ define( function( require ) {
}

// @private - countdown timers for fading out the card background
//REVIEW: What type? Maybe {number|null}
this.cardPreFadeCountdown = null;
this.cardFadeCountdown = null;

//------------------------------------------------------------------------
// emitters
//------------------------------------------------------------------------

// @public, listen only, emits an event when an animation finishes and the destination is reached
// @public {Emitter}, listen only, emits an event when an animation finishes and the destination is reached
this.destinationReachedEmitter = new Emitter();

// @public, listen only, emits an event when coin terms returns to original position and is not user controlled
// @public {Emitter}, listen only, emits an event when coin terms returns to original position and is not user controlled
this.returnedToOriginEmitter = new Emitter();

// @public, listen only, emits an event when this coin term should be broken apart
// @public {Emitter}, listen only, emits an event when this coin term should be broken apart
this.breakApartEmitter = new Emitter();

// @private, used when animating back to original position
// @private {Vector2}, used when animating back to original position
this.initialPosition = options.initialPosition;

//------------------------------------------------------------------------
Expand Down Expand Up @@ -269,21 +272,21 @@ define( function( require ) {
* @public
*/
travelToDestination: function( destination ) {
var self = this;
this.destinationProperty.set( destination );
var currentPosition = this.positionProperty.get();
if ( currentPosition.equals( destination ) ) {

// The coin terms is already at the destination, no animation is required, but emit a notification in case the
// the client needs it.
self.destinationReachedEmitter.emit();
this.destinationReachedEmitter.emit();
}
else {

// calculate the time needed to get to the destination
var animationDuration = self.positionProperty.get().distance( destination ) /
var animationDuration = this.positionProperty.get().distance( destination ) /
EESharedConstants.COIN_TERM_MOVEMENT_SPEED;

//REVIEW: Should this be a separate type, since it's available as a public Property?
this.inProgressAnimationProperty.set( {
startPosition: this.positionProperty.get().copy(),
travelVector: destination.minus( this.positionProperty.get() ),
Expand Down
6 changes: 3 additions & 3 deletions js/common/model/CoinTermFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ define( function( require ) {

/**
* REVIEW: Needs @param
* {number} xValueProperty
* {number} yValueProperty
* {number} zValueProperty
* @param {number} xValueProperty
* @param {number} yValueProperty
* @param {number} zValueProperty
* @constructor
*/
function CoinTermFactory( xValueProperty, yValueProperty, zValueProperty ) {
Expand Down
35 changes: 19 additions & 16 deletions js/common/model/Expression.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,18 @@ define( function( require ) {
// properties
//------------------------------------------------------------------------

this.upperLeftCornerProperty = new Property( Vector2.ZERO ); // @public (read only)
this.widthProperty = new Property( 0 ); // @public (read only)
this.heightProperty = new Property( 0 ); // @public (read only)
this.userControlledProperty = new Property( false ); // @public (read-write)
this.inEditModeProperty = new Property( false ); // @public, indicates whether this expression is being edited
this.collectedProperty = new Property( false ); // @public, indicates whether this is in a collection box (for game)
this.upperLeftCornerProperty = new Property( Vector2.ZERO ); // @public {Property.<Vector2>} (read only)
this.widthProperty = new Property( 0 ); // @public {Property.<number>} (read only)
this.heightProperty = new Property( 0 ); // @public {Property.<number>} (read only)
this.userControlledProperty = new Property( false ); // @public {Property.<boolean>} (read-write)
this.inEditModeProperty = new Property( false ); // @public {Property.<boolean>}, indicates whether this expression is being edited
this.collectedProperty = new Property( false ); // @public {Property.<boolean>}, indicates whether this is in a collection box (for game)

// @public (read only), tracks the current in-progress animation, if any
//REVIEW: Type docs especially important here. No clue what type it takes besides null
this.inProgressAnimationProperty = new Property( null );

// @public (read only) indicates whether the 'combine halo' should be visible
// @public {Property.<boolean>} (read only) indicates whether the 'combine halo' should be visible
this.combineHaloActiveProperty = new Property( false );

// @public (read only) - size and state of the hints that can appear at left and right of the expression
Expand All @@ -77,34 +78,34 @@ define( function( require ) {
// observable arrays
//------------------------------------------------------------------------

// @public, read and listen only, items should be added and removed via methods
// @public {ObservableArray.<CoinTerm>}, read and listen only, items should be added and removed via methods
this.coinTerms = new ObservableArray();

//------------------------------------------------------------------------
// emitters
//------------------------------------------------------------------------

// @public, listen only, emits an event when an animation finishes and the destination is reached
// @public {Emitter}, listen only, emits an event when an animation finishes and the destination is reached
this.destinationReachedEmitter = new Emitter();

// @public, listen only, emits an event when this expression has been selected by the user to be edited
// @public {Emitter}, listen only, emits an event when this expression has been selected by the user to be edited
this.selectedForEditEmitter = new Emitter();

// @public, listen only, emits an event when the size of the expression or the relative positions of the coins
// @public {Emitter}, listen only, emits an event when the size of the expression or the relative positions of the coins
// change, generally used by the view so that it knows when to update, does NOT fire for position-only changes
// or for activation/deactivation of hints
this.layoutChangedEmitter = new Emitter();

// @public, listen only, emits an event when this expression should be broken apart
// @public {Emitter}, listen only, emits an event when this expression should be broken apart
this.breakApartEmitter = new Emitter();

//------------------------------------------------------------------------
// non-observable attributes
//------------------------------------------------------------------------

// @private, tracks coin terms that are hovering over this expression but are being controlled by the user so are
// not yet part of the expression. This is used to activate and size the hints. Coin terms should be added and
// removed via methods.
// @private {Array.<CoinTerm>}, tracks coin terms that are hovering over this expression but are being controlled by
// the user so are not yet part of the expression. This is used to activate and size the hints. Coin terms should
// be added and removed via methods.
this.hoveringCoinTerms = [];

// @private, tracks expressions that are hovering over this expression and would be combined with this one if
Expand Down Expand Up @@ -303,6 +304,7 @@ define( function( require ) {
}
},

// TODO: doc
dispose: function() {
this.disposeExpression();
},
Expand Down Expand Up @@ -337,7 +339,7 @@ define( function( require ) {
* Size the expression and, if necessary, move the contained coin terms so that all coin terms are appropriately
* positioned. This is generally done when something affects the view bounds of the coin terms, such as turning
* on coefficients or switching from coin view to variable view.
* {boolean} animate
* @param {boolean} animate
* @private
*/
updateSizeAndCoinTermPositions: function( animate ) {
Expand Down Expand Up @@ -410,6 +412,7 @@ define( function( require ) {
addCoinTerm: function( coinTerm ) {

if ( this.coinTerms.contains( coinTerm ) ) {
//REVIEW: This.
// TODO: There is a race condition that only occurs during fuzz testing where somehow a coin term that is
// inside an expression becomes user controlled and then is added back to the expression. This is a workaround.
// This should be fully investigated before publication. See
Expand Down
7 changes: 6 additions & 1 deletion js/common/model/ExpressionHint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ define( function( require ) {
*/
function ExpressionHint( anchorCoinTerm, movingCoinTerm ) {

// @public, read only
// @public {CoinTerm}, read only
this.anchorCoinTerm = anchorCoinTerm;
this.movingCoinTerm = movingCoinTerm;

// @public {boolean}
this.anchorOnLeft = anchorCoinTerm.positionProperty.get().x < movingCoinTerm.positionProperty.get().x;

// set the flag indicating that breaking apart is suppressed
Expand All @@ -36,20 +38,23 @@ define( function( require ) {
/**
* returns true if this expression hint includes the provided coin term
* @param {CoinTerm} coinTerm
* @returns {boolean}
* @public
*/
containsCoinTerm: function( coinTerm ) {
return ( coinTerm === this.anchorCoinTerm || coinTerm === this.movingCoinTerm );
},

// @public
//REVIEW: doc
equals: function( otherExpressionHint ) {
return ( otherExpressionHint.anchorCoinTerm === this.anchorCoinTerm &&
otherExpressionHint.movingCoinTerm === this.movingCoinTerm &&
otherExpressionHint.anchorOnLeft === this.anchorOnLeft
);
},

//REVIEW: doc
clear: function() {
this.anchorCoinTerm.breakApartAllowedProperty.set( true );
this.movingCoinTerm.breakApartAllowedProperty.set( true );
Expand Down
64 changes: 41 additions & 23 deletions js/common/model/ExpressionManipulationModel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ define( function( require ) {

/**
* @constructor
* {Object} options
* @param {Object} [options]
*/
function ExpressionManipulationModel( options ) {

Expand All @@ -56,38 +56,52 @@ define( function( require ) {
var initialViewMode = options.allowedRepresentations === AllowedRepresentationsEnum.VARIABLES_ONLY ?
ViewMode.VARIABLES : ViewMode.COINS;

this.viewModeProperty = new Property( initialViewMode ); // @public
this.showCoinValuesProperty = new Property( false ); // @public
this.showVariableValuesProperty = new Property( false ); // @public
this.showAllCoefficientsProperty = new Property( false ); // @public
this.xTermValueProperty = new Property( 2 ); // @public
this.yTermValueProperty = new Property( 5 ); // @public
this.zTermValueProperty = new Property( 10 ); // @public
this.totalValueProperty = new Property( 0 ); // @public, read-only
this.expressionBeingEditedProperty = new Property( null ); // @public, read-only, null when no expression is in edit mode
this.simplifyNegativesProperty = new Property( options.simplifyNegativesDefault ); // @public
// @public {Property.<ViewMode>}
this.viewModeProperty = new Property( initialViewMode );

// @public {Property.<boolean>}
this.showCoinValuesProperty = new Property( false );
this.showVariableValuesProperty = new Property( false );
this.showAllCoefficientsProperty = new Property( false );

// @public {Property.<number>}
this.xTermValueProperty = new Property( 2 );
this.yTermValueProperty = new Property( 5 );
this.zTermValueProperty = new Property( 10 );

// @public {Property.<number>} (read-only)
this.totalValueProperty = new Property( 0 );

// @public {???}, read-only, null when no expression is in edit mode
//REVIEW: type docs?
this.expressionBeingEditedProperty = new Property( null );

// @public {Property.<boolean>}
this.simplifyNegativesProperty = new Property( options.simplifyNegativesDefault );

var self = this;

// @public, read only, factory used to create coin terms
// @public {CoinTermFactory}, read only, factory used to create coin terms
this.coinTermFactory = new CoinTermFactory( this.xTermValueProperty, this.yTermValueProperty, this.zTermValueProperty );

// @public, read only, options that control what is available to the user to manipulate
// @public {AllowedRepresentationsEnum}, read only, options that control what is available to the user to manipulate
this.allowedRepresentations = options.allowedRepresentations;

// @public, read and listen only, list of all coin terms in the model
// @public {ObservableArray.<CoinTerm>}, read and listen only, list of all coin terms in the model
this.coinTerms = new ObservableArray();

// @public, read and listen only, list of expressions in the model
// @public {ObservableArray.<Expression>}, read and listen only, list of expressions in the model
this.expressions = new ObservableArray();

// @public, read and listen only, list of expression hints in the model
// @public {ObservableArray.<ExpressionHint}, read and listen only, list of expression hints in the model
this.expressionHints = new ObservableArray();

// @public (read-only) - coin terms that end up outside these bounds are moved back inside the bounds
// @public {Bounds2} (read-only) - coin terms that end up outside these bounds are moved back inside the bounds
this.coinTermRetrievalBounds = Bounds2.EVERYTHING;

// @public, read only - areas where expressions or coin terms can be collected, used only in game
// @public {Array.<EECollectionArea>}, read only - areas where expressions or coin terms can be collected, used
// only in game
//REVIEW: Type docs are correct? Added but was hard to find
this.collectionAreas = [];

/*
Expand Down Expand Up @@ -980,6 +994,7 @@ define( function( require ) {
},

// @public
//REVIEW: more docs
reset: function() {

// reset any collection areas that have been created
Expand Down Expand Up @@ -1008,6 +1023,7 @@ define( function( require ) {
},

// @private - test if coinTermB is in the "expression combine zone" of coinTermA
//REVIEW: more docs
isCoinTermInExpressionCombineZone: function( coinTermA, coinTermB ) {

// TODO: This could end up being a fair amount of allocations and may need some pre-allocated bounds for good performance
Expand All @@ -1024,6 +1040,7 @@ define( function( require ) {
/**
* returns true if coin term is currently part of an expression
* @param {CoinTerm} coinTerm
* @returns {boolean}
* @public
*/
isCoinTermInExpression: function( coinTerm ) {
Expand All @@ -1039,7 +1056,8 @@ define( function( require ) {
* Check for coin terms that are not already in expressions that are positioned such that they could combine with
* the provided coin into a new expression. If more than one possibility exists, the closest is returned. If none
* are found, null is returned.
* @param thisCoinTerm
* @param {CoinTerm} thisCoinTerm
* @returns {CoinTerm|null}
* @private
*/
checkForJoinableFreeCoinTerm: function( thisCoinTerm ) {
Expand Down Expand Up @@ -1126,9 +1144,9 @@ define( function( require ) {
return mostOverlappingLikeCoinTerm;
},

//REVIEW: docs
getOverlappingLikeCoinTermWithinExpression: function( coinTerm, expression ) {

var self = this;
var overlappingCoinTerm = null;

for ( var i = 0; i < expression.coinTerms.length; i++ ) {
Expand All @@ -1137,11 +1155,11 @@ define( function( require ) {
potentiallyOverlappingCoinTerm.existenceStrengthProperty.get() === 1 &&
potentiallyOverlappingCoinTerm.canCombineWith( coinTerm ) ) {
var overlapAmount = 0;
if ( self.viewModeProperty.get() === ViewMode.COINS ) {
overlapAmount = self.getCoinOverlapAmount( coinTerm, potentiallyOverlappingCoinTerm );
if ( this.viewModeProperty.get() === ViewMode.COINS ) {
overlapAmount = this.getCoinOverlapAmount( coinTerm, potentiallyOverlappingCoinTerm );
}
else {
overlapAmount = self.getViewBoundsOverlapAmount( coinTerm, potentiallyOverlappingCoinTerm );
overlapAmount = this.getViewBoundsOverlapAmount( coinTerm, potentiallyOverlappingCoinTerm );
}
if ( overlapAmount > 0 ) {
overlappingCoinTerm = potentiallyOverlappingCoinTerm;
Expand Down
Loading

0 comments on commit 2785ac2

Please sign in to comment.