Skip to content

Commit

Permalink
Some small cleanup items, see #153
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Sep 2, 2020
1 parent a8d73ee commit c104b0b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 14 deletions.
3 changes: 1 addition & 2 deletions js/common/model/BallSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ class BallSystem {
// @public (read-only) {AxonArray.<Ball>} - an array of the balls currently within the system. Balls
// **must** be from prepopulatedBalls. Its length should match the
// numberOfBallsProperty's value.
this.balls = new AxonArray( { elementOptions: { valueType: Ball } } );
//REVIEW: I can't find this elementOptions referenced, it looks like it used to exist? Can this be removed?
this.balls = new AxonArray();

// Observe when the number of Balls is manipulated by the user and, if so, add or remove the correct number of Balls
// to match the numberOfBallsProperty's value. The same Balls are in the system with the same number of Balls value.
Expand Down
17 changes: 5 additions & 12 deletions js/common/model/CenterOfMass.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import collisionLab from '../../collisionLab.js';
import Ball from './Ball.js';
import CollisionLabPath from './CollisionLabPath.js';

const scratchVector = new Vector2( 0, 0 );

class CenterOfMass {

/**
Expand Down Expand Up @@ -75,7 +77,7 @@ class CenterOfMass {
// This DerivedProperty is never disposed and persists for the lifetime of the sim.
this.velocityProperty = new DerivedProperty(
[ ...ballMassProperties, ...ballVelocityProperties, balls.lengthProperty ],
() => this.computeVelocity( balls ), { //REVIEW: computeVelocity doesn't take parameters anymore?
() => this.computeVelocity(), {
valueType: Vector2
} );

Expand Down Expand Up @@ -136,13 +138,7 @@ class CenterOfMass {
* @returns {number} - in kg.
*/
computeTotalBallSystemMass() {
//REVIEW: if helpful, return _.sumBy( this.balls, ball => ball.mass );
let totalMass = 0;

this.balls.forEach( ball => {
totalMass += ball.mass;
} );
return totalMass;
return _.sumBy( this.balls, ball => ball.mass );
}

/**
Expand All @@ -157,10 +153,7 @@ class CenterOfMass {
// Determine the total first moment (mass * position) of the system.
const totalFirstMoment = Vector2.ZERO.copy();
this.balls.forEach( ball => {
//REVIEW: I see effort to prevent GC here, if that's needed then you can have a scratch vector declared in the
//REVIEW: top-level scope such that this could be totalFirstMoment.add( scratchVector.set( ball.position ).multiply( ball.mass ) )
//REVIEW: so that the .times() wouldn't create garbage.
totalFirstMoment.add( ball.position.times( ball.mass ) );
totalFirstMoment.add( scratchVector.set( ball.position ).multiply( ball.mass ) );
} );

// The position of the center of mass is the total first moment divided by the total mass.
Expand Down

0 comments on commit c104b0b

Please sign in to comment.