Factor out a parent BambooPlot type

[samreid]

From #34, the duplicated code between e.g., BarPlot and UpDownArrowPlot should be factored out. Using TypeScript, we would probably use an abstract update method.

[samreid]

In #37 @pixelzoom asked:

I have been wondering if there should be a Plot.js base class. There's starting to be duplication, as well as some undesirable variation, amoung the plot classes. And if you're trying to create a new plot class, it's not totally clear what the required interface/API is.

[samreid]

I factored out AbstractPlot and it is working great for BarPlot and will work well for UpDownArrowPlot:

Index: js/AbstractPlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/AbstractPlot.js b/js/AbstractPlot.js
new file mode 100644
--- /dev/null	(date 1625952601309)
+++ b/js/AbstractPlot.js	(date 1625952601309)
@@ -0,0 +1,67 @@
+// Copyright 2020-2021, University of Colorado Boulder
+
+/**
+ * Abstract Plot base class that supports ChartTransform, data set and abstract method for update()
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+import Node from '../../scenery/js/nodes/Node.js';
+import bamboo from './bamboo.js';
+
+class AbstractPlot extends Node {
+
+  /**
+   * @param {ChartTransform} chartTransform
+   * @param {Array.<Vector2|null>} dataSet
+   * @param {Object} [options]
+   */
+  constructor( chartTransform, dataSet, options ) {
+
+    super( options );
+
+    // @private
+    this.chartTransform = chartTransform;
+
+    // @public if you change this directly, you are responsible for calling update
+    this.dataSet = dataSet;
+
+    // Update when the transform changes.
+    const changedListener = () => this.update();
+    chartTransform.changedEmitter.addListener( changedListener );
+
+    // @private
+    this.disposeAbstractPlot = () => chartTransform.changedEmitter.removeListener( changedListener );
+  }
+
+  /**
+   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
+   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
+   * @param {Vector2[]} dataSet
+   * @public
+   */
+  setDataSet( dataSet ) {
+    this.dataSet = dataSet;
+    this.update();
+  }
+
+  /**
+   * @public
+   * @abstract
+   */
+  update() {
+    throw new Error( 'update should be implemented in a subtype' );
+  }
+
+  /**
+   * @public
+   * @override
+   */
+  dispose() {
+    this.disposeAbstractPlot();
+    super.dispose();
+  }
+}
+
+bamboo.register( 'AbstractPlot', AbstractPlot );
+export default AbstractPlot;
\ No newline at end of file
Index: js/BarPlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/BarPlot.js b/js/BarPlot.js
--- a/js/BarPlot.js	(revision ba5302cf7dd0df74983d8a4b671db0ee8699a6ab)
+++ b/js/BarPlot.js	(date 1625952685151)
@@ -8,19 +8,19 @@
 
 import Vector2 from '../../dot/js/Vector2.js';
 import merge from '../../phet-core/js/merge.js';
-import Node from '../../scenery/js/nodes/Node.js';
 import Paintable from '../../scenery/js/nodes/Paintable.js';
 import Rectangle from '../../scenery/js/nodes/Rectangle.js';
+import AbstractPlot from './AbstractPlot.js';
 import bamboo from './bamboo.js';
 
 // constants
 const DEFAULT_PAINTABLE_OPTIONS = { fill: 'black' };
 
-class BarPlot extends Node {
+class BarPlot extends AbstractPlot {
 
   /**
    * @param {ChartTransform} chartTransform
-   * @param {Vector2[]} dataSet
+   * @param {Array.<Vector2|null>} dataSet
    * @param {Object} [options]
    */
   constructor( chartTransform, dataSet, options ) {
@@ -34,13 +34,7 @@
       pointToPaintableFields: point => DEFAULT_PAINTABLE_OPTIONS
     }, options );
 
-    super( options );
-
-    // @private
-    this.chartTransform = chartTransform;
-
-    // @public if you change this directly, you are responsible for calling update
-    this.dataSet = dataSet;
+    super( chartTransform, dataSet, options );
 
     // @private
     this.barWidth = options.barWidth;
@@ -50,24 +44,7 @@
 
     // @private {Rectangle[]}
     this.rectangles = [];
-    this.setDataSet( dataSet );
-
-    // Update when the transform changes.
-    const changedListener = () => this.update();
-    chartTransform.changedEmitter.addListener( changedListener );
 
-    // @private
-    this.disposeBarPlot = () => chartTransform.changedEmitter.removeListener( changedListener );
-  }
-
-  /**
-   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
-   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
-   * @param {Vector2[]} dataSet
-   * @public
-   */
-  setDataSet( dataSet ) {
-    this.dataSet = dataSet;
     this.update();
   }
 
@@ -107,15 +84,6 @@
       this.rectangles[ i ].mutate( providedOptions );
     }
   }
-
-  /**
-   * @public
-   * @override
-   */
-  dispose() {
-    this.disposeBarPlot();
-    super.dispose();
-  }
 }
 
 bamboo.register( 'BarPlot', BarPlot );

However, it cannot be used for any of the other Plots because they already extend something else. class CanvasLinePlot extends CanvasPainter , class LinearEquationPlot extends Line, class LinePlot extends Path, class ScatterPlot extends Path. @pixelzoom how do you recommend to proceed?

[pixelzoom]

I was going to suggest using composition for those other plot classes. That would work for LinearEquationPlot, LinePlot, and ScatterPlot -- and would be an improvement imo, because it wouldn't expose the APIs of Line and Path. But I see that ChartCanvasNode requires its plots to be CanvasPainter, and I'm not sure what to do there.

@samreid what do you think about using composition where possible? And does that spark ideas for how to address CanvasPainter?

[samreid]

I was going to suggest using composition for those other plot classes.

For instance, would LinearEquationPlot extend Plot and contain a Line, or vice versa?

[pixelzoom]

The former.

[samreid]

It also seems nice to keep the Plots as Nodes themselves, so usage reads like:

// Anything you want clipped goes in here
const chartClip = new Node( {
  clipArea: chartRectangle.getShape(),
  children: [
    new BarPlot( chartTransform, dataSet, {
      opacity: 0.1
    } ),
    new LinePlot( chartTransform, dataSet, {
      stroke: 'red',
      lineWidth: 2
    } )
  ]
} );

Instead of:

// Anything you want clipped goes in here
const chartClip = new Node( {
  children: [
    new BarPlot( chartTransform, dataSet, {
      opacity: 0.1
    } ).getNode(),
    new LinePlot( chartTransform, dataSet, {
      stroke: 'red',
      lineWidth: 2
    } ).getNode()
  ]
} );

@pixelzoom does this sound reasonable to you, that we would have a type like PlotNode extends Node and adds content via its Node children? On the other hand, this approach isn't compatible with CanvasPainter which should not be a Node. Another way to factor out this code would be with a mixin, but I don't want to add any more mixins until we are using TypeScript.

[pixelzoom]

It also seems nice to keep the Plots as Nodes themselves, ....

Yes, definitely. I don't like (e.g.) new BarPlot(...).getNode().

What if...?

• Node-based plots extend Plot, and use Line, Path, etc. via composition.
• Canvas-based plots extend CanvasPainter, and don't share the Plot API.

[samreid]

I got partway through this patch:

Index: js/LinePlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/LinePlot.js b/js/LinePlot.js
--- a/js/LinePlot.js	(revision ba5302cf7dd0df74983d8a4b671db0ee8699a6ab)
+++ b/js/LinePlot.js	(date 1626217798592)
@@ -14,8 +14,9 @@
 import merge from '../../phet-core/js/merge.js';
 import Path from '../../scenery/js/nodes/Path.js';
 import bamboo from './bamboo.js';
+import Plot from './Plot.js';
 
-class LinePlot extends Path {
+class LinePlot extends Plot {
 
   /**
    * @param {ChartTransform} chartTransform
@@ -25,39 +26,17 @@
   constructor( chartTransform, dataSet, options ) {
 
     options = merge( {
+      pathOptions: {
 
-      // Path options
-      stroke: 'black'
+        // Path options
+        stroke: 'black'
+      }
     }, options );
 
     super( null, options );
 
-    // @private
-    this.chartTransform = chartTransform;
-
-    // @public if you change this directly, you are responsible for calling update
-    this.dataSet = dataSet;
-
-    // Initialize
-    this.update();
-
-    // Update when the transform changes.
-    const changedListener = () => this.update();
-    chartTransform.changedEmitter.addListener( changedListener );
-
-    // @private
-    this.disposeLinePlot = () => chartTransform.changedEmitter.removeListener( changedListener );
-  }
-
-  /**
-   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
-   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
-   * @param {Vector2[]} dataSet
-   * @public
-   */
-  setDataSet( dataSet ) {
-    this.dataSet = dataSet;
-    this.update();
+    this.path = new Path( options.pathOptions );
+    this.addChild( this.path );
   }
 
   /**
@@ -86,16 +65,7 @@
         moveToNextPoint = true;
       }
     }
-    this.shape = shape;
-  }
-
-  /**
-   * @public
-   * @override
-   */
-  dispose() {
-    this.disposeLinePlot();
-    super.dispose();
+    this.path.shape = shape;
   }
 }
 
Index: js/demo/DemoLinePlot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/DemoLinePlot.js b/js/demo/DemoLinePlot.js
--- a/js/demo/DemoLinePlot.js	(revision ba5302cf7dd0df74983d8a4b671db0ee8699a6ab)
+++ b/js/demo/DemoLinePlot.js	(date 1626217912708)
@@ -99,8 +99,8 @@
           new AxisArrowNode( chartTransform, Orientation.VERTICAL ),
 
           // Some data
-          new LinePlot( chartTransform, createDataSet( -2, 2, 5 ), { stroke: 'red', lineWidth: 2 } ),
-          new LinePlot( chartTransform, createDataSet( -2, 2, 10 ), { stroke: 'green', lineWidth: 2 } ),
+          new LinePlot( chartTransform, createDataSet( -2, 2, 5 ), { pathOptions: { stroke: 'red', lineWidth: 2 } } ),
+          new LinePlot( chartTransform, createDataSet( -2, 2, 10 ), { pathOptions: { stroke: 'green', lineWidth: 2 } } ),
           new LinePlot( chartTransform, createDataSet( -2, 2, 20 ), { stroke: 'blue', lineWidth: 2 } ),
           new LinePlot( chartTransform, createDataSet( -2, 2, 30 ), { stroke: 'orange', lineWidth: 2 } )
         ]
Index: js/Plot.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Plot.js b/js/Plot.js
new file mode 100644
--- /dev/null	(date 1626217404613)
+++ b/js/Plot.js	(date 1626217404613)
@@ -0,0 +1,70 @@
+// Copyright 2020-2021, University of Colorado Boulder
+
+/**
+ * "Abstract" base class that includes typical conveniences for bamboo plots.
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+import Node from '../../scenery/js/nodes/Node.js';
+import bamboo from './bamboo.js';
+
+class Plot extends Node {
+
+  /**
+   * @param {ChartTransform} chartTransform
+   * @param {Vector2[]} dataSet
+   * @param {Object} [options]
+   */
+  constructor( chartTransform, dataSet, options ) {
+
+    super( options );
+
+    // @private
+    this.chartTransform = chartTransform;
+
+    // @public if you change this directly, you are responsible for calling update
+    this.dataSet = dataSet;
+
+    // Initialize
+    this.update();
+
+    // Update when the transform changes.
+    const changedListener = () => this.update();
+    chartTransform.changedEmitter.addListener( changedListener );
+
+    // @private
+    this.disposePlot = () => chartTransform.changedEmitter.removeListener( changedListener );
+  }
+
+  /**
+   * Sets the dataSet and redraws the plot. If instead the dataSet array is mutated, it is the client's responsibility
+   * to call `update` or make sure `update` is called elsewhere (say, if the chart scrolls in that frame).
+   * @param {Vector2[]} dataSet
+   * @public
+   */
+  setDataSet( dataSet ) {
+    this.dataSet = dataSet;
+    this.update();
+  }
+
+  /**
+   * Recomputes the rendered shape.
+   * @public
+   * @abstract
+   */
+  update() {
+  }
+
+  /**
+   * @public
+   * @override
+   */
+  dispose() {
+    this.disposePlot();
+    super.dispose();
+  }
+}
+
+bamboo.register( 'Plot', Plot );
+export default Plot;
\ No newline at end of file

When I realized we have to change the options API, because we can't pass the same options to 2 places (because of phet-io):

    options = merge( {
      pathOptions: {

        // Path options
        stroke: 'black'
      }
    }, options );

    super( null, options );

    this.path = new Path( options.pathOptions );

Therefore the call sites would change from:

new LinePlot( chartTransform, createDataSet( -2, 2, 5 ), { stroke: 'red', lineWidth: 2 } ),
new LinePlot( chartTransform, createDataSet( -2, 2, 10 ), { stroke: 'green', lineWidth: 2 } ),

to

new LinePlot( chartTransform, createDataSet( -2, 2, 5 ), { pathOptions: { stroke: 'red', lineWidth: 2 } } ),
new LinePlot( chartTransform, createDataSet( -2, 2, 10 ), { pathOptions: { stroke: 'green', lineWidth: 2 } } ),

Just thought I'd check in and see if that is a deal-breaker.

[pixelzoom]

Hmm, that does seem awkward.

How are you feeling about this issue @samreid? Should we bail, credit the duplication to JS's lack of interfaces, and somehow document (maybe with @typedef?) what the public API for all "plots" is expected to include? Or skip the doc, cross our fingers, and hope that we don't end up with different plot APIs?

[samreid]

There are 2 aspects of this issue--duplicated code and consistent interface. For the duplicated code, here is the code that appears in several files. I removed imports and comments for brevity.

class Plot extends Node {
  constructor( chartTransform, dataSet) {
    this.chartTransform = chartTransform;
    this.dataSet = dataSet;
    this.update();
    const changedListener = () => this.update();
    chartTransform.changedEmitter.addListener( changedListener );
    this.disposePlot = () => chartTransform.changedEmitter.removeListener( changedListener );
  }
  setDataSet( dataSet ) {
    this.dataSet = dataSet;
    this.update();
  }
  update(){
    // ...
  }
  dispose() {
    this.disposePlot();
    super.dispose();
  }
}

In my opinion, none of this is complicated or error-prone, or would cause problems if done slightly differently in a new Plot type.

Second, the public interface looks something like this:

class Plot extends Node {
  constructor( chartTransform, dataSet) {
    
    // @public
    this.chartTransform = chartTransform;
    
    // @public - but if you change it, call update
    this.dataSet = dataSet;
  }
  setDataSet( dataSet ) {
  }
  // abstract
  update(){
  }
  dispose() {
  }
}

Again, this is not complicated or error, prone and if another class did it slightly differently, we would probably find out when swapping Plot types.

None of the proposed approaches in this issues seem better than the problem we're solving. If we had to go with one, though, the best option so far is: change LinearEquationPlot to extend Path, then unify all 3 Path-based plots by extending a new type like PathPlot. However, I recommend revisiting this issue once we start using TypeScript, in that case we can determine whether to use an interface to enforce APIs and/or a mixin to share implementations.

[pixelzoom]

So is your proposal to defer this issue until we start using TypeScript? If so, that sounds good to me.

[samreid]

That sounds good, unassigning and deferring.

[samreid]

I labeled as priority-5:deferred, but then recalled these sentiments:
"status:on-hold" when an issue is waiting on another issue before it can move forward
"priority:5-deferred" when an issue seems unimportant and doesn't warrant attention in the near future (on its own merit)

So I switched the label to "on-hold", but could also add deferred if desired.