feat: Add spans for plots #152
Conversation
There was a problem hiding this comment.
@azerupi thanks for the PR, it's very useful!
Feedback:
- The spans are drawn as plot items and therefore they show up in the legend. The structure in the code makes sense but I'm not sure if the spans should be displayed in the legend. With a lot of spans on a plot this could become quite messy. The name of the spans are displayed in the plot themselves already.
Agreed. I think it rather makes sense to implement them as PlotItems than the current PlotItemBase.
There is a similar implementation in this fork: 0xb-s#39 (for inspiration) but I think yours is better because of the labels.
- The names of the spans are drawn in over them for spans on the x-axis they are drawn at the top-center of the span and for spans on the y-axis they are drawn at the left-middle of the span. When zooming out and the span is too small to display the name it will get truncated and ultimately hidden. I think this looks good and is functional, but this is all very arbitrary so I'm open for other solutions.
I like it! <3
- The spans being plot items means that they also get drawn in the order they are called in the show closure. This means it could be drawn on top of the line plot. This has advantages and disadvantages. I'm not sure if we want to have spans always be drawn in the background of other plot items?
I believe spans should be drawn in the background, and other plot items should be overlayed. If it was photoshop layers, I'd make it 0. background (ticks etc), 1. spans 2. plot items (lines, markers, etc.)
Other things that would be great to add:
- Ability to choose the vertical/horizontal placement of the labels.
- Semiopen intervals: allow +/- inf spans.
| rect_elem::highlighted_color, | ||
| }; | ||
|
|
||
| const LABEL_PADDING: f32 = 4.0; |
There was a problem hiding this comment.
Please add doc comment, which sides the padding applies to (I think all?)
|
|
||
| // If the span is too small to display the full name, find the longest name | ||
| // with "..." appended that we can display within the span | ||
| fn find_name_candidate(&self, width: f32, painter: &Painter, font_id: &FontId) -> String { |
There was a problem hiding this comment.
Let's extract this as a standalone util function, it can be used in other places.
|
Thank you @michalsustr! This is great feedback and exactly what I was looking for. |
|
Great, looking forward! |
|
I was working on the code base over the weekend and got more familiar with it. I changed my mind: I think it should keep being |
|
Sounds good! |
|
@michalsustr just to be sure, you still want me to draw the spans on in the background even if we keep them as |
2 participants
This PR adds supports for spans in plots on either axis. This allows for example to highlight a region of interest or draw states in the background of plots.
There are a few points I would like to highlight that would need some feedback: