improve CanvasLinePlot API

[pixelzoom]

CanvasLinePlot is a high-performance way of drawing multiple plots using CanvasNode. Some issues with the API:

(1) The name is CanvasLinePlot (singular), when it's capable of drawing multiple plots. Rename to CanvasLinePlots (plural)?

(2) The style (stroke, lineWidth,...) of the plots is hardcoded, and all of the plots are rendered in the same style. The API needs a way to describe the style per plot. For example, Fourier uses a different stroke for each plot.

(3) Plot styles need to be mutable. For example, Fourier changes the plot strokes to grayscale when you pointer-over one of the amplitude sliders.

(4) The current hardcoded lineWidth is 0.1. That appears to be specific to DemoCanvasLinePlot, and is unlikely to be appropriate elsewhere.

Assigning to @samreid to discuss what needs to be included for "plot style". Definitely stroke and lineWidth.

I'll be using this in Fourier immediately.

[pixelzoom]

How about something like this?

  constructor( chartModel, dataSets, options ) {

    options = merge( {

      // Default style used for any plots that don't have entries in options.styles
      defaultStyle: {
        stroke: 'black',
        lineWidth: 1
      },

      // {{stroke:ColorDef, lineWidth:number}[]} one per dataSet, unspecified entries are filled in with options.defaultStyle
      styles: []
    }, options );

    // Populate one style per dataSet.
    assert && assert( options.styles.length <= dataSets.length, 'too many entries in options.styles' );
    for ( let i = 0; i < dataSets.length; i++ ) {
      options.styles[ i ] = merge( {}, options.defaultStyle, options.styles[ i ] || {} );
    }

    // @public if you change this directly, you are responsible for calling update
    this.styles = options.styles;

[samreid]

I have a more general proposal that will be ready to share shortly.

[pixelzoom]

@samreid and I discussed his more general approach on Slack. Basically a ChartCanvasNode that extends CanvasNode, has the view bounds of the ChartModel, and can draw a set of CanvasPainters that implement paintCanvas( context ). This approach looks really nice, definitely superior to my above proposal, and @samreid will continue to flesh it out.

[samreid]

I committed a first draft of this, @pixelzoom can you please review and test at your convenience?

[pixelzoom]

(1) The name is CanvasLinePlot (singular), when it's capable of drawing multiple plots. Rename to CanvasLinePlots (plural)?

@samreid your thoughts on this?

[samreid]

ChartCanvasLinePlot renders one data set. ChartCanvasNode renders multiple CanvasPainter instances.

[pixelzoom]

Got it, thanks for clarifying.

[pixelzoom]

@samreid FYI, a few changes in the above commits.

ChartCanvasLinePlot had a broken dispose method, looked like a copy-paste from LinePlot, so I deleted it.

ChartCanvasLinePlot didn't seem like the correct name, so I renamed to CanvasLinePlot --- the Canvas version of LinePlot.

ChartCanvasNode doesn't strike me as the best name, but I haven't thought of a better one yet. We want a name that says "responsible for drawing CanvasPainter instances" (assuming CanvasPainter is the best name).

Onward to integrating this into Fourier.

[pixelzoom]

One more thing that I changed accidentally (playing around) and reverted...

ChartCanvasLinePlot does not support {ColorDef} for options.stroke. In Fourier, the line plot colors are Properties, and are update dynamically.

[pixelzoom]

Hmm... ChartCanvasLinePlot apparent doesn't even support {Color} stroke. So I had to convert my colors to Color for PhET-iO, then convert them back to CSS for ChartCanvasLinePlot.

@samreid Which parts of {ColorDef} should ChartCanvasLinePlot support?

[samreid]

How about adding ColorDef.toCSS for starters? I'm not sure about listening for changes if it is a property--doesn't seem the responsibility of CanvasLinePlot.

[pixelzoom]

I had hoped to use this to improve performance in Fourier, especially while animating in "space & time" domain). But when adjusting amplitude with a slider (for example) using CanvasLinePlot requires redrawing all of the plots that are managed by the CanvasChartNode. LinePlot allows me to redraw only the plot that is changing. So I'm not sure that this is going to be a "win".

[samreid]

• First of all, calling paintCanvas on many curves is much faster than changing a LinePlot path shape and re-rendering it.
• Secondly, in Fourier (at least in Java), in the "space and time" harmonics chart, all curves are moving (not just one). This is the worst-case scenario that requires the most optimization.

[pixelzoom]

Indeed, it's not a problem when all of the curves are animating, because they all need to be redrawn. I said it's an issue "when adjusting amplitude with a slider", which requires updating 1 plot. And to be clear, I'm still going down this path, just didn't realize that if any plot changes, I have to redraw all plots. That's a significant change in the Fourier code, which is currently optimized to draw only the plots that have actually changed.

[pixelzoom]

ChartCanvasNode + CanvasLinePlot is much faster than LinePlot in Fourier.

Fourier always has 11 plots, one for each possible harmonic. Only the relevant ones should be drawn (based on numberOfHarmonicsProperty) and only if their amplitude is !== 0. With LinePlot, I was managing that with setVisible.

Thoughts on how to change the visibility of CanvasLinePlot specifically, and CanvasPainter generally? Do I need to change ChartCanvasNode this.painters and call update?

[samreid]

Are you saying that if a harmonicPlot.colorProperty changes, you would like to set the strokes to their current values? I'm happy to recombine the updates if you think it's better for the sim, reassign me if you would like me to do something like that.

[pixelzoom]

Yes. Reverted in the previous commit.

Reviewing the changes to CanvasLinePlot.

• setStroke is missing doc for @param stroke. What types are acceptable for stroke - ColorDef, PaintDef, some subset? Since you're using PaintColorProperty, I would have expected this to be OK:

plot.setStroke( plot.harmonic.colorProperty );

but it fails, and you changed to:

plot.setStroke( plot.harmonic.colorProperty.value );

Why?

EDIT: Ditto for option stroke: 'black', // {ColorDef}

[pixelzoom]

Are you saying that if a harmonicPlot.colorProperty changes, you would like to set the strokes to their current values?

Oh I see... You're saying that I need to call setStroke if colorProperty changes -- which can happen via colors.html or (perhaps) PhET-iO. Yes, I need to handle that. But no, I don't feel the need to break emphasizedHarmonicsListener into a bunch of little functions.

[samreid]

In the commit, I added PaintDef support in CanvasLinePlot.setStroke and added documentation. I updated Fourier to call with plot.setStroke( plot.harmonic.colorProperty ); instead of plot.setStroke( plot.harmonic.colorProperty.value );. The last part that's missing is a call to chartCanvasNode.update(); if a harmonic.colorProperty value changes. Would you like me to add that part?

[pixelzoom]

... Yes, I need to handle that. ...

It was already handled elsewhere. I factored it out so that it's more obvious.

[samreid]

It was already handled elsewhere. I factored it out so that it's more obvious.

I see it now, thanks!

[pixelzoom]

In the above commit, I chose to use colorProperty.value wherever setStroke is called, instead of colorProperty. This got me to thinking...

I find this (stroke is a Property) to be confusing:

      const colorProperty = new Property( Color.black );
      const plot = new CanvasLinePlot( ..., {
        stroke: colorProperty // this causes plot to listen to colorProperty
      } ); 
      const chartCanvasNode = new ChartCanvasNode( ..., [ plot ], ... );
      // CanvasLinePlot is observing colorProperty, so we only need to call update.
      colorProperty.lazyLink( () => chartCanvasNode.update() );

First, there's an ordering dependency here for the colorProperty listeners. CanvasLinePlot (its PaintColorProperty, actually) had better update its value before chartCanvasNode.update gets called.

Second, I suspect that developers won't totally understand what is going on with stroke and PaintColorProperty, and will tend to write one of these dubious listeners:

      colorProperty.lazyLink( color => { 
        plot.setStroke( color );  // this causes plot to no longer listen to colorProperty
        chartCanvasNode.update() 
     } );

// or
      colorProperty.lazyLink( () => { 
        plot.setStroke( colorProperty ); // this does nothing because useDeepEquality: false in CanvasLinePlot
        chartCanvasNode.update() 
     } );

We already have to listen to colorProperty to call chartCanvasNode.update. So setting stroke to colorProperty.value instead of colorProperty feels clearer and less error-prone. Like this:

      const colorProperty = new Property( Color.black );
      const plot = new CanvasLinePlot( harmonic, this.chartTransform, [], {
        stroke: colorProperty.value
      } ); 
      const chartCanvasNode = new ChartCanvasNode( ..., [ plot ], ... );
      colorProperty.lazyLink( color => {  
        plot.stroke = color;
        chartCanvasNode.update(); 
      } );

So... I'm thinking that we might want to revert PaintColorProperty, and restrict CanvasLinePlot stroke to be {Color|string|null}. And if not, I'll probably avoid passing a stroke Property to CanvasLinePlot, because imo it tends to obfuscate things (as I discovered in fourier HarmonicsChart).

[pixelzoom]

@samreid and I discussed the above comment, and agreed that constraining stroke to {Color|string|null} would be preferrable. I made some good progress on that, then got bogged down in the inconsistencies of how "color" is handled by Color, ColorDef, PaintDef, PaintColorProperty, etc. I briefly discussed with @jonathanolson on Slack, but he's not up to speed.

So... I bailed. Back to @samreid to see if he's inclined to try to move this forward.

[samreid]

Here's an implementation that uses {Color|string|null} (full file, not a diff). It also caches the CSS for high performance.

// Copyright 2020, University of Colorado Boulder

/**
 * CanvasLinePlot renders a {Array<Vector2|null>} dataSet on a canvas that is managed by a ChartCanvasNode.
 * Typically it is preferable to use LinePlot, but this alternative is provided for cases where canvas must be
 * used for performance. Like LinePlot, null values are skipped, and allow you to create gaps in a plot.
 * @see LinePlot
 *
 * @author Sam Reid (PhET Interactive Simulations)
 */

import merge from '../../phet-core/js/merge.js';
import Color from '../../scenery/js/util/Color.js';
import bamboo from './bamboo.js';
import CanvasPainter from './CanvasPainter.js';

class CanvasLinePlot extends CanvasPainter {

  /**
   * @param {ChartTransform} chartTransform
   * @param {Array<Vector2|null>} dataSet
   * @param {Object} [options]
   */
  constructor( chartTransform, dataSet, options ) {

    options = merge( {
      stroke: 'black', // {Color|string|null}
      lineWidth: 1
    }, options );

    super();

    // @private
    this.chartTransform = chartTransform;

    // @public if you change this directly, you are responsible for calling update on the corresponding ChartCanvasNode
    this.dataSet = dataSet;

    // @public if you change this directly, you are responsible for calling update on the corresponding ChartCanvasNode
    this.lineWidth = options.lineWidth;

    // @private - CSS for rendering the stroke
    this.strokeCSS = null; // updated in setStroke
    this.setStroke( options.stroke );
  }

  /**
   * Sets the stroke.
   * @param {Color|string|null} stroke - If you call setStroke, you are responsible for calling update on the associated ChartCanvasNode(s).
   * @public
   */
  setStroke( stroke ) {
    assert && assert( stroke instanceof Color || typeof stroke === 'string' || stroke === null, 'invalid stroke' );
    this.strokeCSS = stroke instanceof Color ? stroke.toCSS() : stroke;
  }

  // @public - see setStroke()
  set stroke( stroke ) {
    this.setStroke( stroke );
  }

  // @public
  dispose() {
    assert && assert( !this.isDisposed, 'already disposed' );
    this.isDisposed = true;
  }

  /**
   * Sets dataSet. You are responsible for calling update on the associated ChartCanvasNode(s).
   * @param {Vector2[]} dataSet
   * @public
   */
  setDataSet( dataSet ) {
    this.dataSet = dataSet;
  }

  /**
   * Intended to be called by ChartCanvasNode.
   * @param {CanvasRenderingContext2D} context
   * @public
   */
  paintCanvas( context ) {
    context.beginPath();

    if ( this.strokeCSS ) {
      context.strokeStyle = this.strokeCSS;
      context.lineWidth = this.lineWidth;

      let moveToNextPoint = true;

      // Only access the data set length once for performance
      const length = this.dataSet.length;
      for ( let i = 0; i < length; i++ ) {

        const dataPoint = this.dataSet[ i ];
        assert && assert( dataPoint === null || dataPoint.isFinite(), 'data points must be finite Vector2 or null' );

        // Draw a line segment to the next non-null value. Null values result in a gap (via move) in the plot.
        if ( dataPoint ) {
          const viewPoint = this.chartTransform.modelToViewPosition( dataPoint );
          if ( moveToNextPoint ) {
            context.moveTo( viewPoint.x, viewPoint.y );
            moveToNextPoint = false;
          }
          else {
            context.lineTo( viewPoint.x, viewPoint.y );
          }
        }
        else {
          moveToNextPoint = true;
        }
      }
      context.stroke();
    }
  }
}

bamboo.register( 'CanvasLinePlot', CanvasLinePlot );
export default CanvasLinePlot;

@pixelzoom does that look OK to you? Will Fourier need to be updated if we go with this approach?

[pixelzoom]

Looks good, but I would suggest one change:

- assert && assert( stroke instanceof Color || typeof stroke === 'string' || stroke === null, 'invalid stroke' );
+ assert && assert( stroke instanceof Color || ( typeof stroke === 'string' && Color.isCSSString( stroke ) ) || stroke === null, 'invalid stroke' );

I've confirmed that no changes to Fourier are required.

Feel free to close this issue after making the change and testing.

[samreid]

I made the change (with the CSS string test) and tested in Fourier. No problems noted, closing.