Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uilist line padding accounts for number of lines in text #76356

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

RenechCDDA
Copy link
Member

@RenechCDDA RenechCDDA commented Sep 11, 2024

Summary

Bugfixes "uilist line padding accounts for number of lines in text"

Purpose of change

Describe the solution

Padding is applied between each line ( s.ItemSpacing.y * expected_num_lines ) and then also two at the top and bottom of the text: ( s.ItemSpacing.y * 2.0 ). Previously the calculated padding area only account for the top and bottom, meaning that each additional line of text ate up more and more vertical space that the window wasn't expecting

I'm not sure why ImGui::CalcTextSize() isn't already returning this properly

Describe alternatives you've considered

There is probably somewhere more fundamental to apply this, like the static calc_size

title_size and desc_size are both calculated the previous way, and technically those likely do have the same error unless changes are made to them/calc_size

I am sure there is a better way to do this, I just don't know what it is!

Testing

See my beautiful images at the comment I made in the linked issue

Additional context

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Sep 11, 2024
@RenechCDDA RenechCDDA marked this pull request as ready for review September 11, 2024 17:08
@RenechCDDA
Copy link
Member Author

I guess we can just mark this ready and see if any objections come up. It shouldn't make the UI any worse!

@db48x
Copy link
Contributor

db48x commented Sep 11, 2024

I‌ suppose we can go ahead and do this. I had been intending to investigate to find out where the extra spacing comes from, since it indicates to me that we are doing something wrong, but I‌ guess that working around it won’t make that any more difficult.

@CLIDragon
Copy link
Contributor

I‌ suppose we can go ahead and do this. I had been intending to investigate to find out where the extra spacing comes from, since it indicates to me that we are doing something wrong, but I‌ guess that working around it won’t make that any more difficult.

Doesn't the extra spacing come from ItemSpacing?

And (separately), to handle this properly (though it is definitely scope creep), I suspect that ItemInnerSpacing, CellPadding, and possibly FramePadding will need to be handled (I think WindowPadding is already handled)

@db48x
Copy link
Contributor

db48x commented Sep 12, 2024

Sure, it apparently comes from ItemSpacing but if it’s supposed to be there why doesn’t CalcTextSize include it?

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 12, 2024
@CLIDragon
Copy link
Contributor

Sure, it apparently comes from ItemSpacing but if it’s supposed to be there why doesn’t CalcTextSize include it?

That's easy, CalcTextSize only calculates the size of the text. See ocornut/imgui/issues/3714

@db48x
Copy link
Contributor

db48x commented Sep 13, 2024

The answer is not so simple. We were calculating the height as he recommends (“CalcTextSize(text).y + style.FramePadding.y * 2.0f for framed items”), but found that we need an additional style.ItemSpacing.y * 2.0f per line of text. If that’s intentional then why doesn’t CalcTextSize include it? I think it must be a bug in our code somewhere.

@Maleclypse Maleclypse merged commit 2f865f7 into CleverRaven:master Sep 13, 2024
31 of 43 checks passed
@RenechCDDA RenechCDDA deleted the uilist_line_spacing branch September 13, 2024 13:54
@RenechCDDA
Copy link
Member Author

DB/CLIDragon if you want to reopen the linked issue around to track what's going on with the vertical spacing until you can really nail it, let me or other triage know. Whatever's easier for you to work on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect vertical size of uilist menus
4 participants