From 3356bd7526dae30932f93e6703f4f4b5cb214bde Mon Sep 17 00:00:00 2001 From: denz1994 Date: Mon, 11 May 2020 12:20:42 -0400 Subject: [PATCH] Refactor direction implementation to Direction.js using Enumeration mapping. Addressing more REVIEW statements. https://github.com/phetsims/masses-and-springs/issues/359 --- js/common/BuildAMoleculeQueryParameters.js | 3 +- js/common/model/Direction.js | 50 +++++++--------- js/common/model/LewisDotModel.js | 10 ++-- js/common/view/BAMScreenView.js | 5 +- js/common/view/MoleculeBondNode.js | 3 +- js/common/view/view3d/Molecule3DNode.js | 67 ++++++++++------------ 6 files changed, 62 insertions(+), 76 deletions(-) diff --git a/js/common/BuildAMoleculeQueryParameters.js b/js/common/BuildAMoleculeQueryParameters.js index a448d7e9..fa5203a1 100644 --- a/js/common/BuildAMoleculeQueryParameters.js +++ b/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' } diff --git a/js/common/model/Direction.js b/js/common/model/Direction.js index 190124f6..d4e69266 100644 --- a/js/common/model/Direction.js +++ b/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; \ No newline at end of file diff --git a/js/common/model/LewisDotModel.js b/js/common/model/LewisDotModel.js index f13a51ac..b5dd42e6 100644 --- a/js/common/model/LewisDotModel.js +++ b/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.} this.connections = {}; - Direction.values.forEach( direction => { + Direction.VALUES.forEach( direction => { this.connections[ direction.id ] = null; // nothing in this direction } ); } diff --git a/js/common/view/BAMScreenView.js b/js/common/view/BAMScreenView.js index 0e4bbd19..bab62a9f 100644 --- a/js/common/view/BAMScreenView.js +++ b/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 ); diff --git a/js/common/view/MoleculeBondNode.js b/js/common/view/MoleculeBondNode.js index f6587d2d..5fa724d4 100644 --- a/js/common/view/MoleculeBondNode.js +++ b/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'; diff --git a/js/common/view/view3d/Molecule3DNode.js b/js/common/view/view3d/Molecule3DNode.js index 764b683c..22ebbba4 100644 --- a/js/common/view/view3d/Molecule3DNode.js +++ b/js/common/view/view3d/Molecule3DNode.js @@ -11,7 +11,6 @@ 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'; @@ -19,6 +18,7 @@ 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.} */ 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 } );