magic numbers related to view transform

[pixelzoom]

BendingLightView has these 2 constructor parameters. They are poorly documented, but appear to be related to a necessary view transformation.

* @param {number} centerOffsetLeft - amount of space that center to be shifted to left
* @param {number} verticalOffset - how much to shift the model view transform in stage coordinates

Next are the magic numbers that appear at call sites.

In PrismView, we find 90 and -43:

BendingLightView.call( this,
      ....
      90,
      -43,
      ....
    );

In IntroView, we find centerOffsetLeft and 0:

BendingLightView.call( this,
      ...
      centerOffsetLeft,
      0,
      ....
    );

... and need to go to IntroScreen to find that centerOffsetLeft is in fact 102.

return new IntroView( model, 102, false, 2, function( introModel ) {

... and to MoreToolsScreen to find that (for this screen) centerOffsetLeft is 0:

IntroView.call( this, moreToolsModel, 0, true, 3, function( model )

So there are several problems here:

(1) First, why isn't a ModelViewTransform2 being used? This entire approach (as implemented) seems dubious, and unlike the approach typically used in PhET sims.

(2) Provide better documentation for the BendingLightView constructor parameters. Why are they needed? What do they apply to? Are they related? What would I need to know if I had to change them?

(3) Document magic numbers at call sites. How were these specific values chosen? If empirically, what did you look at to choose them? Again, what would I need to know to change them?

(4) Providing one of the 2 values (centerOffsetLeft) in the IntroView constructor is very confusing, and spreads the maintenance of the transform across multiple files.

[samreid]

@ariel-phet are we going to have another dev test before RC testing? If so, I'd like to address this beforehand.

[ariel-phet]

yes, we will have another dev test.

[samreid]

(1) First, why isn't a ModelViewTransform2 being used? This entire approach (as implemented) seems dubious, and unlike the approach typically used in PhET sims.

The ModelViewTransform2 is instantiated a few lines lower in the constructor.:

    // @public (read-only)
    this.modelViewTransform = ModelViewTransform2.createSinglePointScaleInvertedYMapping(
      new Vector2( 0, 0 ),
      new Vector2( 388 - centerOffsetLeft, stageHeight / 2 + verticalOffset ),
      scale
    );

[samreid]

(2) Provide better documentation for the BendingLightView constructor parameters. Why are they needed? What do they apply to? Are they related? What would I need to know if I had to change them?

I documented centerOffsetLeft and verticalOffset in the above commits. Here is the new JSDoc:

  /**
   * @param {BendingLightModel} bendingLightModel - main model of the simulations
   * @param {function} clampDragAngle - function that limits the angle of laser to its bounds
   * @param {function} clockwiseArrowNotAtMax - shows whether laser at max angle
   * @param {function} ccwArrowNotAtMax - shows whether laser at min angle
   * @param {function} laserTranslationRegion - region that defines laser translation
   * @param {function} laserRotationRegion - region that defines laser rotation
   * @param {string} laserImageName - name of laser image
   * @param {number} centerOffsetLeft - how much to shift the model view transform horizontally in stage coordinates.
   *                                  - The origin of each screen must be adjusted based on the layout of other control
   *                                  - panels.
   *                                  - Intro Screen: it is shifted far to the left since there is extra room
   *                                  - above the protractor toolbox for the laser to traverse to.
   *                                  - Prisms screen: it is shifted far to the left to center the play area in
   *                                  - the space between the left side of the screen and the control panels on the right
   *                                  - More Tools screen: it is not since there are equal sized control
   *                                  - panels on the right and left side of the screen.
   * @param {number} verticalOffset - how much to shift the model view transform vertically in stage coordinates
   *                                - In the prisms screen, it is shifted up a bit to center the play area above the south
   *                                - control panel.
   * @param {function} occlusionHandler - function that moves objects out from behind a control panel if dropped there
   * @constructor
   */

[samreid]

(3) Document magic numbers at call sites. How were these specific values chosen? If empirically, what did you look at to choose them? Again, what would I need to know to change them?

I also documented these in the above commits, for example:

      // center the play area horizontally in the space between the left side of the screen and the control panels on
      // the right
      90,

      // Center the play area vertically above the south control panel
      -43,

[samreid]

(4) Providing one of the 2 values (centerOffsetLeft) in the IntroView constructor is very confusing, and spreads the maintenance of the transform across multiple files.

The IntroView does not require any verticalOffset (now described as such in the BendingLightView constructor JSDoc), so this parameter is omitted here.

[pixelzoom]

This still seems unnecessarily complex, fragmented and spread across source files - meaning difficult to understand and maintain. Why not just pass a proper ModelViewTransform2 instance into each ScreenView subtype, and document each transform at the location where it's instantiated?

[samreid]

Why not just pass a proper ModelViewTransform2 instance into each ScreenView subtype

The ModelViewTransform2 instances only differ in 2 of the 9 matrix values. Passing in just the different values is one way of factoring out the other 7 identical values. Are you thinking of a static method createBendingLightModelViewTransform2 that can be used to create these similar ModelViewTransform2 instances without duplicating the other 7 values? Or some other strategy?

[pixelzoom]

The transform is instantiated at line 81 in BendingLightView:

    this.modelViewTransform = ModelViewTransform2.createSinglePointScaleInvertedYMapping(
      new Vector2( 0, 0 ),
      new Vector2( 388 - centerOffsetLeft, stageHeight / 2 + verticalOffset ),
      scale
    );

So yeah, I see the problem with passing in a ModelViewTransform instance. The viewPoint and scale are both dependent on the screen's height.

If I put this in BendingLightView constructor:

console.log( this.constructor.name + ' centerOffsetLeft=' + centerOffsetLeft + ' verticalOffset=' + verticalOffset );

I see this in requirejs mode:

IntroView centerOffsetLeft=102 verticalOffset=0
PrismsView centerOffsetLeft=90 verticalOffset=-43
MoreToolsView centerOffsetLeft=0 verticalOffset=0

So it looks to me like centerOffsetLeft:0, verticalOffset=0 would be reasonable defaults, assuming these are converted to options in #315.

[samreid]

The above commit is "Renamed centerOffsetLeft/verticalOffset to horizontalPlayAreaOffset/verticalPlayAreaOffset and moved to options, see #316", reassigning to @pixelzoom for review.

[pixelzoom]

:+1: Closing.