CanvasLinePlot's lack of support for color Property

[pixelzoom]

CanvasLinePlot does not support {Property} stroke because any change to CanvasLinePlot requires calling update for the parent ChartCanvasNode.

In practice, this is causing 2 problems:

(1) I'm constantly forgetting that stroke can't be a Property, and writing code like this that fails an assertion in CanvasLinePlot setStroke:

    const sumPlot = new CanvasLinePlot( this.chartTransform, [], {
      stroke: FMWColors.sumPlotStrokeProperty
    } );

(2) To use a color Property (e.g. a ProfileColorProperty) requires boilerplate like this, which occurs 3x in Fourier:

    const somePlot = new CanvasLinePlot( this.chartTransform, [], {
      stroke: someStrokeProperty.value
    } );

    const someChartCanvasNode = new ChartCanvasNode( this.chartTransform, [ somePlot ] );

    // CanvasLinePlot stroke does not support Property, so handle updates here.
    someStrokeProperty.link( stroke => {
      somePlot.setStroke( stroke );
      someChartCanvasNode.update();
    } );

I don't know how to resolve this, and maybe we don't. But I thought I'd create this issue, and do a little brainstorming, since this API is proving to be a little wonky in practice.

@samreid thoughts?

[samreid]

Brainstorming a proposal:

1. Change CanvasLinePlot to accept stroke: ColorDef
2. Add a changedEmitter property to CanvasPainter
3. When a CanvasPainter is registered on a ChartCanvasNode, the ChartCanvasNode listens for emits from the changedEmitter and automatically repaints itself.
4. A stroke change in CanvasLinePlot triggers CanvasPainter.changedEmitter.emit

[pixelzoom]

I've built something similar to that proposal into HarmonicPlot and InfiniteHarmonicPlot. Basically the same pattern as used in ChartTransform.

[pixelzoom]

I started to implement the proposal to add a changedEmitter to CanvasLinePlot. CanvasLinePlot got much more complicate. It was necessary to conditionally add/remove a listener if stroke was a Property. It was then necessary to implement dispose. And it also seemed like we'd need to add notification for changing lineWidth and dataSet -- making those fields private, and adding setters/getters. Anyway... I bailed, and I guess I'll continue to use the current API.

Closing.

[pixelzoom]

For https://github.com/phetsims/bamboo/issues/47... This issue was raised by @jonathanolson in the code-review for Fourier (https://github.com/phetsims/fourier-making-waves/issues/165. And I (unsuccessfully) tried to handle it in sim-specific code in https://github.com/phetsims/fourier-making-waves/issues/174. So it seems appropriate to reopen this issue - support for stroke Property is clearly something that CanvasLinePlot should have.

[pixelzoom]

Unassigning because I doubt that we'll see progress on this for awhile. Let me know when you'd like to discuss.

[samreid]

It seems good to touch base on this. There was a proposal in https://github.com/phetsims/bamboo/issues/42#issuecomment-897158907, and it seems that or something similar was investigated in https://github.com/phetsims/bamboo/issues/42#issuecomment-897802195. So perhaps we should create a dev meeting subgroup: @pixelzoom @jonathanolson and myself? I'll try that and see if that helps us come up with a plan.

[jonathanolson]

https://github.com/phetsims/bamboo/issues/42#issuecomment-897158907 seems workable, as long as it doesn't kill performance when a lot of painters are added/removed each frame (there may be cases like that?)

[pixelzoom]

+1 for @samreid's proposal in https://github.com/phetsims/bamboo/issues/42#issuecomment-897158907, with one caveat. The changedEmitter should fire for all aspects of CanvasLinePlot that can be mutated (e.g. data set). This dovetails with https://github.com/phetsims/bamboo/issues/45.

[samreid]

Unassigning since I am not scheduled any time for this part of bamboo in the current quarter. This issue will not be lost since I am the responsible dev for the repo.