ChartCanvasNode and the need to call update.

[pixelzoom]

For https://github.com/phetsims/bamboo/issues/47 and https://github.com/phetsims/fourier-making-waves/issues/165 ...

@jonathanolson added this REVIEW comment to Foruier's SumChartNode.js:

    const sumPlot = new CanvasLinePlot( ... );
...
    const chartCanvasNode = new ChartCanvasNode( ..., [ sumPlot ], ... );
...
    // Display the data set.
    //REVIEW: This seems to be a common pattern with Data set Properties, any way to factor it out?
    sumDataSetProperty.lazyLink( dataSet => {
      sumPlot.setDataSet( dataSet );
      chartCanvasNode.update();
    } );

This is a problem that @samreid and I wrestled with previously in bamboo. ChartCanvasNode renders 1 or more CanvasLinePlot instances. CanvasLinePlot does not know about ChartCanvasNode. So when you make changes to a CanvasLinePlot, you are currently responsible for calling ChartCanvasNode.update.

This is well-documented in CanvasLinePlot, for example:

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

Despite the documentation, this is still a pain point - easy to forget, and (as @jonathanolson pointed out in the REVIEW comment) a common pattern that results in duplicated code.

But I should also point out... In terms of performance, there are also benefits here. In Fourier, I have a ChartCanvasNode that is rendering 97 CanvasLinePlot instances. I can update all 97 plots, then call update. If ChartCanvasNode updated each time one of its plots was updated, then I'd have big problems in Fourier. So perhaps having to call update is OK for something like this where performance is critical.

I don't plan to address this for Fourier because of the need to move forward. But let's discuss this again, adding @jonathanolson to the discussion.

[jonathanolson]

CanvasLinePlot does not know about ChartCanvasNode

That seems like something that could change, but I'm curious if it would affect performance.

[pixelzoom]

I unsuccessfully tried to address this in sim specific code, see https://github.com/phetsims/fourier-making-waves/issues/174.

[pixelzoom]

I said above:

But I should also point out... In terms of performance, there are also benefits here. ... having to call update is OK for something like this where performance is critical.

@jonathanolson pointed out that update is currently just invalidatePaint, which simply flags the Canvas as needing to be redrawn. So there should be no significant penalty for calling update many times.

[pixelzoom]

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

[samreid]

Perhaps the best way forward for this issue is to schedule a 15-minute mini dev subteam.

[pixelzoom]

Unassigning until we muster a subgroup meeting, which seems unlikely in the near future.

[samreid]

Do the assignees indicate the dev group subteam? I also want to unassign myself since I don't want to see this in my assigned issues until our subgroup meeting. But leaving myself as an assignee helps indicate that I'm part of the subgroup when it's time.

[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.