Make the drawing logic clearer in `Button` #64351

Rindbee · 2022-08-13T08:03:36Z

Make the drawing logic clearer.

The drawing logic follows the calculation logic in Button::get_minimum_size. According to the order of stylebox, icon, and text, and follow properties such as alignment mode, to fill the display space of the button.

Add a Button::_set_h_separation_is_valid_when_no_text for Button derived classes (like OptionButton) that expects a h_separation between icon and theme icon even if the text is empty.

Fixes #59202.
Supersedes #59340.
Depends on #64218 (Negative values of h_separation will be treated as 0).

Fixes #80386.

Try to follow the rules in Alpha13, but except when alignment is CENTER. This patch will try to draw inside the yellow rectangle.

The following is a set of comparison pictures.

Alpha13	This patch + #64218

Ayush-singla27 · 2022-11-06T15:39:26Z

i would like to work on this issue but this PR is still open. Is there any particular reason why this PR is waiting for a review?

KoBeWi · 2022-11-20T20:48:13Z

I don't really like how this adds a special case for inheriting class, but I guess you could fix #68930 as well now.

Rindbee · 2022-11-21T12:37:54Z

The drawing of this PR is based on get_minimum_size(), when text is empty, CheckBox/CheckButton will ignore h_separation between the inner icon and icon, but OptionButton class will not ignore it. So OptionButton is a bit special.

KoBeWi · 2022-11-21T12:43:46Z

I understand why it needs to be done; it just breaks OOP this way and could be handled better. But it would make the code more complex, so not worth it for a single case.

Rindbee · 2022-11-21T12:52:23Z

If the behavior of classes with internal icons (CheckBox/CheckButton/OptionButton) is consistent, no matter whether h_separation is ignored or not, no special handling is required.

YuriSizov · 2023-02-06T18:19:41Z

If you need a hack like that, then it's better to add an unexposed flag and set it only for the OptionButton. OOP would be preserved, and we'd be safe without weird circular dependencies.

Rindbee · 2023-02-07T01:06:28Z

If you need a hack like that, then it's better to add an unexposed flag and set it only for the OptionButton. OOP would be preserved, and we'd be safe without weird circular dependencies.

OK, use h_separation_is_valid_when_no_text to distinguish between OptionButton (true) and other Button derived classes (false).

YuriSizov · 2023-04-11T11:45:32Z

This needs a rebase (and the commit message could be a bit more detailed, the title of the PR itself is good).

The flag should not be a part of the constructor, it can just have a setter method that we call from the OptionButton constructor. That would be cleaner.

Also does it address #68930 or not? It probably should be addressed in this PR, as suggested.

Rindbee · 2023-04-12T00:48:42Z

Also does it address #68930 or not? It probably should be addressed in this PR, as suggested.

#68930 is mainly caused by enabling fit_to_longest_item to cache size data, and switching expand_icon does not update the cache data. It's not a drawing problem, but a refreshing problem of cached data. Fixed in #75958.

~~Maybe fit_to_longest_item can be made a member variable of Button, but only exposed in OptionButton, which can solve the problem of refreshing the cache when switching expand_icon. 🤔~~

KoBeWi · 2023-05-02T13:03:10Z

Why is there so much empty space between arrow icon and text?

KoBeWi

Aside from the weird spacing, seems correct.

YuriSizov · 2024-01-15T10:22:33Z

I still need to review this because before I did extensive testing and found issues, and since then there was a significant change in the implementation so I need to do it all over again.

YuriSizov

I didn't check all of the implementation, but one bit did stand out to me. And there are regressions in the behavior, which may or may not be related and need to be looked into (see below).

Overall, there are clear improvements in the behavior. Here's a comparison of before and after, which demonstrates both the linked issue being fixed and some inconsistencies being addressed:

Before

godot windows editor dev x86_64_2024-01-15_16-05-08

After

godot windows editor dev x86_64_2024-01-15_16-06-55

There are still some issues which are not addressed by this PR. See the top row of the right side of either image where the icon alignment varies. When the icon is center-aligned the text clipping stops to work. You can see that no matter if you set trimming or ellipses, it just stays in the "trim nothing" mode.

godot.windows.editor.dev.x86_64_2024-01-15_15-38-55.mp4

One new problem the aforementioned regression, is visible with the top-right example. For some reason, if you set the button to "trim nothing", the icon shrinks beyond the size of any other mode, and vertical positioning changes as a result. This was not happening before this patch.

godot.windows.editor.dev.x86_64_2024-01-15_16-10-36.mp4

You can also see that text alignment works funny, this may be related to the bit that I've highlighted in the code review. Left and center alignment are exactly the same, but right alignment is shifted to the left, so it's more to the left than either left or center alignment.

scene/gui/button.h

scene/gui/button.cpp

Rindbee · 2024-01-16T04:56:02Z

There are still some issues which are not addressed by this PR. See the top row of the right side of either image where the icon alignment varies. When the icon is center-aligned the text clipping stops to work. You can see that no matter if you set trimming or ellipses, it just stays in the "trim nothing" mode.

I'm not sure if the button's text is "Some very long te" or "Some very long text", if it's the former, the width is enough for the text to display, so no change is expected.

One new problem the aforementioned regression, is visible with the top-right example. For some reason, if you set the button to "trim nothing", the icon shrinks beyond the size of any other mode, and vertical positioning changes as a result. This was not happening before this patch.

I can't determine the reason. Maybe the width is larger than the minimum width? It is also possible that the cache has not been refreshed.

0.mp4

You can also see that text alignment works funny, this may be related to the bit that I've highlighted in the code review. Left and center alignment are exactly the same, but right alignment is shifted to the left, so it's more to the left than either left or center alignment.

Forgot to take into account the case of h_separation due to width (icon or text_buf) being 0.

text_buf->set_width() only affects the result of text_buf->get_size() when overrun_behavior != TextServer::OVERRUN_NO_TRIMMING.

The text_buf is drawn at the red point, but the display is offset, which seems to be a problem in TextParagraph::draw().

YuriSizov · 2024-01-17T10:53:22Z

I'm not sure if the button's text is "Some very long te" or "Some very long text", if it's the former, the width is enough for the text to display, so no change is expected.

It's full text. The issue is that when the icon is vertically aligned to the top, the amount of space available for the text doesn't change. However, with icon's left and right alignment we still subtract icon's size out of the space allocated for the text, but we don't do that with the center alignment. There may be some logical reason for this, but it feels super unintuitive.

That aside, there is still more text in there but it doesn't get trimmed or ellipse'd correctly. But in any case, as I mentioned, that's a pre-existing problem. We don't have to fix it here now. We can move forward with just a subset of issues fixed, as there are still more problems with button icons not tackled by this PR.

Rindbee · 2024-01-17T14:15:01Z

I noticed that in "Trim Nothing" mode the text always shows up as "Some very long te", if it doesn't show up completely there might be a cache issue in OptionButton.

Like toggling expand_icon, toggling text_overrun_behavior, clip_text and vertical_icon_alignment may affect the minimum size.

0.mp4

Rindbee · 2024-01-18T06:03:05Z

scene/gui/button.cpp


-			int text_width = MAX(1, is_clipped ? MIN(text_clip, text_buf->get_size().x) : text_buf->get_size().x);
+				float text_buf_width = MAX(1.0f, drawable_size_remained.width); // The space's width filled by the text_buf.
+				text_buf->set_width(text_buf_width);


Due to the conditions added in #80402, the calculated drawable_size_remained.width is less affected by text_buf->get_size(). That is to say, the calculated results are more reliable.

If I understand text_buf->set_width() correctly, doing this will align it horizontally correctly in the remaining space.

Visible characters will be offset in the text‘s reserve space by TextParagraph.

So we can later calculate the offset to this space from the left, as the required values are all determined.

scene/gui/button.cpp

YuriSizov

So one issue was actually my mistake in the test scene, I did actually have the text cut off at te for some reason. That still makes me wonder if the alignment works as expected by the user.

What I mean here is that a top-left-aligned icon takes horizontal space from the text, same with top-right, or bottom-left, or bottom-right. Whereas top-center and bottom-center don't do that. I think that when the icon is to the top or to the bottom, it shouldn't take horizontal space from the text. I don't find this useful.

But that's a new discussion. Let's go ahead with this rework for now, so we can finally put it to rest, and do any new fixes in follow-ups.

The drawing logic follows the calculation logic in `Button::get_minimum_size`. According to the order of `stylebox`, `icon`, and `text`, and follow properties such as alignment mode, to fill the display space of the button. Add a `Button::_set_h_separation_is_valid_when_no_text` for Button derived classes (like `OptionButton`) that expects a `h_separation` between `icon` and theme icon even if the `text` is empty.

Rindbee · 2024-01-18T15:18:13Z

What I mean here is that a top-left-aligned icon takes horizontal space from the text, same with top-right, or bottom-left, or bottom-right. Whereas top-center and bottom-center don't do that. I think that when the icon is to the top or to the bottom, it shouldn't take horizontal space from the text. I don't find this useful.

Like this:

When vertical_icon_alignment is center, the behavior is consistent with the previous one.
When vertical_icon_alignment is not center, they behave like two lines of text/icon.

2.mp4

YuriSizov · 2024-01-18T15:21:50Z

Like this:

Yes, this looks perfect!

Rindbee · 2024-01-18T15:25:18Z

Does Button::get_minimum_size() need to be modified? I'm not sure how to calculate the new rules, such as h_separation and so on.

YuriSizov · 2024-01-18T15:35:29Z

Does Button::get_minimum_size() need to be modified? I'm not sure how to calculate the new rules, such as h_separation and so on.

get_minimum_size_for_text_and_icon() doesn't appear to be accounting for internal margins at all. I would say this is a material for another PR. Reviewing this one over and over again gets very complicated.

If there is anything left to fix here, it's my comment above about logic for RIGHT missing. The rest let's address in a new PR after this one is merged.

YuriSizov · 2024-01-18T18:30:37Z

Thanks a lot!

Rindbee requested a review from a team as a code owner August 13, 2022 08:03

Rindbee mentioned this pull request Aug 13, 2022

Fix the expand icon calculation logic to the function Button::get_minimum_size #59340

Closed

Rindbee force-pushed the fix-button-draw branch 2 times, most recently from a47d9ed to 5711926 Compare August 13, 2022 09:38

YuriSizov added bug enhancement topic:gui labels Aug 13, 2022

YuriSizov added this to the 4.0 milestone Aug 13, 2022

Rindbee mentioned this pull request Aug 13, 2022

Fix case where h_separation might not work in Button #64218

Merged

Rindbee force-pushed the fix-button-draw branch 3 times, most recently from 562d7f7 to 63696f2 Compare September 16, 2022 13:21

Rindbee force-pushed the fix-button-draw branch from 63696f2 to c0be05c Compare November 20, 2022 09:19

Rindbee force-pushed the fix-button-draw branch from c0be05c to 38af5ab Compare February 7, 2023 01:01

akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023

Rindbee force-pushed the fix-button-draw branch from 38af5ab to 6314ad9 Compare April 12, 2023 00:37

This was referenced Apr 12, 2023

OptionButton's minimum size does not update after changing expand_icon #68930

Closed

Update size or size cache when toggling expand_icon in Button #75958

Merged

KoBeWi approved these changes May 2, 2023

View reviewed changes

YuriSizov suggested changes Jan 15, 2024

View reviewed changes

scene/gui/button.h Show resolved Hide resolved

scene/gui/button.cpp Outdated Show resolved Hide resolved

scene/gui/button.cpp Outdated Show resolved Hide resolved

Rindbee commented Jan 16, 2024

View reviewed changes

scene/gui/button.cpp Outdated Show resolved Hide resolved

Rindbee force-pushed the fix-button-draw branch 3 times, most recently from 2538295 to 26ca4c6 Compare January 17, 2024 02:45

Rindbee force-pushed the fix-button-draw branch from 26ca4c6 to 2b618d0 Compare January 17, 2024 13:55

Rindbee force-pushed the fix-button-draw branch 3 times, most recently from e6dba72 to 897cc8b Compare January 18, 2024 05:14

Rindbee commented Jan 18, 2024

View reviewed changes

Rindbee mentioned this pull request Jan 18, 2024

Button text "Quit" is truncated to "Qui" under certain conditions #81624

Closed

YuriSizov reviewed Jan 18, 2024

View reviewed changes

scene/gui/button.cpp Outdated Show resolved Hide resolved

YuriSizov reviewed Jan 18, 2024

View reviewed changes

scene/gui/button.cpp Outdated Show resolved Hide resolved

YuriSizov approved these changes Jan 18, 2024

View reviewed changes

Rindbee force-pushed the fix-button-draw branch from 897cc8b to f29a7d3 Compare January 18, 2024 15:02

YuriSizov merged commit bc88d77 into godotengine:master Jan 18, 2024
16 checks passed

Rindbee deleted the fix-button-draw branch January 18, 2024 23:38

KoBeWi mentioned this pull request Jan 22, 2024

Editor checkboxes have too little separation between text and icon #87473

Closed

Rindbee mentioned this pull request Jan 22, 2024

Fix forgetting h_separation when internal elements exist #87474

Merged

Maran23 mentioned this pull request May 10, 2024

Main editor toolbar icons are blurry #88165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the drawing logic clearer in `Button` #64351

Make the drawing logic clearer in `Button` #64351

Rindbee commented Aug 13, 2022 •

edited

Loading

Ayush-singla27 commented Nov 6, 2022

KoBeWi commented Nov 20, 2022

Rindbee commented Nov 21, 2022

KoBeWi commented Nov 21, 2022

Rindbee commented Nov 21, 2022

YuriSizov commented Feb 6, 2023

Rindbee commented Feb 7, 2023

YuriSizov commented Apr 11, 2023

Rindbee commented Apr 12, 2023 •

edited

Loading

KoBeWi commented May 2, 2023

KoBeWi left a comment

YuriSizov commented Jan 15, 2024

YuriSizov left a comment •

edited

Loading

Rindbee commented Jan 16, 2024 •

edited

Loading

YuriSizov commented Jan 17, 2024

Rindbee commented Jan 17, 2024

Rindbee Jan 18, 2024 •

edited

Loading

Rindbee Jan 18, 2024

YuriSizov left a comment

Rindbee commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

Rindbee commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

Make the drawing logic clearer in Button #64351

Make the drawing logic clearer in Button #64351

Conversation

Rindbee commented Aug 13, 2022 • edited Loading

Ayush-singla27 commented Nov 6, 2022

KoBeWi commented Nov 20, 2022

Rindbee commented Nov 21, 2022

KoBeWi commented Nov 21, 2022

Rindbee commented Nov 21, 2022

YuriSizov commented Feb 6, 2023

Rindbee commented Feb 7, 2023

YuriSizov commented Apr 11, 2023

Rindbee commented Apr 12, 2023 • edited Loading

KoBeWi commented May 2, 2023

KoBeWi left a comment

Choose a reason for hiding this comment

YuriSizov commented Jan 15, 2024

YuriSizov left a comment • edited Loading

Choose a reason for hiding this comment

Before

After

Rindbee commented Jan 16, 2024 • edited Loading

YuriSizov commented Jan 17, 2024

Rindbee commented Jan 17, 2024

Rindbee Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Rindbee Jan 18, 2024

Choose a reason for hiding this comment

YuriSizov left a comment

Choose a reason for hiding this comment

Rindbee commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

Rindbee commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

YuriSizov commented Jan 18, 2024

Make the drawing logic clearer in `Button` #64351

Make the drawing logic clearer in `Button` #64351

Rindbee commented Aug 13, 2022 •

edited

Loading

Rindbee commented Apr 12, 2023 •

edited

Loading

YuriSizov left a comment •

edited

Loading

Rindbee commented Jan 16, 2024 •

edited

Loading

Rindbee Jan 18, 2024 •

edited

Loading