search

phetsims / griddle

Dynamic charting library built with Scenery
MIT License
2 stars 4 forks source link

Delete XYPlotNode #63

Closed jessegreenberg closed 4 years ago

jessegreenberg commented 4 years ago

It looks like the only usage of XYPlotNode is currently in gene-expression-essentials. If that usage can be removed we can remove XYPlotNode and then rename ScrollingChartNode to something more general.

jessegreenberg commented 4 years ago

This would likely be the last thing needed for the mega-issue https://github.com/phetsims/griddle/issues/63.

jessegreenberg commented 4 years ago

There are no more usages of XYPlotNode. @samreid you are listed as an original author. Are you OK with removing XYPlotNode as well as XYDataSeriesNode, now that these can be replaced by ScrollingChartNode + DynamicSeriesNode? This would potentially close up #63.

Then we can rename ScrollingChartNode to something more general potentially.

samreid commented 4 years ago

Energy Skate Park is having a CT failure possibly related to this:

Uncaught Error: Assertion failed: object is not compatible with merge
Error: Assertion failed: object is not compatible with merge
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/assert/js/assert.js:22:13)
at assertIsMergeable (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/phet-core/js/merge.js:59:3)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/phet-core/js/merge.js:31:17
at arrayEach (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/sherpa/lib/lodash-4.17.4.js:537:11)
at Function.forEach (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/sherpa/lib/lodash-4.17.4.js:9359:14)
at merge (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/phet-core/js/merge.js:29:5)
at new ScrollingChartNode (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/griddle/js/ScrollingChartNode.js:45:15)
at new XYCursorPlot (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/griddle/js/XYCursorPlot.js:50:5)
at new EnergyPlot (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/energy-skate-park/js/graphs/view/EnergyPlot.js:63:5)
at new EnergyGraphAccordionBox (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1599610510818/energy-skate-park/js/graphs/view/EnergyGraphAccordionBox.js:86:24)
id: Bayes Chrome

Please reassign to me after that's addressed.

jessegreenberg commented 4 years ago

Sorry about that, fixed in https://github.com/phetsims/griddle/issues/66. Was from #65.

samreid commented 4 years ago

I removed them in the commit.

Then we can rename ScrollingChartNode to something more general potentially.

Is it general enough to just be ChartNode? What name do you recommend?

jessegreenberg commented 4 years ago

We still have BarChartNode, so distinguishing from that would be good. ScrollingChartNode can show both line and scatter representations. How about PlotNode? To me that could include line and scatter, but excludes bar charts.

samreid commented 4 years ago

That sounds reasonable to me, but I recommend you run it past @pixelzoom and perhaps other developers to get consensus.

pixelzoom commented 4 years ago

If griddle will only handle 2D data, then PlotNode is fine. If you think it might handle other dimensions, then it's not sufficiently descriptive - and neither is ScrollingChartNode, DynamicSeries, etc.

If I recall correctly, JFreeChart used "XY" prefix rather consistently for 2D charts, data sets, etc. and that made things super clear.

pixelzoom commented 4 years ago

Also note that you still have XYCursorPlot. It's a subclass of ScrollingChartNode with an interactive cursor, and the name doesn't tell me that.

jessegreenberg commented 4 years ago

I do like the XY prefix. I am not aware of plans for 3D, but this is clear and leaves it open. The old XYPlotNode was removed. A more general issue was opened for renaming several things in Griddle I am going to move this to #71.