search

phetsims / gene-expression-essentials

An educational simulation about how genes work to create proteins.
GNU General Public License v3.0
4 stars 6 forks source link

Replace XYPlotNode with ScrollingChartNode #132

Closed jessegreenberg closed 3 years ago

jessegreenberg commented 3 years ago

We would like to remove XYPlotNode and replace with ScrollingChartNode in https://github.com/phetsims/griddle/issues/63. This is the last sim to use the old XYPlotNode, if it can be replaced here it can be removed.

jessegreenberg commented 3 years ago

It looks like this plot needs labels along x axis but not y axis, something ScrollingChartNode doesn't support yet.

EDIT: I lied, theres an option for this.

jessegreenberg commented 3 years ago

Replacement was straight forward except that the path for data plotting looks a bit more jagged now:

image

jessegreenberg commented 3 years ago

Looks like DynamicSeriesNode needs lineJoin: round in the drawing code to look the same.

jessegreenberg commented 3 years ago

Done in the above commit. @jbphet would you like to take a look at the change since you are responsible dev?

jbphet commented 3 years ago

@jessegreenberg - Thanks for making this update, and the changes look good and the behavior seems correct. There is one thing that threw me though - it looks like ProteinLevelChartNode was changed to use ScrollingChartNode, but then ScrollingChartNode was renamed to XYPlotNode in f381ea2b92f5189ff08f844fb986a8fe39d02157, so if one looks at the code now it looks like the change described in the title of this issue never happened. It appears that the issue that motivated the change back was https://github.com/phetsims/griddle/issues/71.

This is all fine with me, but I wanted the information recorded here so that it doesn't confuse someone in the future (as it just confused me). I don't think there is anything left to do here, but I'm going to assign it to @jessegreenberg to make sure that my summary is correct. @jessegreenberg - if it is, you can just close.

jessegreenberg commented 3 years ago

Yes, https://github.com/phetsims/gene-expression-essentials/issues/132#issuecomment-715629080 is a correct summary of the history. Thanks for documenting that, closing.