Add better scene graph disposal patterns with Node

[zepumph]

From #1494, @jonathanolson and I have been brainstorming on how to add better support for dispose with Node and its children/parent relationship.

Here is one idea we have, which has a few different parts, each of which deserve their own brainstorm to improve upon.

TL:DR: Node will opt in to disposing children with a flag called disposeChildren, and each child will opt in to allowing that to proceed with disposeOnParentDispose. Both default to true, and each allow you to opt out of this new default behavior in given circumstances.

Part 1: Node changes: Something like this patch will get us pointed in the right direction to support these two options:

public disposeImplementation() {
if ( !this.isDisposed ) {
this.isDisposed = true;
this.dispose();
}

if( assert && this.dispose !== Disposable.prototype.dispose){
  assert( false, 'uh oh');
}

}

public dispose(): void {
assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
this._disposeEmitter.emit();
this.isDisposed = true;
}

3. Go to all DAG/dependency injection spots and mark as disposeOnParentDispose: false


Subject: [PATCH] combine tandemFragmentRenamed
---
Index: js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/Node.ts b/js/nodes/Node.ts
--- a/js/nodes/Node.ts	(revision 9a5aab10a87c1a085a5551d8b57f8ed1f124b086)
+++ b/js/nodes/Node.ts	(date 1674076532376)
@@ -256,6 +256,8 @@
   'touchArea', // Changes the area touches can interact with, see setTouchArea() for more documentation
   'clipArea', // Makes things outside of a shape invisible, see setClipArea() for more documentation
   'transformBounds', // Flag that makes bounds tighter, see setTransformBounds() for more documentation
+  'disposeChildren', // Flag when true, disposing this node will dispose all children.
+  'disposeOnParentDispose', // Flag when true, disposing this node will dispose all children.
   ...REQUIRES_BOUNDS_OPTION_KEYS,
   ...DISPOSABLE_OPTION_KEYS // From Disposable.ts
 ];
@@ -283,7 +285,9 @@
   cssTransform: false,
   excludeInvisible: false,
   webglScale: null,
-  preventFit: false
+  preventFit: false,
+  disposeChildren: true,
+  disposeOnParentDispose: true
 };
 
 export type RendererType = 'svg' | 'canvas' | 'webgl' | 'dom' | null;
@@ -353,6 +357,8 @@
   excludeInvisible?: boolean;
   webglScale?: number | null;
   preventFit?: boolean;
+  disposeChildren?: boolean;
+  disposeOnParentDispose?: boolean;
   mouseArea?: Shape | Bounds2 | null;
   touchArea?: Shape | Bounds2 | null;
   clipArea?: Shape | null;
@@ -691,6 +697,10 @@
   // Should be overridden by subtypes.
   public drawableMarkFlags!: string[];
 
+  // When true, disposing this Node will trigger disposing of its children.
+  private _disposeChildren: boolean;
+  private _disposeOnParentDispose: boolean;
+
   /**
    * Creates a Node with options.
    *
@@ -757,6 +767,8 @@
     this._maxHeight = DEFAULT_OPTIONS.maxHeight;
     this._appliedScaleFactor = 1;
     this._inputListeners = [];
+    this._disposeChildren = DEFAULT_OPTIONS.disposeChildren;
+    this._disposeOnParentDispose = DEFAULT_OPTIONS.disposeOnParentDispose;
 
     this.inputEnabledProperty.lazyLink( this.pdomBoundInputEnabledListener );
 
@@ -4981,6 +4993,66 @@
     return this._hints.preventFit;
   }
 
+  /**
+   * Sets the disposeChildren. When true, disposing this node will trigger disposal for all direct children.
+   */
+  public setDisposeChildren( disposeChildren: boolean ): void {
+    if ( disposeChildren !== this._disposeChildren ) {
+      this._disposeChildren = disposeChildren;
+    }
+  }
+
+  /**
+   * See setDisposeChildren() for more information
+   */
+  public set disposeChildren( value: boolean ) {
+    this.setDisposeChildren( value );
+  }
+
+  /**
+   * See isDisposeChildren() for more information
+   */
+  public get disposeChildren(): boolean {
+    return this.willDisposeChildren();
+  }
+
+  /**
+   * Returns whether the disposeChildren is set.
+   */
+  public willDisposeChildren(): boolean {
+    return this._disposeChildren;
+  }
+
+  /**
+   * Sets the disposeOnParentDispose. When true, disposing this node will trigger disposal for all direct children.
+   */
+  public setDisposeOnParentDispose( disposeOnParentDispose: boolean ): void {
+    if ( disposeOnParentDispose !== this._disposeOnParentDispose ) {
+      this._disposeOnParentDispose = disposeOnParentDispose;
+    }
+  }
+
+  /**
+   * See setDisposeOnParentDispose() for more information
+   */
+  public set disposeOnParentDispose( value: boolean ) {
+    this.setDisposeOnParentDispose( value );
+  }
+
+  /**
+   * See isDisposeOnParentDispose() for more information
+   */
+  public get disposeOnParentDispose(): boolean {
+    return this.willDisposeOnParentDispose();
+  }
+
+  /**
+   * Returns whether the disposeOnParentDispose is set.
+   */
+  public willDisposeOnParentDispose(): boolean {
+    return this._disposeOnParentDispose;
+  }
+
   /**
    * Sets whether there is a custom WebGL scale applied to the Canvas, and if so what scale.
    */
@@ -6451,8 +6523,19 @@
     // remove all PDOM input listeners
     this.disposeParallelDOM();
 
-    // When disposing, remove all children and parents. See https://github.com/phetsims/scenery/issues/629
+    // Reverse direction because disposing a child will detach from this array.
+    for ( let i = this._children.length - 1; i >= 0; i-- ) {
+      const child = this._children[ i ];
+      if ( this._disposeChildren && child._disposeOnParentDispose ) {
+        !child.isDisposed && child.dispose();
+      }
+    }
+
+    // When disposing, remove all children. See https://github.com/phetsims/scenery/issues/629
+    // No need to call this if all children were disposed above.
     this.removeAllChildren();
+
+    // When disposing, remove all parents. See https://github.com/phetsims/scenery/issues/629
     this.detach();
 
     // In opposite order of creation
Index: js/nodes/NodeTests.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/NodeTests.js b/js/nodes/NodeTests.js
--- a/js/nodes/NodeTests.js	(revision 9a5aab10a87c1a085a5551d8b57f8ed1f124b086)
+++ b/js/nodes/NodeTests.js	(date 1674074012965)
@@ -157,6 +157,23 @@
   assert.ok( trailMatrix.equalsEpsilon( nodeMatrix, epsilon ), 'Trail and Node transform equivalence' );
 } );
 
+QUnit.test( 'Node.disposeChildren', assert => {
+  const a = new Node( { disposeChildren: true } );
+  const b = new Node();
+  const c = new Node();
+  a.addChild( b );
+  b.addChild( c );
+
+  assert.ok( !a.isDisposed, 'a not disposed on startup' );
+  assert.ok( !b.isDisposed, 'b not disposed on startup' );
+  assert.ok( !c.isDisposed, 'c not disposed on startup' );
+  a.dispose();
+
+  assert.ok( a.isDisposed, 'a disposed' );
+  assert.ok( b.isDisposed, 'a should dispose child b' );
+  assert.ok( !c.isDisposed, 'a should no cascade through b to dispose ancestor c since b did not mark disposeChildren' );
+} );
+
 QUnit.test( 'Mutually exclusive options', assert => {
 
   assert.ok( true, 'always true, even when assertions are not on.' );

Part 2: With this change, we will most likely need to make a system-wide change to enable dispose() to be CALLED multiple times, but to only RUN once. This will likely be easiest with a rename of all dispose() prototype methods to something like disposeImplementation or hopefully something better. Then dispose() is only implemented a single time in Disposable, and calls disposeImplementation if the object hasn't already been disposed. Here is a patch to illustrate some of the nuance.

details

```typescript

public disposeImplementation():void {
if ( !this.isDisposed ) {
this.isDisposed = true;
this.dispose();
}

if( assert && this.dispose !== Disposable.prototype.dispose){
  assert( false, 'uh oh');
}

}

public dispose(): void {
assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
this._disposeEmitter.emit();
this.isDisposed = true;
}

</details>


**Part 3**: Then we have to cover all the cases in the project where we shouldn't be assuming that we can dispose our children. We should make an assertion here to help us find these. For example this Node is reused in each dynamic ACVoltage Node created as the image, but shouldn't be disposed each time: https://github.com/phetsims/circuit-construction-kit-common/blob/88ccbaf3df307aa64757d6308365dafb460826b2/js/view/ACVoltageNode.ts#L39-L44. It isn't clear how much work this part will be, likely a lot.


---------------------------

@jonathanolson and I came up with above idea, but in general we are not tied to it. We are just looking for what would be the best long term solution. It seemed like having those new Node options be opt-in is just a partial approach that will lead to more confusion without the benefits of the all-in behavior. Let's keep talking!

[zepumph]

Today @jonathanolson and I are leaning away from the above, and instead opting for both flags to default as false, and then either or will trigger child disposal on parent dispose.