search

phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Investigate duplication related to CanvasLinePlot. #174

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

In a REVIEW comment for #165 code review, @jonathanolson asked in HarmonicsPlot.js:

    //REVIEW: Would HarmonicPlot/InfiniteHarmonicsPlot benefit from a supertype (dataSetProperty and colorProperty + emitter)?

It's true that HarmonicPlot and InfiniteHarmonicsPlot have some duplication for dealing with dataSetProperty and changeEmitter.

Because CanvasLinePlot doesn't support Property for its stroke, there's similar code for observing that Property and setting the stroke. And looking at other places that I've used CanvasLinePlot, there is similar color for every (?) instance of CanvasLinePlot. I have previously identified this as problematic in https://github.com/phetsims/bamboo/issues/42, but we didn't find a solution, and closed the issue.

I'm going to take another look at this. Maybe I can at least create a sim-specific subclass of CanvasLinePlot that addresses the duplication in this sim.

pixelzoom commented 2 years ago

Note that some of this duplication is related to https://github.com/phetsims/bamboo/issues/45 (ChartCanvasNode and the need to call update).

pixelzoom commented 2 years ago

There are 2 occurrences of extends CanvasLinePlot: HarmonicPlot, InfiniteHarmonicsPlot.

There are 6 occurrences of new CanvasLinePlot.

There are 0 occurrences of extends ChartCanvasNode.

There are 6 occurrences of new ChartCanvasNode.

pixelzoom commented 2 years ago

I went down the path of creating subclasses FMWCanvasLinePlot and FMWChartCanvasNode. The idea was to give FMWCanvasLinePlot a changedEmitter that would emit when setDataSet or setStroke was called, and have FMWChartCanvasNode listen to the changedEmitter for each of its "painters" to handle its own updates. And FMWCanvasLinePlot would also support a Property for stroke.

To make a long story short, I bailed, for the following reasons:

(1) Supporting the stroke Property got complicated - which is probably why we bailed on it in bamboo.

(2) It's unclear that these things need to be supported in bamboo. But how or if that support will be added is undetermined. So trying to cram that support into my own subclasses seems a little counterproductive. I think it's safer to use what's available now, warts and all.

(3) I started getting nervous about the number of changes required - the code is currently solid, and we've very close to the testing phase. It doesn't feel like the time to be making this change, especially given (2).

I did clean up a few things related to CanvasLinePlot stroke Properties in the above commit, so at least a little something good did come out of this issue.

Closing and "won't fix".