Prefer children to addChild.

[pixelzoom]

For code review #41 ...

There's a significant performance difference between Node's children option and its addChild method that I thought would be worth bringing to your attention.

The current implementation of SyncIcon is:

export default class SyncIcon extends Node {
  public constructor( radius: number ) {
    super();
    const bottomArrowShape = new ResetShape( radius, { startAngle: Math.PI * 0.9, endAngle: -2 * Math.PI * 0.45 } );
    const topArrowShape = new ResetShape( radius, { startAngle: Math.PI * -0.09, endAngle: -2 * Math.PI * 0.45 } );

    const bottomArrow = new Path( bottomArrowShape, { fill: 'black' } );
    const topArrow = new Path( topArrowShape, { fill: 'black' } );

    this.addChild( bottomArrow );
    this.addChild( topArrow );
  }
}

A more efficient implementation would use the children option instead of addChild:

export default class SyncIcon extends Node {
  public constructor( radius: number ) {

    const bottomArrowShape = new ResetShape( radius, { startAngle: Math.PI * 0.9, endAngle: -2 * Math.PI * 0.45 } );
    const topArrowShape = new ResetShape( radius, { startAngle: Math.PI * -0.09, endAngle: -2 * Math.PI * 0.45 } );

    const bottomArrow = new Path( bottomArrowShape, { fill: 'black' } );
    const topArrow = new Path( topArrowShape, { fill: 'black' } );

    super( {
      children: [ bottomArrow, topArrow ]
    } );
  }
}

There's no significant performance difference in this specific case, because so few Nodes are involved. But (according to @jonathanolson) in general, children is much faster than addChild, particularly for anything that cares about children (e.g. layout containers). So if you get in the habit of preferring children to addChild across your entire code base, you can realize a performance improvement.

There's nothing that you need to change, just wanted you to be aware of this.

[marlitas]

@pixelzoom follow up question to this. On the BeakerNode review you mentioned that super.mutate is not very efficient either. Would setting this.children = [ child, child ] and then calling super.mutate( options ) be more efficient, less efficient, or approximately the same efficiency as this.addChild ?

[pixelzoom]

What BeakerNode is doing now is most efficient. It's setting options.children, then calling super( options ).

In general, calling super( options ) is more efficient that super() + this.mutate( options ). The former initializes correctly. The latter initializes incorrectly, then mutates to make things right, so it's doing more work.

So I would only use super() + this.mutate( options ) in cases where it's absolutely necessary. Those cases tend to be situations where your Node must have valid bounds in order to apply options.

As for what you asked about specifically... Doing this.children = (assignment) followed by this.mutate( options ) is a pattern to avoid. In fact, assigning to any field that can be set via options, then calling this.mutate( options ) should be avoided. If options contains a value for that field, you'll silently overwrite what you did with the assignment.

[marlitas]

Mmm... Okay. @pixelzoom what is the correct pattern to follow when super needs to be called before children are initialized?

For example: In IntroScreenView.ts super( options ) must be called before some children can be added since pipeMap has to be an instance variable to be called from step(). Is this a situation where using this.addChild is preferred? Is there another recommended pattern?

[pixelzoom]

A useful general pattern is to instantiate as a local const before super is called, then assign that const value to the field (instance variable) after super is called. In IntroScreenView.ts, create const pipeMap before the call to super. Assign to this.pipeMap after the call to super. Here's what that looks like:

export default class IntroScreenView extends MeanShareAndBalanceScreenView {

  // Keeps track of pipeNode's and their respective pipeModel for disposal.
  private readonly pipeMap: Map<PipeModel, PipeNode>;

  public constructor( model: IntroModel, providedOptions: LevelingOutScreenViewOptions ) {
    ...
    const pipeMap = new Map<PipeModel, PipeNode>();
    ...
    super( model, options );
    ...
    this.pipeMap = pipeMap;
  }
...
}

One case where this pattern doesn't work is when a superclass field is require to instantiate/initialize something. I.e., the super constructor needs to run first, so that you can use an inherited field to instantiate a Node (or something else) in your subclass. In that case, call super( options ) before instantiating child Node. Then make one assignment to this.children after all Nodes are instantiated, instead of making multiple this.addChild calls. Be sure to omit children from your subclass' options type -- you don't want to allow callers to pass children to your subclass constructor, then silently replace them by assigning to this.children.

For example:

// Superclass.ts

type SelfOptions = {...};
export type SuperclassOptions = SelfOptions & NodeOptions;

export default class Superclass extends Node {

  protected readonly someProperty: IProperty<...>;

  constructor( providedOptions?: ... ) {
     super( providedOptions );
     this.someProperty = new Property( ... );
  }
}

// Subclass.ts
type SelfOptions = {...};

// We'll be setting this.children, so do not allow the caller to provide children.
export type SubclassOptions = SelfOptions & StrictOmit< SuperclassOptions, 'children'>;

export class Subclass extends Superclass {

  private readonly someNode: Node;

  constructor( providedOptions?: ... ) {
    
   // We need this.someProperty to exist to instantiate this.someNode, so call super first.
   super( options );

   // We now have everything needed to instantiate this.someNode.
   this.someNode = new MyNode( this.someProperty, ... );

   // Instantiate other child Nodes ...
   ....

   // .. then set all children at once, instead of making multiple addChild calls.
   this.children = [ this.someNode, ... ];
  }
}

[pixelzoom]

I should also reiterate... For a small sim, you're unlikely to see much (or any) perceptible difference in performance between children vs addChild. Some developers would argue that you shouldn't worry about it, and only address it if it's a performance problem. I prefer to make "attention to performance" part of my style -- I think it results in a better product, and avoids some typical problems that others choose to address "later". So... It's up to you how/whether to integrate this recommendation into your programming style.

[marlitas]

Ran into hasChild needs to be called with a Node assertions. I thought I navigated them all, but am still running into the problem, and don't fully understand why. Will pair with @samreid on this. The following is the patch that got me here.

Index: js/intro/view/IntroScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/intro/view/IntroScreenView.ts b/js/intro/view/IntroScreenView.ts
--- a/js/intro/view/IntroScreenView.ts	(revision 2025b41c92ca6bd9a348c9a201dcd483d16499af)
+++ b/js/intro/view/IntroScreenView.ts	(date 1661452579544)
@@ -26,12 +26,13 @@
 import Dimension2 from '../../../../dot/js/Dimension2.js';
 import ValveNode from './ValveNode.js';
 import TableNode from './TableNode.js';
+import { ScreenViewOptions } from '../../../../joist/js/ScreenView.js';
+import { combineOptions } from '../../../../phet-core/js/optionize.js';
 
 
 type LevelingOutScreenViewOptions = MeanShareAndBalanceScreenViewOptions;
 
 export default class IntroScreenView extends MeanShareAndBalanceScreenView {
-  private readonly pipeNodes: PipeNode[];
   public readonly predictMeanVisibleProperty: Property<boolean>;
   public readonly meanVisibleProperty: Property<boolean>;
   public readonly tickMarksVisibleProperty: Property<boolean>;
@@ -39,17 +40,16 @@
   public constructor( model: IntroModel, providedOptions: LevelingOutScreenViewOptions ) {
 
     const options = providedOptions;
-    super( model, meanShareAndBalanceStrings.introQuestionProperty, MeanShareAndBalanceColors.introQuestionBarColorProperty, model.numberOfCupsProperty, options );
 
-    this.predictMeanVisibleProperty = new BooleanProperty( false, {
+    const predictMeanVisibleProperty = new BooleanProperty( false, {
       // phet-io
       tandem: options.tandem.createTandem( 'predictMeanVisibleProperty' )
     } );
-    this.meanVisibleProperty = new BooleanProperty( false, {
+    const meanVisibleProperty = new BooleanProperty( false, {
       // phet-io
       tandem: options.tandem.createTandem( 'meanVisibleProperty' )
     } );
-    this.tickMarksVisibleProperty = new BooleanProperty( false, {
+    const tickMarksVisibleProperty = new BooleanProperty( false, {
       // phet-io
       tandem: options.tandem.createTandem( 'tickMarksVisibleProperty' )
     } );
@@ -57,14 +57,12 @@
     const modelViewTransform2DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_2D_CENTER_Y ), MeanShareAndBalanceConstants.CUP_HEIGHT );
     const modelViewTransform3DCups = ModelViewTransform2.createSinglePointScaleInvertedYMapping( new Vector2( 0, 0 ), new Vector2( 0, MeanShareAndBalanceConstants.CUPS_3D_CENTER_Y ), MeanShareAndBalanceConstants.CUP_HEIGHT );
 
-    model.numberOfCupsProperty.lazyLink( () => this.interruptSubtreeInput );
-
     //Predict Mean Line that acts as a slider for alternative input.
     const predictMeanSlider = new PredictMeanSlider(
       model.meanPredictionProperty, model.dragRange,
       model.numberOfCupsProperty, () => model.getActive2DCups(),
       modelViewTransform2DCups, {
-        visibleProperty: this.predictMeanVisibleProperty,
+        visibleProperty: predictMeanVisibleProperty,
         valueProperty: model.meanPredictionProperty,
 
         // Constant range
@@ -81,32 +79,42 @@
       excludeInvisibleChildrenFromBounds: true
     } );
 
-    // 2D/3D water cup nodes addition and removal
-    // Center 2D & 3D cups
-    const checkboxGroupWidthOffset = ( MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH + MeanShareAndBalanceConstants.CONTROLS_HORIZONTAL_MARGIN ) / 2;
-    const cupsAreaCenterX = this.layoutBounds.centerX - checkboxGroupWidthOffset;
-    const centerWaterCupLayerNode = () => {
-      waterCupLayerNode.centerX = cupsAreaCenterX;
-      predictMeanSlider.x = waterCupLayerNode.x - 12.5;
-    };
+    // Configure layout
+    const controlPanel = new IntroControlPanel( tickMarksVisibleProperty, meanVisibleProperty, predictMeanVisibleProperty, options.tandem );
+
+    // Pipe toggle
+    const pipeSwitch = new ABSwitch( model.arePipesOpenProperty,
+      false, new ValveNode( new Vector2( 0, 0 ), new Property( 0 ), options.tandem.createTandem( 'closedValveIcon' ) ),
+      true, new ValveNode( new Vector2( 0, 0 ), new Property( Math.PI / 2 ), options.tandem.createTandem( 'openValveIcon' ) ), {
+        toggleSwitchOptions: {
+          size: new Dimension2( 40, 20 )
+        },
+
+        // phet-io
+        tandem: options.tandem.createTandem( 'pipeSwitch' )
+      } );
+
+    const tableNode = new TableNode( { centerX: 6, y: 200 } );
+
+    const combinedOptions = combineOptions<ScreenViewOptions>( { children: [ tableNode, waterCupLayerNode, predictMeanSlider ] }, options );
+
+    super( model, meanShareAndBalanceStrings.introQuestionProperty, MeanShareAndBalanceColors.introQuestionBarColorProperty, model.numberOfCupsProperty, combinedOptions );
 
     // add all cup nodes to the view
     model.waterCup2DArray.forEach( ( cupModel, index ) => {
-      const cupNode = new WaterCup2DNode( cupModel, model.waterCup3DArray[ index ], modelViewTransform2DCups, model.meanProperty, this.tickMarksVisibleProperty,
-        this.meanVisibleProperty, { tandem: options.tandem.createTandem( `waterCup2DNode${cupModel.linePlacement}` ) } );
+      const cupNode = new WaterCup2DNode( cupModel, model.waterCup3DArray[ index ], modelViewTransform2DCups, model.meanProperty, tickMarksVisibleProperty,
+        meanVisibleProperty, { tandem: options.tandem.createTandem( `waterCup2DNode${cupModel.linePlacement}` ) } );
       waterCupLayerNode.addChild( cupNode );
-      centerWaterCupLayerNode();
     } );
 
     model.waterCup3DArray.forEach( cupModel => {
-      const cupNode = new WaterCup3DNode( this.tickMarksVisibleProperty, model, cupModel, modelViewTransform3DCups, {
+      const cupNode = new WaterCup3DNode( tickMarksVisibleProperty, model, cupModel, modelViewTransform3DCups, {
         tandem: options.tandem.createTandem( `waterCup3DNode${cupModel.linePlacement}` )
       } );
       waterCupLayerNode.addChild( cupNode );
-      centerWaterCupLayerNode();
     } );
 
-    this.pipeNodes = model.pipeArray.map( pipeModel => {
+    model.pipeArray.map( pipeModel => {
       const index = model.pipeArray.indexOf( pipeModel );
       const pipeNode = new PipeNode( pipeModel, model.arePipesOpenProperty, modelViewTransform2DCups,
         { tandem: options.tandem.createTandem( `pipeNode${index}` ) } );
@@ -114,29 +122,22 @@
       return pipeNode;
     } );
 
+    // Center waterCups as they are activated and de-activated
+    const checkboxGroupWidthOffset = ( MeanShareAndBalanceConstants.MAX_CONTROLS_TEXT_WIDTH + MeanShareAndBalanceConstants.CONTROLS_HORIZONTAL_MARGIN ) / 2;
+    const cupsAreaCenterX = this.layoutBounds.centerX - checkboxGroupWidthOffset;
+    const centerWaterCupLayerNode = () => {
+      waterCupLayerNode.centerX = cupsAreaCenterX;
+      predictMeanSlider.x = waterCupLayerNode.x - 12.5;
+      tableNode.centerX = waterCupLayerNode.centerX;
+      tableNode.y = waterCupLayerNode.y - 28;
+    };
+
     model.numberOfCupsProperty.link( centerWaterCupLayerNode );
+    model.numberOfCupsProperty.lazyLink( () => this.interruptSubtreeInput );
 
-    // Configure layout
-    const controlPanel = new IntroControlPanel( this.tickMarksVisibleProperty, this.meanVisibleProperty, this.predictMeanVisibleProperty, options.tandem );
     this.controlsVBox.addChild( controlPanel );
-
-    // Pipe toggle
-    const pipeSwitch = new ABSwitch( model.arePipesOpenProperty,
-      false, new ValveNode( new Vector2( 0, 0 ), new Property( 0 ), options.tandem.createTandem( 'closedValveIcon' ) ),
-      true, new ValveNode( new Vector2( 0, 0 ), new Property( Math.PI / 2 ), options.tandem.createTandem( 'openValveIcon' ) ), {
-        toggleSwitchOptions: {
-          size: new Dimension2( 40, 20 )
-        },
-
-        // phet-io
-        tandem: options.tandem.createTandem( 'pipeSwitch' )
-      } );
     this.dataStateVBox.addChild( pipeSwitch );
 
-    this.addChild( new TableNode( { centerX: waterCupLayerNode.centerX - 6, y: waterCupLayerNode.bottom - 28 } ) );
-    this.addChild( waterCupLayerNode );
-    this.addChild( predictMeanSlider );
-
     this.pdomPlayAreaNode.pdomOrder = [
       waterCupLayerNode,
       controlPanel,
@@ -146,6 +147,10 @@
     this.pdomControlAreaNode.pdomOrder = [
       this.resetAllButton
     ];
+
+    this.predictMeanVisibleProperty = predictMeanVisibleProperty;
+    this.meanVisibleProperty = meanVisibleProperty;
+    this.tickMarksVisibleProperty = tickMarksVisibleProperty;
   }
 
   public override reset(): void {
Index: js/common/view/MeanShareAndBalanceScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MeanShareAndBalanceScreenView.ts b/js/common/view/MeanShareAndBalanceScreenView.ts
--- a/js/common/view/MeanShareAndBalanceScreenView.ts	(revision 2025b41c92ca6bd9a348c9a201dcd483d16499af)
+++ b/js/common/view/MeanShareAndBalanceScreenView.ts	(date 1661450593131)
@@ -138,6 +138,7 @@
       topMargin: MeanShareAndBalanceConstants.CONTROLS_VERTICAL_MARGIN
     } );
 
+    // refactoring this to use children is inefficient. Too many of the elements rely on the layoutBounds of the class instance
     this.addChild( this.questionBar );
     this.addChild( this.resetAllButton );
     this.addChild( this.controlsAlignBox );

[marlitas]

I have converted all the instances of .addChild to a children: implementation, that made sense in the context and capabilities of each file.

@pixelzoom, can you review and close if all seems good?

[pixelzoom]

Looks good overall. But there are a few places where 'children' needs to be omitted from options, see #103.

Closing.