generalize LaserPointerNode

[pixelzoom]

It's now used in MOTHA, BLL, bending-light.

[pixelzoom]

In MOTHA, it's implement programmatically in LightNode.

In BLL, it's an image file (light.png) used in LightNode.

In bending-light, it's 2 image files (laser.png, laser_knob.png), with or without a "knob". The laser has a knob if it's both rotatable and translatable, no knob if it's only rotatable. Very odd, because when a knob is present, the knob is used to do rotation.

[pixelzoom]

The name LightNode is a bit too generic for this. How about LaserPointerNode?

Here's the node:

Here's a real laser pointer:
http://martinjclemens.com/laser-pointers-the-new-ufo-menace/

@samreid What do you think about the name LaserPointerNode? Or something else?

[pixelzoom]

Generalization is done. Remaining tasks:

• move to scenery-phet, change namespace
• address memory leak in sun (see TODO in code, line 86)
• create issue to use in beers-law-lab
• create issue to use in bending-light (where it requires decoration with knob)

[samreid]

LaserPointerNode is the best name I've heard so far, with LaserNode a close second.

[pixelzoom]

In 29b5edf, I removed the short-circuit of button.dispose, after adding stubs to sun buttons. Note that LaserPointerNode will leak until those stubs are implemented.

[pixelzoom]

LaserPointerNode lives in scenery-phet, and all tasks in #2 (comment) have been completed.

LaserPointerNode is now used in beers-law-lab.LightNode, which had been instrumented for PhET-iO. LightNode was not calling tandem.addInstance, but it seemed like LaserPointerNode should. But without an answer to phetsims/beers-law-lab#131, I''m not sure.

Assigning to @samreid to:

(1) Verify that it's correct to call tandem.addInstance in LaserPointerNode, and

(2) Do a general review of LaserPointerNode (which may make sense to do at the same time as phetsims/bending-light#339)

[samreid]

The formatting is inconsistent here, I'll choose the most prevalent style which appears in 3 of the 4 sites:

    /**
     * Sets the enabled state of the laser's button.
     * @param {boolean} value
     * @public
     */
    setEnabled: function( value ) {
      this.button.enabled = value;
    },
    set enabled( value ) { this.setEnabled( value ); },

    /**
     * Gets the enabled state of the laser's button.
     * @returns {boolean}
     * @public
     */
    getEnabled: function() {return this.button.enabled; },
    get enabled() { return this.getEnabled(); }

[samreid]

I noticed this line:

options.children = [ nozzleNode, bodyNode, this.button ];

This means any children passed in through the options would be overwritten. Two potential solutions to this:

• assert that options.children is undefined or empty array before setting it
• append to the children array instead of overwriting it.

EDIT: I thought there would be a way to visualize multiple checkboxes in an issue, but couldn't find it, so I've removed the comment checkbox.

[samreid]

I noticed this default:

buttonTouchExpansion: 15,

But I thought we had decided that common code wouldn't expand touch areas by default?

[samreid]

We spent time on ProbeNode making sure the light came from the top left no matter how the ProbeNode was oriented, but it looks like the gradients in LaserPointerNode are hard-coded. Should we address this?

EDIT: Here's a picture of LaserPointerNode in MOTHA showing the light coming from the bottom left on the button, the left on the laser and the top-left on the control panel:

[samreid]

Is it redundant to put @public on a constructor?

[samreid]

Review summary: LaserPointerNode.js is in good shape. It is well organized and well-commented, nice work! If I have any other suggestions after integrating into bending light, I'll create separate issues. Back to you @pixelzoom.

[samreid]

One more thought, I saw this code:

tandem: options.tandem ? options.tandem.createTandem( 'button' ) : null

and realized it is a bit more idiomatic for us to use && instead in cases like this:

tandem: options.tandem && options.tandem.createTandem( 'button' )

[pixelzoom]

In #2 (comment), @samreid said:

But I thought we had decided that common code wouldn't expand touch areas by default?

We decided that common-code that was not PhET-specific (sun, scenery, etc.) would not set defaults, but that PhET-specific common-code (eg, scenery-phet) would set defaults that were appropriate for typical PhET uses. LaserPointerNode is in scenery-phet, so it's setting a default that's typically useful for PhET.

[pixelzoom]

In #2 (comment), @samreid asked:

Is it redundant to put @public on a constructor?

This is a more general discussion than I'd like to have here. If you want to pursue it, please open a general issue and label for developer-meeting.

[pixelzoom]

Addressed comment #2 (comment):

// add any children specified by the client
options.children = [ nozzleNode, bodyNode, this.button ].concat( options.children || [] );

[pixelzoom]

Re #2 (comment), lighting direction... Good point, I'll have a look.

@samreid How do you intend to address this in Bending Light, where the laser is dynamically rotated?

[pixelzoom]

LaserPointerNode added to 'Components' screen of scenery-phet demo application.

[pixelzoom]

Re lighting cues, there are 2 issues:

(1) The button:
The button may be either RoundMomentaryButton or RoundStickyToggleButton, neither of which has a convenient API for setting lighting cues. The easiest thing to do would be to rotate the button, by adding something like options.buttonRotation. Is that sufficient?

(2) The body:
As exhibited in bending-light, the orientation of this node can be arbitrary. I don't think we should attempt to address that. I think that the gradient direction should always be perpendicular to the direction that the laser is pointing, so that the specular highlight is parallel to the laser direction. If we agree on that, then the lighting can be changed via existing options: topColor, middleColor, bottomColor. And it may be useful to export the default values via LaserPointerNode.DEFAULT_OPTIONS. Does that sound sufficient?

Back to @samreid for feedback.

[pixelzoom]

I implemented what I described in #2 (comment) for lighting cues. I needed to add one additional option, highlightColorStop, in order to shift the specular highlight. See the scenery-phet demo for an example.

All review comments have been addressed. Back to @samreid for review.

[samreid]

Changes look great, the last thing I would recommend for this file is to document (in the DEFAULT_OPTIONS) for the nozzle if its width is parallel to the laser direction or perpendicular. For someone unfamiliar with the sim, they may think the nozzle width is how wide it is (perpendicular).

[samreid]

I guess the same argument applies to the laser body itself. The code appears that the width is along the parallel axis, which makes sense for a normal 2d rectangle, but not when referring to the dimensions of a laser pointer. Imagine a laser pointer pointing up like in the MOTHA example above. What would you call the laser pointer width in that image?

Not recommending a specific action at this point, just want to discuss it to understand what's best.

[pixelzoom]

The JSdoc defines this component's default orientation:

* Default orientation is pointing to the right. Origin is at right center (the edge of the output nozzle).

As with any UI component, I would expect the semantics of all options to be relative to the non-transformed orientation. And since we're dealing with the view coordinate frame, I would expect bodySize and nozzleSize (both Dimension2) to be view dimensions. So I think we should leave them as is.

[samreid]

Sounds good since developers will have to be familiar with the default orientation. I think we're done here, nice work.

[pixelzoom]

Thanks for the thorough review!