deps: bump ratatui to 0.26 #1406
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
+ Coverage 54.64% 55.90% +1.25%
==========================================
Files 101 102 +1
Lines 17764 18359 +595
==========================================
+ Hits 9708 10263 +555
- Misses 8056 8096 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Yeah, we fixed the gap problem in a previous release (I forget if it was 0.25 or 0.24). It was due fundamentally to the layout algorithm choosing to focus on rounding the floating point widths of the rects to integer amounts rather than the position of the start and end of the rects. This resulted in things like: a rect from x=9.4 width=0.4 being computed as a rect at x=~9, width=~0, rather than being a rect at x= |
| //! Vendored from <https://github.com/fdehau/tui-rs/blob/fafad6c96109610825aad89c4bba5253e01101ed/src/widgets/canvas/mod.rs>. | ||
| //! Vendored from <https://github.com/fdehau/tui-rs/blob/fafad6c96109610825aad89c4bba5253e01101ed/src/widgets/canvas/mod.rs> | ||
| //! and <https://github.com/ratatui-org/ratatui/blob/c8dd87918d44fff6d4c3c78e1fc821a3275db1ae/src/widgets/canvas.rs>. | ||
| //! Main difference is in the Braille rendering, which can now effectively be done in a single layer without the effects | ||
| //! of doing it all in a single layer via the normal tui-rs crate. This means you can do it all in a single pass, with | ||
| //! of doing it all in a single layer via the normal ratatui crate. This means you can do it all in a single pass, with | ||
| //! just one string alloc and no resets. |
There was a problem hiding this comment.
Is there something here that would be worth upstreaming into ratatui? I don't understand the comment about single pass - do you have more context about what problem this solves?
There was a problem hiding this comment.
Hmmm... honestly, that message is kinda confusing to me too, but I think I know what it's referring to. It's been a while (like 2022), but the relevant PRs were #918 and #937.
For context, at least from what I remember, I was flamegraphing the program out of curiosity and noticed a considerable portion of work was spent drawing points for the CPU widget - I store and draw a decent amount of points, and for the CPU widget, it draws braille lines for each thread, so it does more work compared to, say, the memory or network widgets where they only draw like one or two lines.
What I found back then, at least from flamegraphs (I don't know if it still holds now though!) is that this code in chart.rs/time_chart.rs:
for dataset in &self.datasets {
Canvas::default()
.background_color(self.style.bg.unwrap_or(Color::Reset))
.x_bounds(self.x_axis.bounds)
.y_bounds(self.y_axis.bounds)
.marker(dataset.marker)
.paint(|ctx| {
// Draw your points using ctx
})
.render(graph_area, buf);
}performed way worse than doing:
Canvas::default()
.background_color(self.style.bg.unwrap_or(Color::Reset))
.x_bounds(self.x_axis.bounds)
.y_bounds(self.y_axis.bounds)
.marker(dataset.marker)
.paint(|ctx| {
for dataset in &self.datasets {
// Draw your points using ctx
}
})
.render(graph_area, buf);That is, you move the for loop inside the paint closure.
I think I remember it was due to the multiple canvas/render calls, which at least back then was kinda expensive? I'm assuming that's what I meant back then by saying "single pass", in that you only do one canvas + render call, and you only ever really touch one "layer" as opposed to (at least IIRC) how stacking multiple canvases + renders would touch multiple layers, do multiple allocations, etc., which added up as you drew more and more lines, when I could get the same effect by just messing with one layer and drawing that one, final layer.
However I remember doing it this way broke some things at first in terms of colouring - hence #937, where I vendored in a bunch of tui-rs code to be able to override the implementation for drawing BrailleCell:
fn paint(&mut self, x: usize, y: usize, color: Color) {
let index = y / 4 * self.width as usize + x / 2;
if let Some(curr_color) = self.colors.get_mut(index) {
if *curr_color != color {
*curr_color = color;
if let Some(cell) = self.cells.get_mut(index) {
*cell = symbols::braille::BLANK;
*cell |= symbols::braille::DOTS[y % 4][x % 2];
}
} else if let Some(c) = self.cells.get_mut(index) {
*c |= symbols::braille::DOTS[y % 4][x % 2];
}
}
}There was a problem hiding this comment.
As for whether it's worth upstreaming - I don't know, I remember finding how this worked to be fine for my use case of just drawing a bunch of braille lines, but I'm not sure if drawing this way might break other use cases. I also don't know if nowadays if it's actually a problem for ratatui as-is, I haven't tested.
If there is interest in upstreaming it though, it's a pretty simple change, so there's that. Just gotta ignore all the extra code about interpolation with off-screen points (unless that's something else desired).
There was a problem hiding this comment.
For the colors, I suspect that the right approach is one that keeps other people's apps running well, but if there are measurable performance wins then that's cool.
Just gotta ignore all the extra code about interpolation with off-screen points (unless that's something else desired).
I think that might be nice to have too. I noticed that problem in Ratatui a while back, but haven't dug into it.
I haven't played much with the charts / canvas personally, so both items are a low priority for my own use cases. I'll cut an issue for this and let someone more motivated pick it up (if that's you, awesome, but we've also been getting a devs pick up random tasks marked as good first issue recently).
ClementTsang
force-pushed
the
bump_ratatui_0.26
branch
from
bbb5b3e to
d507051
Compare
Description
A description of the change, what it does, and why it was made. If relevant (such as any change that modifies the UI), please provide screenshots of the changes:
Bumps to 0.26. Followed the guide for breaking changes from 0.26: https://github.com/ratatui-org/ratatui/blob/main/BREAKING-CHANGES.md#v0260
Changes:
Textand some column spacing code for drawing tables.TimeChartand somecanvas.rscode.Another thing to check in a separate PR is to see if I can skip how I currently manually calculate widths for spacing (see #916 and #896 for the original problem).
Issue
If applicable, what issue does this address?
Closes: #
Testing
If relevant, please state how this was tested. All changes must be tested to work:
If this is a code change, please also indicate which platforms were tested:
Note: test this on all 3 main platforms! It should be fine if it works on Linux but eh, might as well be sure.
Checklist
If relevant, ensure the following have been met:
cargo fmt)README.md, help menu, doc pages, etc.)