-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Plotting: Add line markers #363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The markers look sweet!
I'm not sure it makes sense to couple them to Curve
though.
It seems likely users will want them to show e.g. a scatterplot, or to show the datapoint to which a smooth curve has been fitted. But right now it is more designed to highlight the corners of a coarse curve (like the demo shows), which is just a weird use case to me (but that may just be me). Such a coarse curve isn't very... well, curvy!think
Consider an alternative where markers where instead part of a separate Points
primitive. So you can add a curve and points, or just points, or just a curve. That way we can keep Curve
for smooth things (and add methods to e.g. show the derivative/tangent on hover) and use the markers for sparse point data.
What do you think?
@emilk Thanks for the review! I'm very much open to having a separate |
@emilk I've now created a separate let curve = Curve::new(ValueSeries::from_values_iter(sin));
let points = Points::new(ValueSeries::from_values_iter(sin));
ui.add(
Plot::new("Test Plot").curve(curve).points(points)
); |
One issue with this approach (apart from the duplicate code) is that points are always drawn on top of curves. But I want the user to have full control over the drawing order. One way to fix both issues might be to share an interface between |
Done, I'm quite happy with how this turned out. I also took the chance to rename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better!
I only have some minor notes
egui/src/widgets/plot/items.rs
Outdated
@@ -208,19 +265,295 @@ impl Curve { | |||
self | |||
} | |||
|
|||
/// Stroke color. Default is `Color32::TRANSPARENT` which means a color will be auto-assigned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer true?
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
@emilk Thanks for having another look. Much appreciated. I applied all your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
Closes #362
recording.mp4