Skip to content

Add Galley::intrinsic_size and use it in AtomLayout#7146

Merged
lucasmerlin merged 16 commits intomainfrom
lucas/atoms-preferred-size
Jul 9, 2025
Merged

Add Galley::intrinsic_size and use it in AtomLayout#7146
lucasmerlin merged 16 commits intomainfrom
lucas/atoms-preferred-size

Conversation

@lucasmerlin
Copy link
Collaborator

@lucasmerlin lucasmerlin marked this pull request as draft June 14, 2025 10:38
@github-actions
Copy link

Preview available at https://egui-pr-preview.github.io/pr/7146-lucasatoms-preferred-size
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin lucasmerlin force-pushed the lucas/atoms-preferred-size branch from ca8d3eb to 642c258 Compare June 16, 2025 09:09
@lucasmerlin lucasmerlin added feature New feature or request egui epaint labels Jun 16, 2025
@lucasmerlin lucasmerlin marked this pull request as ready for review June 16, 2025 09:14
@lucasmerlin lucasmerlin changed the title Correctly calculate preferred/intrinsic size in AtomLayout Add Galley::desired_size and use it in AtomLayout Jun 16, 2025
@lucasmerlin lucasmerlin force-pushed the lucas/atoms-preferred-size branch from 642c258 to f44b50a Compare June 16, 2025 09:16
@lucasmerlin lucasmerlin added this to the 0.32.0 milestone Jun 18, 2025
@lucasmerlin
Copy link
Collaborator Author

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.
Another benefit is that this will also work for truncated text.

@lucasmerlin
Copy link
Collaborator Author

I ran the demo and button benchmarks, the change was "within noise threshold" as criterion likes to say.

Comment on lines +204 to +206
if idx == 0 {
desired_size.y += point_scale.round_to_pixel(paragraph.empty_paragraph_height);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only if idx == 0 ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to (also) use first_row_min_height here?

Copy link
Collaborator Author

@lucasmerlin lucasmerlin Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lucasmerlin
Copy link
Collaborator Author

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.

@lucasmerlin lucasmerlin changed the title Add Galley::desired_size and use it in AtomLayout Add Galley::intrinsic_size and use it in AtomLayout Jul 8, 2025
@lucasmerlin
Copy link
Collaborator Author

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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a test for this in

fn jobs() -> Vec<LayoutJob> {

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw your comment in #7146 (comment) - let's do that in a follow-up PR then :)

Comment on lines +207 to +212
if paragraph.glyphs.is_empty() {
if idx == 0 {
intrinsic_size.y += point_scale.round_to_pixel(paragraph.empty_paragraph_height);
}
continue;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@lucasmerlin lucasmerlin merged commit 508c60b into main Jul 9, 2025
47 checks passed
@lucasmerlin lucasmerlin deleted the lucas/atoms-preferred-size branch July 9, 2025 06:19
lucasmerlin added a commit that referenced this pull request Jul 9, 2025
* Follow up to #7146 

Previously when galleys were splitted, each exept the last had an extra
empty row that had to be removed when they were concated. This changes
it to remove the `\n` from the layout jobs when splitting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

egui epaint feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants