review comments, #154

js/common/model/GeometricOpticsModel.js

@@ -59,7 +59,7 @@ class GeometricOpticsModel {
     // @private {RangeWithValue} - time range (in seconds) for the animation
     this.timeRange = new RangeWithValue( 0, ANIMATION_TIME, 0 );
 
-    // @public (read-only) {Property.<number>} - time for ray animation
+    // @public (read-only) {Property.<number>} - time for ray animation // REVIEW: in seconds or ms? Or better yet, I would recommend using the `range` and `units` options to NumberProperty just for clarity.
     this.timeProperty = new NumberProperty( this.timeRange.defaultValue );
 
     // @public {Property.<boolean>} the image/target can be seen if enabled
@@ -82,8 +82,9 @@ class GeometricOpticsModel {
     // @public {Object} rulers for the simulations
     this.rulers = {
       horizontal: new Ruler( HORIZONTAL_RULER_INITIAL_POSITION, HORIZONTAL_RULER_LENGTH ),
-      vertical: new Ruler( VERTICAL_RULER_INITIAL_POSITION, VERTICAL_RULER_LENGTH,
-        { orientation: Ruler.Orientation.VERTICAL } )
+      vertical: new Ruler( VERTICAL_RULER_INITIAL_POSITION, VERTICAL_RULER_LENGTH, {
+        orientation: Ruler.Orientation.VERTICAL
+      } )
     };
 
     // @public {Optic} - model of the optic
@@ -98,6 +99,8 @@ class GeometricOpticsModel {
     // @public {SourceObject} the object/ source
     this.sourceObject = new SourceObject( this.optic.positionProperty, this.representationProperty, tandem );
 
+    // REVIEW: perhaps instead of first/second, we could name these in optics terms, or relate them to the
+    // REVIEW: multiplicativeFactor? Like positiveFocalPoint, negativeFocalPoint? ordinal numbers here doesn't increase understanding to me.
     // @public {FocalPoint} first principal focal point
     this.firstFocalPoint = new FocalPoint( this.optic.positionProperty, this.optic.focalLengthProperty, {
       tandem: tandem.createTandem( 'firstFocalPoint' )

js/common/model/Optic.js

@@ -24,41 +24,44 @@ import geometricOptics from '../../geometricOptics.js';
 class Optic {
 
   /**
-   * @param {Vector2} position - center of the optical element
+   * @param {Vector2} initialPosition - center of the optical element
    * @param {RangeWithValue} radiusOfCurvatureRange - range of radius of curvature (in centimeters)
    * @param {RangeWithValue} diameterRange - range of height for optical element (in centimeters)
    * @param {RangeWithValue} indexOfRefractionRange
    * @param {Optic.Curve} curve - initial curve of optical element - acceptable values (CONVEX and CONCAVE)
    * @param {Optic.Type} type - type of optical element - acceptable values (MIRROR and LENS)
    * @param {Tandem} tandem
    */
-  constructor( position,
+  constructor( initialPosition,
                radiusOfCurvatureRange,
                diameterRange,
                indexOfRefractionRange,
                curve,
                type,
                tandem ) {
     assert && assert( tandem instanceof Tandem, 'invalid tandem' );
-    assert && assert( position instanceof Vector2, 'invalid position' );
+    assert && assert( initialPosition instanceof Vector2, 'invalid initialPosition' );
     assert && assert( radiusOfCurvatureRange instanceof RangeWithValue, 'invalid radiusOfCurvature' );
     assert && assert( diameterRange instanceof RangeWithValue, 'invalid diameterRange' );
 
     // @public {Property.<Vector2>} Position of the optical element
-    this.positionProperty = new Vector2Property( position );
+    this.positionProperty = new Vector2Property( initialPosition );
 
     // @public {Property.<number>} Radius of curvature of the optical element - The convention is positive as
     // converging.
-    this.radiusOfCurvatureProperty = new NumberProperty( radiusOfCurvatureRange.defaultValue,
-      { range: radiusOfCurvatureRange } );
+    this.radiusOfCurvatureProperty = new NumberProperty( radiusOfCurvatureRange.defaultValue, {
+      range: radiusOfCurvatureRange
+    } );
 
     // @public {Property.<number>} Height of the optical element - controls the optical aperture of the optical element
-    this.diameterProperty = new NumberProperty( diameterRange.defaultValue,
-      { range: diameterRange } );
+    this.diameterProperty = new NumberProperty( diameterRange.defaultValue, {
+      range: diameterRange
+    } );
 
     // @public {Property.<number>}  index of refraction of the lens
-    this.indexOfRefractionProperty = new NumberProperty( indexOfRefractionRange.defaultValue,
-      { range: indexOfRefractionRange } );
+    this.indexOfRefractionProperty = new NumberProperty( indexOfRefractionRange.defaultValue, {
+      range: indexOfRefractionRange
+    } );
 
     // @public {Property.<Optic.Curve>} Type of Curvature of the optical element.
     this.curveProperty = new EnumerationProperty( Optic.Curve, curve );
@@ -72,7 +75,7 @@ class Optic {
       [ this.radiusOfCurvatureProperty, this.indexOfRefractionProperty, this.curveProperty ],
       ( radiusOfCurvature, indexOfRefraction, curve ) => {
 
-        // a positive sign indicate the optic is converging
+        // a positive sign indicates the optic is converging
         // sign is determined based on the curve and the type of optic.
         const sign = this.getConvergingSign( curve );
 
@@ -100,6 +103,7 @@ class Optic {
         return this.getShapes( radius, diameter, curve );
       } );
 
+    // REVIEW: correct typedoc would be helpful here. Is it supported to just be a {Range}, or does it actually need to be one with a value now that the value was given to the NumberProperty above?
     // @private {number} diameter of the optic
     this.diameterRange = diameterRange;
   }

js/common/model/SourceObject.js

@@ -4,6 +4,12 @@
  * Model element for object (in the sense commonly used in geometric optic) or source of light
  * The sourceObject has a position and a "second source" position within it.
  *
+ * REVIEW: In general this type seems like it serves its purpose, but it feels really hard coded to the current, specific
+ * REVIEW: implementation of this sim. There is only room for two sources, and only room for the second one moving (and that
+ * REVIEW: is very tied to its representation). The source/object is a pretty general part of an optics model, so it would be neat
+ * REVIEW: if this could have a bit more flexibility, but I understand why we may not want to go back to implement that
+ * REVIEW: at this time. Basically this comment is an extension of https://github.com/phetsims/geometric-optics/issues/164
+ *
  * @author Martin Veillette
  */
 
@@ -59,9 +65,11 @@ class SourceObject {
     this.unconstrainedSecondSourcePositionProperty = new Vector2Property( DEFAULT_SOURCE_POINT_2 );
 
     // @private {Property.<number>} vertical offset (in centimeters) of second object with respect to the first
-    this.verticalOffsetProperty = new NumberProperty( SECOND_OBJECT_VERTICAL_RANGE.defaultValue,
-      { range: SECOND_OBJECT_VERTICAL_RANGE } );
+    this.verticalOffsetProperty = new NumberProperty( SECOND_OBJECT_VERTICAL_RANGE.defaultValue, {
+      range: SECOND_OBJECT_VERTICAL_RANGE
+    } );
 
+    // REVIEW: This is not just the initial position, as it is a Property that changes with the optic, right? If not this needs more explanation.
     // @public (read-only) {Vector2} initial position of the optic
     this.opticPositionProperty = opticPositionProperty;
 
@@ -91,7 +99,6 @@ class SourceObject {
       // {Vector2} update the left top position - the firstPosition is the ground truth when changing representation
       this.leftTopProperty.value = this.firstPositionProperty.value.plus( this.offsetPosition );
     } );
-
   }
 
   /**

js/common/model/Target.js

@@ -69,7 +69,8 @@ class Target {
 
     // the position of the focus as predicted by lens and mirror equation
     // @public (read-only) {Property.<Vector2>}
-    this.positionProperty = new DerivedProperty( [ objectPositionProperty,
+    this.positionProperty = new DerivedProperty( [
+        objectPositionProperty,
         optic.positionProperty,
         optic.focalLengthProperty ],
       ( objectPosition, opticPosition, focalLength ) => {
@@ -78,31 +79,34 @@ class Target {
 
     // @public (read-only) {Property.<number>}
     // the scale can be negative, indicating the target/image is inverted.
-    this.scaleProperty = new DerivedProperty( [ objectPositionProperty,
+    this.scaleProperty = new DerivedProperty( [
+        objectPositionProperty,
         optic.positionProperty,
         optic.focalLengthProperty ],
       ( objectPosition, opticPosition, focalLength ) => {
         return this.getScale( objectPosition, opticPosition, focalLength );
       } );
 
     // @public (read-only) {Property.<boolean>}
-    this.isInvertedProperty = new DerivedProperty( [ objectPositionProperty,
+    this.isInvertedProperty = new DerivedProperty( [
+        objectPositionProperty,
         optic.positionProperty,
         optic.focalLengthProperty ],
       () => {
         return this.isInverted();
       } );
 
     // @public (read-only) {Property.<boolean>}
-    this.isVirtualProperty = new DerivedProperty( [ objectPositionProperty,
+    this.isVirtualProperty = new DerivedProperty( [
+        objectPositionProperty,
         optic.positionProperty,
         optic.focalLengthProperty ],
       () => {
         return this.isVirtual();
       } );
 
     // @public (read-only) {Property.<bounds2>}
-    // Bounds of the actual IMAGE  based on the representation
+    // Bounds of the actual Image  based on the Representation
     this.boundsProperty = new DerivedProperty( [
         this.positionProperty,
         representationProperty,

js/common/view/SourceObjectNode.js

@@ -95,6 +95,8 @@ class SourceObjectNode extends Node {
       visibleBounds => {
         return new Bounds2( visibleBounds.minX,
           visibleBounds.minY + sourceObject.boundsProperty.value.height,
+
+          // REVIEW: This feels like a code small to get the optic position from the source here. The optic position is important to the model, but it seems weird that we get the position from the source object. Does it feel like too close of coupling to you?
           sourceObject.getOpticPosition().x - sourceObject.boundsProperty.value.width,
           visibleBounds.maxY );
       } );