Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow borrowing plot points by adding PlotPoints::Borrowed #35

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mo8it
Copy link

@mo8it mo8it commented Jul 31, 2024

  • I have followed the instructions in the PR template

Done the following TODO:

// Borrowed(&[PlotPoint]), // TODO(EmbersArc): Lifetimes are tricky in this case.

It is really an inner pain for me to clone a long Vec every time just to plot it.

Breaking changes

The addition of lifetimes can be a breaking change that should be communicated in the CHANGELOG.

Demo

I tested it using the following small demo:

use eframe::egui;
use egui_plot::{Line, Plot, PlotPoint, PlotPoints};

#[derive(Default)]
struct App {
    x: f64,
    points: Vec<PlotPoint>,
}

impl eframe::App for App {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        self.x += 0.1;
        self.points.push(PlotPoint::new(self.x, self.x.sin()));

        egui::CentralPanel::default().show(ctx, |ui| {
            let line = Line::new(PlotPoints::Borrowed(&self.points));
            Plot::new("sin").show(ui, |plot_ui| plot_ui.line(line));
        });
    }
}

fn main() -> eframe::Result {
    eframe::run_native(
        "Demo",
        Default::default(),
        Box::new(|_| Ok(Box::new(App::default()))),
    )
}

Please tell me where I should add a test and how you prefer it.

self,
ui: &mut Ui,
build_fn: impl FnOnce(&mut PlotUi) -> R + 'a,
build_fn: impl FnOnce(&mut PlotUi<'b>) -> R + 'a,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a tricky one.

The following example helped me with figuring out the correct lifetime annotation:

trait PlotItem: std::fmt::Debug {}

#[derive(Debug)]
struct Line<'a>(&'a [f64]);

impl<'a> PlotItem for Line<'a> {}

#[derive(Debug)]
struct PlotUi<'a>(Vec<Box<dyn PlotItem + 'a>>);

impl<'a> PlotUi<'a> {
    fn line(&mut self, line: Line<'a>) {
        self.0.push(Box::new(line));
    }
}

fn show<'a>(f: impl FnOnce(&mut PlotUi<'a>)) {
    let mut foo = PlotUi(Vec::new());
    f(&mut foo);
}

fn main() {
    let v = vec![1.0, 2.0];
    show(|plot_ui| {
        plot_ui.line(Line(&v));
        println!("{plot_ui:?}");
    });
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we should add that code as an example or test then 🤔

@riccardostokker
Copy link

riccardostokker commented Sep 13, 2024

Thank you for your work! I am working on a plot viewer that has to render a lot of points and this seems like one of the things that might improve its performance. Currently plotting 30000 points or so seems to bring the FPS down to 30 with just one plot.

Looking forward to this pull request being merged!

@mo8it
Copy link
Author

mo8it commented Sep 13, 2024

@riccardostokker Could you please try this branch in your project to see the performance benefit? You can do so by replacing egui_plot in Cargo.toml by the following line:

egui_plot = { git = "https://github.com/mo8it/egui_plot.git", branch = "borrow", version = "0.28.1" }

@mo8it
Copy link
Author

mo8it commented Sep 13, 2024

I just pushed a commit to relax the closure requirements of from_explicit_callback. The provided closure can borrow from its environment now. It was easy to do since PlotPoints already has a lifetime now.

@riccardostokker
Copy link

I am currently using a custom struct that stores the points that should be printed in the plot. This makes generating the vector of PlotPoint a problem since the f64 variables would have to be copied over anyway. Do you think we could convert PlotPoint to use references rather than owned variables? If you think it's something feasible I would be happy to help out.

This is how I am currently drawing my plots:

            for (circuit, waveform) in mappings {
                let points: Vec<[f64; 2]> = waveform
                    .x
                    .iter()
                    .zip(waveform.y.iter())
                    .map(|(&x, &y)| [x, y])
                    .collect();
                let points = PlotPoints::new(points);

                let line = Line::new(points).name(circuit.label.clone());

                lines.push(line);
            }

@mo8it
Copy link
Author

mo8it commented Sep 13, 2024

Why don't you merge the two fields waveform.x and waveform.y into one field points which is a vector of PlotPoint? Then you can use PlotPoints::Borrowed by only passing a reference to that vector.

@riccardostokker
Copy link

Why don't you merge the two fields waveform.x and waveform.y into one field points which is a vector of PlotPoint? Then you can use PlotPoints::Borrowed by only passing a reference to that vector.

The TimeSeries struct is coming from a different package that is not developed to work with egui so I cannot add a type from egui inside that definition.

@mo8it
Copy link
Author

mo8it commented Sep 13, 2024

Then we need a more flexible generator which takes Box<dyn IntoIterator<Item = PlotPoint> + 'a>. That would be a separate feature which can build on this PR.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

self,
ui: &mut Ui,
build_fn: impl FnOnce(&mut PlotUi) -> R + 'a,
build_fn: impl FnOnce(&mut PlotUi<'b>) -> R + 'a,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we should add that code as an example or test then 🤔

Comment on lines 155 to +160
///
/// These can be an owned `Vec` or generated with a function.
pub enum PlotPoints {
pub enum PlotPoints<'a> {
Owned(Vec<PlotPoint>),
Generator(ExplicitGenerator),
// Borrowed(&[PlotPoint]), // TODO(EmbersArc): Lifetimes are tricky in this case.
Generator(ExplicitGenerator<'a>),
Borrowed(&'a [PlotPoint]),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this docstring

@bircni
Copy link
Contributor

bircni commented Dec 4, 2024

@mo8it looks good - could you fix the changes emilk requested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants