Refactor direction implementation to Direction.js using Enumeration m…

…apping. Addressing more REVIEW statements. phetsims/masses-and-springs#359

js/common/BuildAMoleculeQueryParameters.js

@@ -11,8 +11,7 @@ import buildAMolecule from '../buildAMolecule.js';
 const BuildAMoleculeQueryParameters = QueryStringMachine.getAll( {
 
   // Triggers a successfully completed collection. The user just needs to fill a single box to go to next collection.
-  //REVIEW: I see private:false now, documentation about why this is the correct choice would be good
-  easyMode: { type: 'flag', private: false },
+  easyMode: { type: 'flag', private: true },
 
   // Triggers console logs for information related to created molecules, collected molecules, and split molecules
   logData: { type: 'flag' }

js/common/model/Direction.js

@@ -11,44 +11,38 @@ import Vector2 from '../../../../dot/js/Vector2.js';
 import Enumeration from '../../../../phet-core/js/Enumeration.js';
 import buildAMolecule from '../../buildAMolecule.js';
 
-// constants
-const DirectionOrientation = Enumeration.byKeys( [ 'NORTH', 'EAST', 'SOUTH', 'WEST' ] );
-
-//REVIEW: a rich-style enumeration should probably be used here, using Enumeration.byMap() so that we can provide the
-//REVIEW: directions. We'll then get Direction.values for free, and this is essentially an enumeration.
-//REVIEW: Probably will need to use beforeFreeze to specify the connecting data, e.g. in that callback, you can
-//REVIEW: execute the "opposite direction" setters, etc.
-
-class Direction {
+class DirectionValue {
   /**
    * @param {Vector2} vector
-   * @param {string} id REVIEW: Currently doesn't seem to be a string, seems like it's enumeration values
    */
-  constructor( vector, id ) {
+  constructor( vector ) {
 
     // @public {Vector2}
     this.vector = vector;
 
-    // @public {number} REVIEW: Doesn't seem to be a string (as above) or a number (here), currently an enumeration value
-    this.id = id;
+    // @public {DirectionValue}
+    this.opposite = null;
   }
 }
 
-//REVIEW: If we're using a class, these would require visibilities/types. Prefer the Enumeration above significantly
-
-// Declare directions
-Direction.North = new Direction( new Vector2( 0, 1 ), DirectionOrientation.NORTH );
-Direction.East = new Direction( new Vector2( 1, 0 ), DirectionOrientation.EAST );
-Direction.South = new Direction( new Vector2( 0, -1 ), DirectionOrientation.SOUTH );
-Direction.West = new Direction( new Vector2( -1, 0 ), DirectionOrientation.WEST );
-
-// Declare direction opposites
-Direction.North.opposite = Direction.South;
-Direction.South.opposite = Direction.North;
-Direction.West.opposite = Direction.East;
-Direction.East.opposite = Direction.West;
-
-Direction.values = [ Direction.North, Direction.East, Direction.South, Direction.West ];
+// Declare cardinal direction values
+const NORTH = new DirectionValue( new Vector2( 0, 1 ) );
+const SOUTH = new DirectionValue( new Vector2( 0, -1 ) );
+const EAST = new DirectionValue( new Vector2( 1, 0 ) );
+const WEST = new DirectionValue( new Vector2( -1, 0 ) );
+
+// Declare opposites
+NORTH.opposite = SOUTH;
+SOUTH.opposite = NORTH;
+EAST.opposite = WEST;
+WEST.opposite = EAST;
+
+const Direction = Enumeration.byMap( {
+  NORTH: NORTH,
+  SOUTH: SOUTH,
+  EAST: EAST,
+  WEST: WEST
+} );
 
 buildAMolecule.register( 'Direction', Direction );
 export default Direction;

js/common/model/LewisDotModel.js

@@ -40,7 +40,7 @@ class LewisDotModel {
     const dotAtom = this.getLewisDotAtom( atom );
 
     // disconnect all of its bonds
-    Direction.values.forEach( direction => {
+    Direction.VALUES.forEach( direction => {
       if ( dotAtom && dotAtom.hasConnection( direction ) ) {
         const otherDotAtom = dotAtom.getLewisDotAtom( direction );
         this.breakBond( dotAtom.atom, otherDotAtom.atom );
@@ -89,7 +89,7 @@ class LewisDotModel {
   getOpenDirections( atom ) {
     const result = [];
     const dotAtom = this.getLewisDotAtom( atom );
-    Direction.values.forEach( direction => {
+    Direction.VALUES.forEach( direction => {
       if ( dotAtom && !dotAtom.hasConnection( direction ) ) {
         result.push( direction );
       }
@@ -108,7 +108,7 @@ class LewisDotModel {
   getBondDirection( a, b ) {
     const dotA = this.getLewisDotAtom( a );
     for ( let i = 0; i < 4; i++ ) {
-      const direction = Direction.values[ i ];
+      const direction = Direction.VALUES[ i ];
       if ( dotA && dotA.hasConnection( direction ) && dotA.getLewisDotAtom( direction ).atom === b ) {
         return direction;
       }
@@ -188,7 +188,7 @@ class LewisDotModel {
 
     // check all directions so we can explore all other atoms that need to be mapped
     for ( let i = 0; i < 4; i++ ) {
-      const direction = Direction.values[ i ];
+      const direction = Direction.VALUES[ i ];
       if ( dotAtom && dotAtom.hasConnection( direction ) ) {
         const otherDot = dotAtom.getLewisDotAtom( direction );
 
@@ -230,7 +230,7 @@ class LewisDotAtom {
 
     // @private {Object.<DirectionID:null|LewisDotAtom>}
     this.connections = {};
-    Direction.values.forEach( direction => {
+    Direction.VALUES.forEach( direction => {
       this.connections[ direction.id ] = null; // nothing in this direction
     } );
   }

js/common/view/BAMScreenView.js

@@ -11,7 +11,6 @@
 import Property from '../../../../axon/js/Property.js';
 import Bounds2 from '../../../../dot/js/Bounds2.js';
 import ScreenView from '../../../../joist/js/ScreenView.js';
-import Shape from '../../../../kite/js/Shape.js';
 import ThreeUtils from '../../../../mobius/js/ThreeUtils.js';
 import ResetAllButton from '../../../../scenery-phet/js/buttons/ResetAllButton.js';
 import DragListener from '../../../../scenery/js/listeners/DragListener.js';
@@ -116,7 +115,6 @@ class BAMScreenView extends ScreenView {
     this.refillButton.touchArea = this.refillButton.selfBounds.union( this.refillButton.childBounds ).dilated( 10 );
 
     // @public {function} Refill button is enabled if atoms exists outside of the bucket
-    //REVIEW: if this is marked as public with JSDoc, we really should move it to a method
     this.updateRefillButton = () => {
       this.refillButton.enabled = !this.bamModel.currentCollectionProperty.value.currentKitProperty.value.allBucketsFilled();
     };
@@ -144,8 +142,7 @@ class BAMScreenView extends ScreenView {
       right: this.layoutBounds.right - BAMConstants.VIEW_PADDING / 2,
       bottom: kitPanel.bottom
     } );
-    //RREVIEW: touchArea accepts {Bounds2}, no need for Shape.bounds wrapping
-    this.resetAllButton.touchArea = Shape.bounds( this.resetAllButton.bounds.dilated( 7 ) );
+    this.resetAllButton.touchArea = this.resetAllButton.bounds.dilated( 7 );
     this.addChild( this.resetAllButton );
     this.resetAllButton.moveToBack();
     this.addChild( this.refillButton );

js/common/view/MoleculeBondNode.js

@@ -22,6 +22,7 @@ import cursorImage from '../../../images/scissors_cur.js';
 import scissorsImage from '../../../images/scissors_png.js';
 import buildAMolecule from '../../buildAMolecule.js';
 import BAMConstants from '../BAMConstants.js';
+import Direction from '../model/Direction.js';
 
 //REVIEW: Imports don't match up with the image files, I would expect to see:
 //REVIEW: scissorsClosedUpImage, scissorsClosedUpImage, scissorsClosedImage, scissorsClosedImage, scissorsUpImage, scissorsUpImage, scissorsImage
@@ -69,7 +70,7 @@ class MoleculeBondNode extends Node {
 
     // Use the lewis dot model to get our bond direction
     const bondDirection = kit.getBondDirection( this.a, this.b );
-    const isHorizontal = bondDirection.id.name === 'WEST' || bondDirection.id.name === 'EAST';
+    const isHorizontal = bondDirection === Direction.WEST || bondDirection === Direction.EAST;
 
     // Define images for opened and closed scissors
     let openFile = 'scissors';

js/common/view/view3d/Molecule3DNode.js

@@ -11,14 +11,14 @@ import BooleanProperty from '../../../../../axon/js/BooleanProperty.js';
 import Bounds3 from '../../../../../dot/js/Bounds3.js';
 import Matrix3 from '../../../../../dot/js/Matrix3.js';
 import Quaternion from '../../../../../dot/js/Quaternion.js';
-import Vector2 from '../../../../../dot/js/Vector2.js';
 import Vector3 from '../../../../../dot/js/Vector3.js';
 import Arc from '../../../../../kite/js/segments/Arc.js';
 import EllipticalArc from '../../../../../kite/js/segments/EllipticalArc.js';
 import DOM from '../../../../../scenery/js/nodes/DOM.js';
 import Color from '../../../../../scenery/js/util/Color.js';
 import Utils from '../../../../../scenery/js/util/Utils.js';
 import buildAMolecule from '../../../buildAMolecule.js';
+import Vector2 from '../../../../../dot/js/Vector2.js';
 
 // constants
 // debug flag, specifies whether master transforms are tracked and printed to determine "pretty" setup transformations
@@ -142,63 +142,58 @@ class Molecule3DNode extends DOM {
    * @param {number} theta
    *
    * @private
-   * @returns {*}
+   * @returns {Object.<number,number>}
    */
   ellipticalArcCut( ra, rb, d, theta ) {
     if ( theta > Math.PI / 2 ) {
       // other one is in front, bail!
     }
 
-    // 2d circle-circle intersection point (ix,iy)
-    const ix = ( d * d + ra * ra - rb * rb ) / ( 2 * d );
-    const ixnorm = ix * ix / ( ra * ra );
+    // 2d circle-circle intersection point (interSectionPointX,interSectionPointY)
+    const interSectionPointX = ( d * d + ra * ra - rb * rb ) / ( 2 * d );
+    const ixnorm = interSectionPointX * interSectionPointX / ( ra * ra );
     if ( ixnorm > 1 ) {
       // one contains the other
       return null;
     }
-    const iy = ra * Math.sqrt( 1 - ixnorm );
+    const interSectionPointY = ra * Math.sqrt( 1 - ixnorm );
+    const interSectionPoint = new Vector2( interSectionPointX, interSectionPointY );
 
     // elliptical arc center
-    const cx = ix * Math.sin( theta );
-    const cy = 0;
+    const arcCenterX = interSectionPoint.x * Math.sin( theta );
+    const arcCenterY = 0;
+    const arcCenter = new Vector2( arcCenterX, arcCenterY );
 
     // elliptical semi-minor/major axes
-    const rx = iy * Math.cos( theta );
-    const ry = iy;
+    const ellipticalSemiMinor = interSectionPoint.y * Math.cos( theta );
+    const ellipticalSemiMajor = interSectionPoint.y;
 
-    // yes, tan( ix/iy ) converts to this, don't let your instincts tell you otherwise
-    const cutoffTheta = Math.atan2( ix, iy );
+    // yes, tan( interSectionPointX/interSectionPointY ) converts to this, don't let your instincts tell you otherwise
+    const cutoffTheta = Math.atan2( interSectionPoint.x, interSectionPoint.y );
 
     if ( theta < cutoffTheta - 1e-7 ) {
       // no arc needed
       return null;
     }
 
-    const nx = ix / ( ra * Math.sin( theta ) );
+    const nx = interSectionPoint.x / ( ra * Math.sin( theta ) );
 
     // start angle for our elliptical arc (from our ra circle's parametric frame)
-    const psi = Math.acos( nx );
+    const startAngle = Math.acos( nx );
 
     // start angle for our elliptical arc (from the elliptical arc's parametric frame)
-    const alpha = Math.atan2( ra * Math.sqrt( 1 - nx * nx ) / ry, ( ra * nx - cx ) / rx );
-
-    assert && assert( isFinite( rx ) );
+    const alpha = Math.atan2( ra * Math.sqrt( 1 - nx * nx ) / ellipticalSemiMajor, ( ra * nx - arcCenter.x ) / ellipticalSemiMinor );
 
-    //REVIEW: Maybe work this out with JO (Not the best example of documentation)
-    //REVIEW: JO: Looks like it's my fault! Usages somewhat give clues, can always doc in @returns with
-    //REVIEW: {ix:number, iy:number, ...}. Doesn't look performance-criticial anymore (just used for icon displays and
-    //REVIEW: things of that nature), so we could potentially properly type things, and switch things to Vector2?
-    //REVIEW: e.g. instead of ix/iy, it would be intersectionPoint:Vector2, etc.
+    assert && assert( isFinite( ellipticalSemiMinor ) );
     return {
-      ix: ix,
-      iy: iy,
-      cx: cx,
-      cy: cy,
-      rx: rx,
-      ry: ry,
-      nx: nx,
-      psi: psi,
-      alpha: alpha
+      interSectionPointX: interSectionPoint.x, // Vector2  intersectionPointX
+      interSectionPointY: interSectionPoint.y, // Vector2  intersectionPointY
+      arcCenterX: arcCenter.x, // Vector2  arcCenterX
+      arcCenterY: arcCenter.y, // Vector2  arcCenterY
+      ellipticalSemiMinor: ellipticalSemiMinor, // number  ellipticalSemiMinor
+      ellipticalSemiMajor: ellipticalSemiMajor, // number  ellipticalSemiMajor
+      startAngle: startAngle, // number startAngle
+      alpha: alpha // number
     };
   }
 
@@ -243,14 +238,14 @@ class Molecule3DNode extends DOM {
 
             // angle to center of ellipse
             const phi = Math.atan2( delta.y, delta.x );
-            const center = new Vector2( arcData.cx, arcData.cy ).rotated( phi );
+            const center = new Vector2( arcData.arcCenterX, arcData.arcCenterY ).rotated( phi );
             arcs.push( {
               center: center,
-              rx: arcData.rx,
-              ry: arcData.ry,
+              rx: arcData.ellipticalSemiMinor,
+              ry: arcData.ellipticalSemiMajor,
               rotation: phi,
-              circleStart: phi - arcData.psi,
-              circleEnd: phi + arcData.psi,
+              circleStart: phi - arcData.startAngle,
+              circleEnd: phi + arcData.startAngle,
               ellipseStart: -arcData.alpha,
               ellipseEnd: arcData.alpha
             } );