Add Galley::intrinsic_size and use it in AtomLayout - #7146
Conversation
|
Preview available at https://egui-pr-preview.github.io/pr/7146-lucasatoms-preferred-size |
ca8d3eb to
642c258
Compare
Galley::desired_size and use it in AtomLayout
642c258 to
f44b50a
Compare
|
I've changed the approach and now calculate the desired_size as the galley is laid out. This should improve performance (since the calculation is now cached) and makes it more accurate by having access to the Paragraphs. |
|
I ran the demo and button benchmarks, the change was "within noise threshold" as criterion likes to say. |
| if idx == 0 { | ||
| desired_size.y += point_scale.round_to_pixel(paragraph.empty_paragraph_height); | ||
| } |
There was a problem hiding this comment.
Did you mean to (also) use first_row_min_height here?
There was a problem hiding this comment.
I did that first but got wrong results. This matches what happens in rows_from_paragraphs.
In this if
if paragraph.glyphs.is_empty() {
rows.push(PlacedRow {
pos: Pos2::ZERO,
row: Arc::new(Row {
section_index_at_start: paragraph.section_index_at_start,
glyphs: vec![],
visuals: Default::default(),
size: vec2(0.0, paragraph.empty_paragraph_height),
ends_with_newline: !is_last_paragraph,
}),
});
} else {the empty row is always placed at (0, 0), so it only has an effect on the total rect if it's the first and only row.
This seems like a bug, I'll try to fix it in a separate PR. first_row_min_height is also not used in the empty case (which is probably also a bug).
If I remove the if, the test_split_paragraph test fails since it causes a empty Paragraph to be added at the end of the first split Galley.
…h response.intrinsic_size
|
I renamed the desired and preferred size names I had in AtomLayout and in this PR to intrinsic size since I think this is the best name (we use desired size a lot in other places and preferred size is very similar to desired size) and for consistency with response.intrinsic_size. |
Galley::desired_size and use it in AtomLayoutGalley::intrinsic_size and use it in AtomLayout
|
Also made a small test ui to show that this works: Screen.Recording.2025-07-08.at.17.49.51.mov egui::Window::new("Test").show(ctx, |ui| {
ui.set_min_size(ui.available_size());
for wrapping in [
// TextWrapMode::Extend,
TextWrapMode::Wrap,
TextWrapMode::Truncate,
] {
ui.style_mut().wrap_mode = Some(wrapping);
let response = ui.button(format!(
"Hello world this is a long text that should {wrapping:?}"
));
ui.ctx().debug_painter().debug_rect(
Rect::from_min_size(
response.rect.min,
response.intrinsic_size.unwrap_or_default(),
),
Color32::RED,
"",
);
}
}); |
| .max_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal)) | ||
| .unwrap_or(paragraph.empty_paragraph_height); | ||
| if idx == 0 { | ||
| height = f32::max(height, job.first_row_min_height); |
There was a problem hiding this comment.
We should add a test for this in
egui/crates/epaint/src/text/fonts.rs
Line 1079 in f46926a
There was a problem hiding this comment.
I saw your comment in #7146 (comment) - let's do that in a follow-up PR then :)
| if paragraph.glyphs.is_empty() { | ||
| if idx == 0 { | ||
| intrinsic_size.y += point_scale.round_to_pixel(paragraph.empty_paragraph_height); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Is this if-statement even needed? Seems like the logic below should be robust to handling this regardless? In fact, more robust, since the code below takes first_row_min_height into account
There was a problem hiding this comment.
Yes, otherwise the test_split_paragraphs test fails. (I explained why in this comment in case you didn't see it #7146 (comment))
I'll try to fix this in a separate PR.
cross_intrinsic_sizebookkeeping in Placer #5762