Add bar charts and boxplots #499
Conversation
Replace PlotItem::series() by get_bounds() and closest()
| ]) | ||
| .width(0.7) | ||
| .name("Set 2") | ||
| .stack_on(&[&chart1]); |
There was a problem hiding this comment.
I'm not sure about this from a user perspective. I haven't thought about it too much but maybe it would make sense to have a StackedBarChart type or something like BarChart::new_stacked(<vector or something>).
egui/src/widgets/plot/items.rs
Outdated
| fn series_mut(&mut self) -> &mut Values; | ||
| fn get_bounds(&self) -> Bounds; | ||
| fn closest( |
There was a problem hiding this comment.
Give this function a more descriptive name.
| closest_dist_sq = dist_sq; | ||
| closest_value = Some(value); | ||
| closest_item = Some(item.name()); | ||
| if let Some(closest) = item.closest(ui, pointer, transform, *show_x, *show_y) { |
There was a problem hiding this comment.
This seems to create a bunch of shapes for all items and then throws all of them away except for maybe one set right? That is very inefficient.
There was a problem hiding this comment.
I just had an idea about that (commit 912b2f3)
There was a problem hiding this comment.
This looks better, I don't know enough about about the language to estimate the performance impact of creating potentially thousands of closures though. Did you consider splitting this into two functions, one to get the distance to the item and one to then get the hover shapes? Then you could do some min_by and then call the trait method to get the shapes just for that object.
There was a problem hiding this comment.
I considered the split with a simple distance_square and hover_shapes on PlotItem, but decided against because it required to calculate the distance twice. I think I can salvage it by putting an index instead of the closure, and having fn hover_shapes(index: usize) on PlotItem. I think at this point, I need to do some benchmark of the different ideas. I am not sure how to bench a gui app though, I saw some code in the repo. I will try to have something concrete later this week.
There was a problem hiding this comment.
Sorry for the delay, it took me some time to get the benchmarks somewhat clean. I implemented an alternative here niladic@24d6547. Also, after trying to implement a non boxed closure here, I found out that I was mistaken on the feasibility. It is not possible without existential types (unstable feature).
I benched (code here niladic@fd8e344 and niladic@ecf9074)
- code before this PR as baseline feature/add-barchart-v0-bench
- my first commit with all the allocs feature/add-barchart-v1-bench
- the closure implementation feature/add-barchart-v2-bench
- the alternative with indexes feature/add-barchart-v3-bench
Results are on this branch feature/add-barchart-bench-results. I tried to do it as clean as possible on my machine, but the benches are still a bit noisy. I would say they are really close to each other and there are no regressions in terms of perf. The most interesting bits are the boxed closure vs before this PR as baseline, the alternative vs before this PR as baseline and the alternative vs the boxed closure as baseline.
I also added some flamegraphs on what I think is high but somewhat realistic load:
- boxed closure 100 barcharts of 100 bars
- boxed closure 100 lines of 100 points
- boxed closure 100x100 points
- alternative 100 barcharts of 100 bars
- alternative 100 lines of 100 points
- alternative 100x100 points
To sum up, since there are no regressions and both implementations are comparable in terms of perf, I have no strong preference. The alternative with indexes might be simpler and easier to work with down the line. Tell me if you have any preference or other idea, otherwise I will go with the alternative one.
There was a problem hiding this comment.
Hey thanks a lot for doing those benchmarks! I agree that we should go with the alternative using indices.
|
Merged in #863 |
Following up on #467, this PR adds bar charts and boxplots. Both work and can be tested in the demo app (I am sorry for the amount of code...). I open this PR as a draft because it conflicts with #471, and there might be some debatable choices:
items.rsinto submodules. I don't do it for ease of review here, but can do it before merging.PlotItem::series()byget_bounds()andclosest()(because it is only used in the hover behavior). My idea here it to makePlotItemsufficiently abstract to allow for more complex drawings like bar charts and boxplots here.struct HoverElementwhich can cause some concerns about performance. I choose this solution because it is simpler, however I am totally open to change it for some kind of reference (I did not see a simple implementation, suggestions are welcome).dummy_values, it is possible to remove them, but this will conflict with More plot items #471 (I can cleanup at some point)In the plot demo, I put some values that were generated using(fixed)rand, but did not know if it was appropriate to add the crate as a dependency.