Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

uilist line padding accounts for number of lines in text

Open RenechCDDA opened this issue 1 year ago • 4 comments

Summary

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

Purpose of change

  • Fixes #75698

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

RenechCDDA avatar Sep 11 '24 12:09 RenechCDDA

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

RenechCDDA avatar Sep 11 '24 17:09 RenechCDDA

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.

db48x avatar Sep 11 '24 20:09 db48x

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)

CLIDragon avatar Sep 12 '24 17:09 CLIDragon

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

db48x avatar Sep 12 '24 21:09 db48x

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

CLIDragon avatar Sep 13 '24 04:09 CLIDragon

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.

db48x avatar Sep 13 '24 05:09 db48x

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

RenechCDDA avatar Sep 13 '24 14:09 RenechCDDA