Gip-Gip / egui-plotter

Simple to use utilties for integrating plotter into egui
MIT License
40 stars 15 forks source link

make chart data generic and mutable #1

Closed bananaturtlesandwich closed 1 year ago

bananaturtlesandwich commented 1 year ago
Gip-Gip commented 1 year ago

Sorry for not noticing this earlier, not sure why I didn't just do this. I'll take a look at it after I get some coffee. Changes made in the new version should be easy to implement.

Gip-Gip commented 1 year ago

I made the following minor changes:

I really appreciate the time you took to contribute! Anything else you can suggest? Any changes you don't like?

Gip-Gip commented 1 year ago

Also just adapted the code to 0.3.0, went pretty smoothly

bananaturtlesandwich commented 1 year ago

Just realised a few things

Gip-Gip commented 1 year ago
Gip-Gip commented 1 year ago

I'll decide everything tomorrow I need to wake up in 8 hours and go to work though so not the best time to fix this lol

bananaturtlesandwich commented 1 year ago

Gotcha thx for answering - for the first thing the only reason egui does that is because there was a deadlock issue which that resolved - that's not going to happen here so a mutable reference is just better I think

Gip-Gip commented 1 year ago
* you should probably put the builtin chart types behind a feature since most people won't use them

* std::time::Instant doesn't exist on wasm so the timechart stuff won't work there (the above would avoid this problem but wouldn't allow any future builtins or you could put the type in the example or you could use the instant crate but you don't really want another dependency so I think probably just put in the example)

I'm not quite sure how I'm going to get it working with wasm yet, apparently plotter supports it but it also requires backends to implement the Error trait, which is in the standard library only. I'll have to look at it when I have more time; I'm currently on lunch break, I'm a little confused.

bananaturtlesandwich commented 1 year ago

wasm isn't no_std it just doesn't have the time and fs modules due to platform agnostics

bananaturtlesandwich commented 1 year ago

Also you uncapitalised some of the code in the readme :p

Gip-Gip commented 1 year ago

Also you uncapitalised some of the code in the readme :p

I know there is a typo in the commit but it is fixed. Melatonin kicking in and I wanted to knock this out of the way lol

It's kinda embarrassing how many times I try to yank a block in vim and accidentally hit 'u'. And even more embarrassing in that I don't even notice it most of the time until I get compile errors.

I'll work on making charts a separate feature in another branch, I think this code achieves the goal it was set for though and I'll go ahead and merge it. Thanks again!

bananaturtlesandwich commented 1 year ago

Thanks!