Awkward options in PhotonBeamNode

[samreid]

I noticed this in PhotonBeamNode:

  function PhotonBeamNode( canvasBounds, photonBeam, options ) {

    this.beamBounds = canvasBounds;
    this.photons = photonBeam.photons;
    this.color = photonBeam.color;

    options = _.extend( { pickable: false, canvasBounds: canvasBounds }, options );
...

I thought it was unusual to move a required parameter into the options parameter. Could be confusing if the client code is providing canvasBounds both as a required parameter and in the options. Maybe just move it to the options, and document that the client code should supply it in the options?

[aaronsamuel137]

I moved canvasBounds into options. I am also wondering, do we need this pickable: false option? I just left it there because it was there in the ShakerParticleNode that this was based on originally, but I don't have a clear understanding of why we want it. Assigning to @samreid for reveiw

[pixelzoom]

pickable:false not needed, it is the default. The only time you need to set pickable:false is if you have a node that is pickable:true (which typically means it has an input listener) and you want one or more of its children to be non-interactive. If it's not clear when you'd want to do that, let's discuss when we meet on Monday, I can show you examples.

[pixelzoom]

One more note about pickable:false… You'll see some vestigial uses of pickable:false in HTML5 sim code because the default in Scenery was changed. Most of those uses should be removed to prevent exactly the type of confusion that you experienced, but it's not high priority so unlikely to happen.

[aaronsamuel137]

Thanks for the explanation! I have removed pickable: false.

[samreid]

Looks good, thanks! Closing.