Node options should be passed to Node.call() when possible

[aaronsamuel137]

During code review #56 I noticed this code in PeriodTraceNode

    Node.call( this, options );

    this.rotation = Math.PI / 2;

It seems like the usual PhET convention would be to pass rotation in as one of the Node options like so:

    Node.call( this, _.extend( {
      rotation: Math.PI / 2
    }, options ) );

This way it is more clear that rotation is a Node a option and not a private attribute. Is there any reason why this isn't being done? Will you either change this or add some documentation. Thanks.

[aaronsamuel137]

A similar pattern occurs in PendulumLabView:

    var returnButtonNode = new ReturnButtonNode( {
      listener: pendulumLabModel.returnHandler.bind( pendulumLabModel )
    } );
    returnButtonNode.centerX = resetAllButton.bounds.minX - 75;
    returnButtonNode.centerY = height - SCREEN_PADDING.BOTTOM - 5;

[aaronsamuel137]

And in StopwatchNode:

    Timer.call( this, stopwatch.elapsedTimeProperty, stopwatch.isRunningProperty );

    this.centerX = toolsControlPanelNodeBounds.maxX - this.width / 2;
    this.centerY = toolsControlPanelNodeBounds.minY - this.height / 2 - 5;

[andrey-zelenkov]

Added using of Node.call in PeriodTraceNode and PendulumLabView. Center of StopwatchNode depend on Times's size and can be set only after creating of Timer. Reassign to @aaronsamuel137 for verification.

[pixelzoom]

@aaronsamuel137 Actually, the more common pattern is:

function MyNode( ..., options ) {

  options = _.extend( {
     // define options and their default values
  }, options );

  Node.call( this );

  // add children, do other things

  this.mutate( options );
}

If you pass options to Node.call before children are added, transformation-related options will cause an assertion error (something like "suspicious matrix operation") because the bounds of a Node are Bounds2.NOTHING by default. Doing this.mutate( options ) as the final step in the constructor ensures that the node's portion of the scene graph is defined before applying options.

[aaronsamuel137]

@pixelzoom, I'm aware of that pattern, but in these cases there was no need to call this.mutate( options ).

Would you suggest using that pattern everywhere for consistency even when it isn't necessary?

[pixelzoom]

No need to use that pattern everywhere. And I didn't look at the specific case is question here. But the example you gave was for Node, and Node.call( this, options ) is highly likely to cause an assertion failure. So for subtypes of Node, it's highly recommended to use the this.mutate pattern.

[aaronsamuel137]

OK thanks for pointing that out and clarifying.

[aaronsamuel137]

This looks good to me as far as these specific instances are concerned. Closing