phetsims / griddle

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

Refactor griddle XYChartNode so it is more decorator/composition friendly #86

Closed samreid closed 3 years ago

samreid commented 3 years ago

There is a lot hard-coded into XYChartNode, and it is difficult to see how it can support every permutation of decoration or design we will want. Instead, let's try removing functionality from XYChartNode such as (what is currently known as the grid labels) and make it easier to add position-specific decorators on the chart. This can be used for rendering data, showing the cursor, showing the axes, showing tick labels, etc.

samreid commented 3 years ago

Question for designers: do we need to support nonlinear model->view mappings, for example, for log plots?

samreid commented 3 years ago

Notes from discussion with @jonathanolson:

Raw notes:

Mathematica gives defaults for each type of chart. Maybe give a higher-level type that does many good things by default, but leave them customizable Beneficial to have the separate components--that is a good direction. Higher level API and/or heuristics for automatic grids/labels would be nice. Grid Lines and tick marks we shoudl be able to specify the exact values. See D3 and ListPlot. Mathematica has heuristics for coming up with the plot range based on the data, and default grid lines. Heuristics for creating nice grid lines, or updating them when the data changes. When zooming in and out, it is difficult to provide complete design over all gridlines and everything--maybe heuristics are more important there. Should GridLineSet change logic or change spacing on zoom? Unclear, maybe either would work ok. JO: I recommend handling log plots in the transform. Treat them separately. Controlling x and y zoom separately. SR: Would you ever want mixture between the x and y? JO: That could be done with a preprocessing step in the model data. JO: Yes, we could think of a case with mixture, but that would probably warrant a separate implementation. Keep things rectilinear with independent axes. JO: Check out Mathematica range handling--it sets the range for x without having to re-specify the y range. JO: instead of specifying the xMap function and yMap function, specify the range + mapping function. Like "go from 0 to 100 logarithmically". https://reference.wolfram.com/language/ref/ListPlot.html PlotRange. Default is a good way to show it. Or you can pick "show a certain range". Check out commonly https://developers.google.com/chart/interactive/docs/gallery/linechart Sweet spot without painting into a corner. JO: I like the general approach, modular parts. JO: Maybe sure we aren't painting ourselves into a corner. SR: What about when layers need to know about each other? JO: Minor grid lines could know about major gridlines, or could have one type that does both. Performance? SR: Should ChartModel output to rectangle, or width x height? JO: I think width x height seems best. JO: If we have a more powerful grid line set, it could say "there is a grid line at this value, what should its stroke be?" JO: I really like this proposed approach. We will just need to think about what if some components need to know about other components. Like what if a label seeds to know not to overlap an axis JO: Thanks for doing this, I'm excited to use it.
samreid commented 3 years ago

Adding a reference to https://github.com/phetsims/fourier-making-waves/issues/3

samreid commented 3 years ago

Other questions and concerns:

samreid commented 3 years ago

@pixelzoom and I reviewed this today:

CM: This seems pretty nice. CM: Should we abandon griddle and just go towards bamboo?

SR: Should bamboo be nested in griddle? Or a separate repo? CM: It may be preferable in a separate repo. CM: Maybe ask another developer. Maybe JO.

SR: What else does Fourier need before this can be usable? CM: I can take it for a test drive.

CM: What about separating TickMarkSet from the TickMarkLabelSet? Better simple building blocks. SR: That sounds good.

CM: The zoom button group shouldn't be part of bamboo, but can be included in chart hierarchy if necessary (sim specific)

SR: Should we use one clip node, or clip each child separately? CM: One central point seems preferable. That may be better performance too.

CM: Fix the labeling thing, "edge" option "max" vs "min", that will be a barrier to usage.

SR: Should another developer code review this before usage in Fourier? CM: Fourier/common code has a new category. As I start to use it, I could review it as I go. As long as they don't mind it in this category.

CM: For next steps: who should review it? If CM is using it for Fourier, perhaps he is a good candidate. But will we need approval to spend time on that? One plan: I think this is a really nice start. We will need to decide whether it is a new repo, and some minor improvements. Then the sprint is over, and I'll try to use it in Fourier in a few weeks. Then I'll need some @samreid time for discussion, and I'll review it while I use it. Perhaps we should run this plan past @ariel-phet to make sure we are on a good path.

samreid commented 3 years ago

@ariel-phet agreed @pixelzoom can review this and continue with this, and it will count as part of the Fourier common code time.

@ariel-phet indicated that Fourier, Eating & Exercise and Moving Man will use this, so it makes sense for @samreid and @pixelzoom to take the lead.

I also pointed out that the CCK-AC scrolling chart nodes require features not yet supported by griddle, and that I may use bamboo for them, @ariel-phet said that makes sense.

samreid commented 3 years ago

I separated LabelSet out of TickMarkSet and fixed the min/max problem.

samreid commented 3 years ago

We moved this to a new repo "bamboo", closing.