WIP: replace cassowary with taffy (Proof-of-Concept) #385
Conversation
src/layout.rs
Outdated
| // disabling rounding, because otherwise the following tests fail: | ||
| // widgets_table_columns_widths_can_use_mixed_constraints | ||
| // widgets_table_column_spacing_can_be_changed | ||
| // | ||
| // but disabling rounding makes the following tests fail: | ||
| // layout::tests::test_vertical_split_by_height | ||
| taffy.disable_rounding(); |
There was a problem hiding this comment.
this needs to be discussed, opening a conversation
some extra notes:
test layout::tests::test_vertical_split_by_height fails with reason:
thread 'layout::tests::test_vertical_split_by_height' panicked at 'assertion failed: `(left == right)`
left: `10`,
right: `9`', src/layout.rs:711:9where as the other 2 tests fail because of
thread 'widgets_table_columns_widths_can_use_mixed_constraints' panicked at 'Buffers are not equal
Expected:
"┌────────────────────────────┐"
"│Hea Head2 He │"
"│ │"
"│Row Row12 Ro │"
"│Row Row22 Ro │"
"│Row Row32 Ro │"
"│Row Row42 Ro │"
"│ │"
"│ │"
"└────────────────────────────┘"
Got:
"┌────────────────────────────┐"
"│Hea Head2 Hea│"
"│ │"
"│Row Row12 Row│"
"│Row Row22 Row│"
"│Row Row32 Row│"
"│Row Row42 Row│"
"│ │"
"│ │"
"└────────────────────────────┘"
thread 'widgets_table_column_spacing_can_be_changed' panicked at 'Buffers are not equal
Expected:
"┌────────────────────────────┐"
"│Head1 Head2 Head│"
"│ │"
"│Row11 Row12 Row1│"
"│Row21 Row22 Row2│"
"│Row31 Row32 Row3│"
"│Row41 Row42 Row4│"
"│ │"
"│ │"
"└────────────────────────────┘"
Got:
"┌────────────────────────────┐"
"│Head1 Head2 Head3"
"│ │"
"│Row11 Row12 Row13"
"│Row21 Row22 Row23"
"│Row31 Row32 Row33"
"│Row41 Row42 Row43"
"│ │"
"│ │"
"└────────────────────────────┘"
There was a problem hiding this comment.
For these tests, I'd generally approach this by adding simpler unit tests that cover the same functionality on the current implementation first to work out exactly which calculation is failing.
That assert in test_vertical_split_by_height should just assert the actual Rects instead of properties of the rects - doing so would give the information necessary to debug what's different. It's probably a demonstration of one of things I believe is actually a bug not a feature - 10 rows split into [10%, max(5), min(1)] should lead to [1,5,4], not [1,5,3] (or whatever the actual result is).
There was a problem hiding this comment.
Oh - and I think probably keeping rounding enabled seems like the intuitively right choice (but I'm not sure I understand the tradeoff well enough).
There was a problem hiding this comment.
That assert in test_vertical_split_by_height should just assert the actual Rects instead of properties of the rects - doing so would give the information necessary to debug what's different.
i know why test_vertical_split_by_height is failing - it is because rounding is disabled (read the comment again), because 10% of 9 is <1.0, which because rounding is disabled, gets trimmed to 0
i had disabled rounding because of the other 2 mentioned tests, and i wanted feedback on that first before deciding on a path, because it seems to me that widgets_table_columns_widths_can_use_mixed_constraints at least is now actually outputting the correct things (no extra gap at the end?)
as for why widgets_table_column_spacing_can_be_changed is failing, i currently have no clue, but it is likely related to the previous test failing
also i have not encountered tests that test specific lengths that something should give and so some thing may be missed (at least not for mixed expandable and non-expandable), example test_vertical_split_by_height currently tests 2 things: 1. that all elements add up to the size specified, and 2. that the next elements are always y <= y, no specific tests on which the size the elements themself should be
There was a problem hiding this comment.
also i have not encountered tests that test specific lengths that something should give
Yeah, that's exactly the sort of thing I see as a problem (a problem I've spent many days dealing on in legacy software maintenance in $PREVIOUS_JOB). testing a second order thing rather than a first order (e.g. asserting collection.is_empty() = true rather than checking collection = []) makes broken tests annoying to fix.
There was a problem hiding this comment.
Oh - and I think probably keeping rounding enabled seems like the intuitively right choice
i now had a look at the original cassowary code, it does not use rounding, it just trims of the decimal point:
Line 387 in 37fa6ab
(context:
value is f64)
some example value from running examples/layout.rs with some extra logging where the width x height is 116x57:
[2023-08-08T12:40:46.727+02:00 WARN ratatui::layout]: res_f64 [
ResultingRects {
x: 0.0,
y: 0.0,
width: 116.0,
height: 4.0,
},
ResultingRects {
x: 0.0,
y: 4.0,
width: 116.0,
height: 28.5,
},
ResultingRects {
x: 0.0,
y: 32.5,
width: 116.0,
height: 24.5,
},
]
[2023-08-08T12:40:46.728+02:00 WARN ratatui::layout]: result [
Rect {
x: 0,
y: 0,
width: 116,
height: 4,
},
Rect {
x: 0,
y: 4,
width: 116,
height: 28,
},
Rect {
x: 0,
y: 32,
width: 116,
height: 24,
},
]The same test run against current PR (rounding disabled, using trimming), with similar logging
[2023-08-08T12:45:56.635+02:00 WARN ratatui::layout]: taffy f32 [
ResultingRects {
x: 0.0,
y: 0.0,
width: 116.0,
height: 4.0,
},
ResultingRects {
x: 0.0,
y: 4.0,
width: 116.0,
height: 17.458824,
},
ResultingRects {
x: 0.0,
y: 21.458824,
width: 116.0,
height: 35.541176,
},
]
[2023-08-08T12:45:56.635+02:00 WARN ratatui::layout]: results [
Rect {
x: 0,
y: 0,
width: 116,
height: 4,
},
Rect {
x: 0,
y: 4,
width: 116,
height: 17,
},
Rect {
x: 0,
y: 21,
width: 116,
height: 35,
},
]There was a problem hiding this comment.
because 10% of 9 is <1.0
The layout it's splitting is 10 high, not 9, so 10% should always render as 1.0. With rounding disabled it was rendering the heights as 1,4,4 (for perc(10), max(5), min(1)), but 1,5,4 is a better result.
widgets_table_columns_widths_can_use_mixed_constraints definitely is rendered better (IMO) with taffy than cassowary as taffy uses the full width rather than truncating one character short.
widgets_table_columns_widths_can_use_ratio_constraints looks like it's just a matter of choosing which column gets the extra space (28 cols split into 3 ratio(1,3) constraints - the extra space makes more sense going into the second column rather than the third IMO
widgets_table_column_spacing_can_be_changed fails because the layout is trying to fit lengths 5+7+5+7+5=29 into 28 columns. Guessing the flex algorithm picks the box to shrink based on which one falls furthest away from an integer rather than just truncating the last one (i.e. the unrounded values are:
- (0)
- (5/29*28 = 4.82)
- ((5+7)/29*28 = 11.58)
- ((5+7+5)/29*28 = 16.41)
- ((5+7+5+7)/29*28 = 23.17)
- ((5+7+5+7+5)/29*28 = 28).
Rounded that's (0, 5, 12, 16, 23, 28) which makes widths of (5, 7, 4, 7, 5) - which is exactly the failure I see:
expected vs actual:
"│Head1 Head2 Head│"
"│Head1 Head Head3│"
So overall, I think that taffy definitely changes some edge cases, but the places it does do that are already broken behavior that have have test coverage that tests for the broken behavior (which is good)
hasezoey
changed the title
|
also note that current compile times have "ballooned" from 10sec (PR base) to 19s (this PR at da4b42c) running |
src/layout.rs
Outdated
| // disabling rounding, because otherwise the following tests fail: | ||
| // widgets_table_columns_widths_can_use_mixed_constraints | ||
| // widgets_table_column_spacing_can_be_changed | ||
| // | ||
| // but disabling rounding makes the following tests fail: | ||
| // layout::tests::test_vertical_split_by_height | ||
| taffy.disable_rounding(); |
There was a problem hiding this comment.
Oh - and I think probably keeping rounding enabled seems like the intuitively right choice (but I'm not sure I understand the tradeoff well enough).
src/layout.rs
Outdated
| // layout::tests::test_vertical_split_by_height | ||
| taffy.disable_rounding(); | ||
|
|
||
| for elt in &layout.constraints { |
There was a problem hiding this comment.
I think this could probably get down to something like:
let child_nodes = layout.constraints
.iter()
.map(|c| taffy.new_leaf((c, layout.direction).into())) // not sure what's idiomatic here
.collect_vec()There was a problem hiding this comment.
i dont think i can get it down to something like this, unless the .map closure is pretty long
There was a problem hiding this comment.
This compiles and runs some of the examples fine (and seems reasonably understandable even):
fn split(area: Rect, layout: &Layout) -> Rc<[Rect]> {
use taffy::prelude::*;
let inner = area.inner(&layout.margin);
let mut taffy = Taffy::with_capacity(layout.constraints.len());
// disabling rounding, because otherwise the following tests fail:
// widgets_table_columns_widths_can_use_mixed_constraints
// widgets_table_column_spacing_can_be_changed
//
// but disabling rounding makes the following tests fail:
// layout::tests::test_vertical_split_by_height
taffy.disable_rounding();
let children = layout
.constraints
.iter()
.map(|constraint| ConstraintAndDirection(*constraint, layout.direction).into())
.map(|style| taffy.new_leaf(style).unwrap())
.collect_vec();
let root_node = taffy
.new_with_children(
Style {
// setting flex_direction, because otherwise "Column" would be used for
// "Horizontal", which makes it basically render 0 length
flex_direction: match layout.direction {
Direction::Horizontal => FlexDirection::Row,
Direction::Vertical => FlexDirection::Column,
},
// root node needs to take up full space of available_space later, otherwise the
// child elements are all length 0
size: Size {
width: percent(1.0),
height: percent(1.0),
},
..Default::default()
},
&children,
)
.unwrap();
taffy
.compute_layout(
root_node,
Size {
width: AvailableSpace::Definite(inner.width as f32),
height: AvailableSpace::Definite(inner.height as f32),
},
)
.unwrap();
children
.iter()
.map(|key| taffy.layout(*key).unwrap())
.map(|computed| crate::layout::Rect {
width: computed.size.width as u16,
height: computed.size.height as u16,
x: computed.location.x as u16 + inner.x,
y: computed.location.y as u16 + inner.y,
})
.collect::<Rc<[crate::layout::Rect]>>()
}
struct ConstraintAndDirection(Constraint, Direction);
impl From<ConstraintAndDirection> for Style {
fn from(c: ConstraintAndDirection) -> Style {
let main_axis = match c.0 {
Constraint::Percentage(p) => percent(p as f32 / 100.0),
Constraint::Ratio(a, b) => percent(a as f32 / b as f32),
Constraint::Length(length) => points(length as f32),
Constraint::Max(max) => points(max as f32),
Constraint::Min(min) => points(min as f32),
};
let (width, height) = match c.1 {
Direction::Horizontal => (main_axis, auto()),
Direction::Vertical => (auto(), main_axis),
};
match c.0 {
Constraint::Percentage(_) | Constraint::Ratio(_, _) | Constraint::Length(_) => Style {
size: Size { width, height },
..Default::default()
},
Constraint::Max(_) => Style {
max_size: Size { width, height },
..Default::default()
},
Constraint::Min(_) => Style {
min_size: Size { width, height },
..Default::default()
},
}
}
}The unit test fails in a different way however - so it's got bugs.
There was a problem hiding this comment.
Adding
Constraint::Max(_) => Style {
size: auto(),
max_size: Size { width, height },
flex_grow: 1.0,
..Default::default()
},
Constraint::Min(_) => Style {
size: auto(),
min_size: Size { width, height },
flex_grow: 1.0,
..Default::default()
},Gets most of the way there - the failing tests now pretty much are debatable edge cases. Especially if rounding is turned back on.
|
|
i did some extra testing, and it seems this PR currently covers most of the direct transition from cassowary to taffy, but it is now unclear what each thing may or may not do, so it not everything is probably covered also taffy may "overallocate" (when using the same would also apply when re-enabling rounding, where it would maybe round to a upper number on some element, and so result in them being (even though slightly) out-of-bounds how should this be handled?
|
Perhaps start with a minimal unit (or integ if unit can't work) test that shows the behavior against cassowary, followed by an assessment of whether that behavior is "correct" or not, then run the test against the taffy approach to show how it behaves differently. I think we should ensure that layout never allocates outside of the rect and avoids panics as much as possible. Better to allocate 0 than fail. For a real implementation of this, what if we keep the old implementation around and have a secondary taffy implementation enabled with a a feature flag? Done right we could even enable both feature flags to compare them directly in tests. |
|
I was attempting to implement the Cassowary algorithm myself for another project. Here's some of my notes:
|
src/layout.rs
Outdated
| width: dim(elt_applied as f32), | ||
| height: auto(), | ||
| }, | ||
| flex_basis: dim(elt_applied), |
There was a problem hiding this comment.
What was the effect of this change rather than size?
There was a problem hiding this comment.
this is not in use anyone in the latest version of this PR, i had done this because it was smaller and equivalent to the previous version.
flex-basis is "size across the main axis", where the "main axis" is being defined via "flex_direction" (where column is height, and row (default) width), so it just made the code more clean instead of of having to have a Size struct with a auto() property
also see this stackoverflow answer
Benchmarks on taffy give 0.3ms for 1000 nodes on a 2021 MacBook Pro with M1 Pro. (It would be good to see cassowary numbers for something similar). The main goal of swapping in taffy here is less about performance and more about reducing the complexity, and making it easy to fix some of the layout rendering issues. They'd be very difficult to fix with the existing approach, but much more reasonable with the taffy based code. Compare: 146 lines of complex nested code (7 for loops, 6 if statements, 9 match statements): Lines 274 to 420 in 37fa6ab 97 lines (excluding comments) of much simpler linear code (0 for loops, 0 if statements, 4 match statements) Lines 271 to 380 in 8cdfe2a As popular as cassowary is. This difference is undeniably compelling. |
|
(reply to this comment)
i dont know what the maintainers think, but this package until now has been relying on rust-only libraries, so i dont see this being implemented
taffy also supports this, via the
it also support css grid
i personally find it easier to write taffy constraints instead of cassowary, but i have not been too deep into the documentation of either to fully understand it.
taffy may also support this in the future, see DioxusLabs/taffy#460 the main reason for me to switch over to taffy is that it has less "undefined behavior" or "weird behavior" and is personally for me easier to write and extend. weird behavior like if there is more space available and there are multiple expandable constraints, only one gets chosen (i dont know if this could be changed in cassowary) and generally allowing more options to be available (also with overflow, if this is something that is useful to a case, like a table with more columns than could be displayed at once) |
|
Reading through the MDN docs for both flex and grid, I noticed the comment about Content out or layout in?, which suggests that it might be worth using grids instead of flex for replacing the cassowary layout. The other neat thing I saw was that taffy supports defining your own layout tree type that implements a trait. This would be useful if we wanted to do retained layout. I.e. we could define an app's layout once, call taffy::layout() on it whenever it's dirty (new item / resized window), and pass the generated Rects into the app's rendering method. This might be useful as an interim approach regardless of where this lands. |
|
In #393 I updated the layout example to show a bunch of combinations of how constraints interact. I'm not sure whether we want to merge that PR, but this be useful as a good visual check for equivalency (and be an easy way to describe any issues that we hit with taffy) |
from what i can tell (and i am not experienced in flexbox, grid or cassowary), is that flexbox is one-dimensional by default and grid is two-dimensional by default, but the current ratatui layout is one-dimensional, so i think flex is the better option here
depending on what you mean with that, the though that would mean we would have a global (taffy / cassowary?) instance for all widgets, or we would need to add this to all existing states (like |
2D is just 1D with a dimension that you can ignore ;) The part I found that aligned better with grid vs flex is that the current ratatui approach define how much space to give each layout Rect, rather than defining the size of things that are stored and then allocating space to fit. I think we'll definitely want to embrace both approaches in Ratatui apps, and I wonder whether expanding the existing simplified constraints to two dimensions (e.g. Vec<Vec>) would be an easy intermediate step toward simplifying some common layout tasks - like in recent layout or colors example, where there's a lot of repetition).
I was referring to: https://docs.rs/taffy/0.3.12/taffy/tree/trait.LayoutTree.html. It's definitely out of scope for this PR. |
i know what you meant with |
I can't recall the exact point I was making. Sorry Re: screenshots: VHS sometimes doesn't get background colors right - there's a bug (likely in one of the dependencies) that I haven't quite gotten to the bottom of, but it's something to do with background color of spaces that are not terminated by whitespace. charmbracelet/vhs#344 I'm really glad you ran the example against this. It's definitely surprising that this is so different when the tests mostly succeed. Looks like the taffy version is much more likely to share the space rather than truncating and prioritizes constraints differently. It's treating Min as a hard minimum and not respecting the space available. I wonder if making the compute_layout take a min_size(), and giving the root node a size in points would fix this? |
i cant quite tell what is wrong with the vhs output, because it looks the same to me on my terminal (ignoring that the color looks different because of theme) EDIT: is it maybe the ending gaps? |
(It didn't)
Everywhere in green that doesn't have a "·" in VHS is blank in your term screenshot (but VHS bleeds the background). Same with the extra couple of spaces in the red bleed on line 6 of Min/Ratio. Turn off the borders and it would be worse (the borders serve to reset whatever the bug is). Your terminal is the more correct rendering for this. |
|
after the refactors of the cassowary usage, is this still something that is wanted to be looked at, or is cassowary now going to stay? |
|
I think this is worth it as an idea. I wanted to spend some time understanding the cassowary implementation better before forming an opinion on replacing it lest someone invoke Chesterton's fence. Also - it's well worth putting in place more tests that help flesh out the edge cases, that we have currently and working out where the pain points are. These help working out just how much we should expose of taffy. They also serve as regression tests to ensure we don't break things drastically when and if we change. I feel that we're pretty close to using cassowary to the max, whereas the same functionality would be using taffy fairly minimally, which means there's a lot of scope for future features. In particular, the possibility of sizing based on content rather than content sizing to container is a significant benefit. The two dimensional aspects are also pretty appealing. I'm not in a hurry to merge this. What I think would be good to do though would be to flesh out some examples using taffy, that would help gain some extra feedback on the more advanced features, and kickstart informed conversations. We need some high quality answers to "how can i make this layout" easily. Three things that would be definitely easier in taffy are:
|
|
Taffy maintainer here. Some thoughts:
|
|
I'm going to close this out for now as unplanned (we're cleaning up the backlog) - we can come back on this if we really want to pursue it. |
i dont really mind it, this was just a Proof-of-Concept PR, i didnt (and still dont) fully understand both algorithms and so wouldnt be able to properly implement it anyway. also the current cassowary implementation got a lot better since the creation of this PR |
|
Yup - we definitely appreciate that this led to better things elsewhere. The cassowary code went from a mess to something much more maintainable. |
re #374
This PR replaces
cassowary-rswithtaffy, this PR is a WIP because this implementation is basically structured to be a Proof-of-Concept / hacked-in without too much extra though aside from keeping the external API the same (and all tests passing)also contains some debugging helper tools (ie
logandflexi_logger), because printing to stdout / stderr while running is not a great experiencePS: i dont fully understand the
cassowaryalgorithm, and i also dont too much experience with (css) flex / gridcloses #23