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

Improve plot item UX #1816

Merged
merged 15 commits into from Jul 24, 2022
Merged

Improve plot item UX #1816

merged 15 commits into from Jul 24, 2022

Conversation

EmbersArc
Copy link
Contributor

Improves the ergonomics of creating plot items. I also tried to address #1485 by allowing the user to pass borrowed data. I however couldn't find a way to do this for non-static life times. Maybe someone can find a way to add this as well.

This adds bytemuck as a dependency for the egui crate but I figured it's okay since all integrations have it as a dependency anyway. I can remove it again otherwise.

@EmbersArc EmbersArc mentioned this pull request Jul 10, 2022
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.

Overall, I like the new names (PlotPoint is much better than Value).

But let's make a bytemuck opt-in - sure the official integrations all use it, but not all 3rd party does, and as I noted it makes compiling egui a lot slower.

egui/src/widgets/plot/items/values.rs Outdated Show resolved Hide resolved
egui/src/widgets/plot/items/values.rs Outdated Show resolved Hide resolved
Comment on lines 175 to 181
pub fn new(points: Vec<[f64; 2]>) -> Self {
Self::Owned(bytemuck::cast_vec(points))
}

pub fn from_slice(points: &'v [[f64; 2]]) -> Self {
Self::Borrowed(bytemuck::cast_slice(points))
}
Copy link
Owner

Choose a reason for hiding this comment

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

bytemuck is nice, but it also makes compiling egui from scratch take ~twice as long (because of the syn crate). I'd rather keep the number of required dependencies low, so let's make bytemuck opt-in.

I think it's good to prefix these with #[cfg(feature = "bytemuck")]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. I removed the dependency again. I also removed PlotPoints::Borrowed for now because I couldn't find a way to to make it work with arbitrary lifetimes. Once somebody finds a way to get that working, it will make sense to add bytemuck again.

egui_demo_app/Cargo.toml Show resolved Hide resolved
Plot::Sigmoid => [x, sigmoid(x)],
}
})
.collect::<Vec<_>>(),
Copy link
Owner

Choose a reason for hiding this comment

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

Is .collect::<Vec<_>>() really more ergonomic than PlotPoints::from_iter(…) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I implemented FromIterator for PlotPoints and used this in the examples.

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.

Awesome!

@emilk
Copy link
Owner

emilk commented Jul 23, 2022

Closes #1843

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@emilk emilk merged commit 0bf9fc9 into emilk:master Jul 24, 2022
@EmbersArc EmbersArc deleted the optimize-plot branch July 24, 2022 17:07
@enomado
Copy link
Contributor

enomado commented Jul 27, 2022

Overall, I like the new names (PlotPoint is much better than Value).

I like Value. The idea is that you have some metric space of functions you draw, it translates into screen-plot-space. but you already name it PlotPoint.

In other words we have screen-space, plot-space, and value-space. but value-space is plot-space now.

That could be misunderstood

@emilk
Copy link
Owner

emilk commented Jul 28, 2022

There are only two coordinate systems though, afaict: the normal screen-space point-based coordinate system of egui/epaint, and the coordinate system of the data in the plot. Pos2 for one, PlotPoint for the other. I see no confusion there.

The reason I like "PlotPoint" is that it is descriptive (a point in plot space). "Value", on the other hand, is about as vague as it gets (rivaled only by "Data" and "Thing"). After all, what variable is not a value?

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.

None yet

3 participants