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

Implement run-time extra line spacing #4742

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

thedmd
Copy link
Contributor

@thedmd thedmd commented Nov 20, 2021

This PR address #4578, #2945 feature requests.

Summary

I made a pass over ImGui and prepared necessary patch to support extra spacing in text.

I introduced two new parameters in ImFont:

  • ExtraLineHeight - by how many pixels line height should be increased, extra space will be equally distributed between top and bottom of the font
  • ExtraLineAdvance - by how many pixels spacing should be increased between consecutive lines of multiline texts, does not affect line height, just move cursor more

Tasks

  • Implement ExtraLineHeight
  • Implement ExtraLineAdvance Removed in favor of ExtraLineHeight
  • Implement BaselineOffset
  • Use FontLineHeight for vertical axis instead of FontSize
  • Collect feedbak
  • Rename _FontSize back to FontSize
  • Version bump / cleanup
  • Determine course of action with GetFontXXX functions

Preview

ExtraLineHeight=0 ExtraLineAdvance=0 image
ExtraLineHeight=8 ExtraLineAdvance=0 image
ExtraLineHeight=0 ExtraLineAdvance=8 image
ExtraLineHeight=8 ExtraLineAdvance=8 image

How atlas generation changed?

Atlas itself isn't affected. Change is entirely run-time only. Also Font scale, window scale and global scale still works as expected.

image

How does it behave in action?

https://user-images.githubusercontent.com/1197433/142724958-04c92c42-87ee-4473-b234-6b5322fd368d.mp4

@thedmd thedmd mentioned this pull request Nov 20, 2021
imgui.h Outdated
@@ -2742,6 +2743,8 @@ struct ImFont
ImVector<float> IndexAdvanceX; // 12-16 // out // // Sparse. Glyphs->AdvanceX in a directly indexable way (cache-friendly for CalcTextSize functions which only this this info, and are often bottleneck in large UI).
float FallbackAdvanceX; // 4 // out // = FallbackGlyph->AdvanceX
float FontSize; // 4 // in // // Height of characters/line, set during loading (don't change after loading)
float ExtraLineHeight; // 4 // in // // Extra line height
float ExtraLineAdvance; // 4 // in // // Extra spacing between lines
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment above should include these new fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added real comments here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The line right above describes how many bytes are hot for this struct.

imgui/imgui.h

Line 2744 in 630e42c

// Members: Hot ~20/24 bytes (for CalcTextSize)

I don't know how CalcTextSize() works exactly, but if it would want to break lines, it would prompt inclusion of these new fields into the hot memory calculations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved new fields down. They're not needed in hot path nor reason to invalidate assumption made by this comment.

Thanks for pointing that out.

@wolfpld
Copy link
Contributor

wolfpld commented Nov 20, 2021

In my use case I would want to use negative values. It may not be advisable to limit the sliders present in the "Hello world!" window to be >= 0.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 20, 2021

In my use case I would want to use negative values. It may not be advisable to limit the sliders present in the "Hello world!" window to be >= 0.

Negative values does work, these parameters are not limited to positive values.

@wolfpld
Copy link
Contributor

wolfpld commented Nov 20, 2021

Yes, I didn't saw anything in the code which would limit them. This is more about the immediate availability through the UI. It may be of use to some people, considering that most fonts now produce an ugly margin. I don't understand this, as typesetting is hard (let's go shopping) and manually fixing this through these parameters may be the easy way to go.

For example, this is Noto Sans:
obraz

And this is Droid Sans of the same size:
obraz

If a random comment on reddit is to be believed, these fonts should be rendered without any differences.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 20, 2021

If a random comment on reddit is to be believed, these fonts should be rendered without any differences.

They may share outlines, but overall metrics may be changed to better fit purpose they were created for. EM scaling appears to be different between them. That's my theory without looking at font files.

@meshula
Copy link

meshula commented Nov 20, 2021

I would definitely appreciate this.

Adding the extra equally on top and bottom would be problematic for me. I'm already using cursor position to fix typography in my Dear ImGui apps, and extra-equally wouldn't fix the issues for me.

Could you split the extra line height into two please? In typography it's common to need line spacing at the bottom only, and ocassionally at the top. If a font has a very low x height for example, it may be desirable to only add space at the bottom because the top of the font may already be very empty.

Another use case is attempting to align a text label with a particular graphic element. Adding spacing at top or bottom would be the most precise way to do that, if it's equal, I'd have to also adjust cursor position any way to compensate.

I think your proposal, with the split, would make my life much easier, as I think it would be less cumbersome that over-riding Dear ImGui's layout.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 20, 2021

I think I would add baseline offset rather than two separate spacings. It will allow to achieve the same effect while being more intuitive, I think.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 21, 2021

@meshula There is a baseline offset available now

image

@thedmd
Copy link
Contributor Author

thedmd commented Nov 22, 2021

From conversation outside of github.

What value to do see separating LineHeight and LineAdvance?

LineAdvance is used in multiline context allowing to control how much new lines will advance.
Can be safetly merged with LineHeight.

Shouldn't some of those values eventually become Style values, but then how we can be shared across multiple font?

Usually user want to tweak single font instance. Moving ExtraLineHeight and BaselineOffset to style would mean user will have to keep these values along with ImFont and remember to change style together with font. This appears to be less user friendly interface, so I put them in ImFont directly.

We already have a GetTextLineHeight() and GetTextLineHeightWithSpacing() btw need to clarify how they differs/overlap with new funcitons

GetFontSize() is used to calculate item width exclusively across ImGui.
GetFontLineHeight() was added for sake of completness. As it turns out GetTextLineHeight() does exactly same thing. GetFontLineHeight() can be safetly removed. GetFontSize() comment could point that is should not be used as font line height and GetTextLineHeight() should be used instead.
GetFontLineAdvance() may be removed if we decide to merge it with line height.
GetFontBaselineOffset() was added for completness, may be safetly removed.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 23, 2021

Changes:

  • I squashed tiny fixups together
  • Pushed a commit that removes ExtraLineAdvance in favor ExtraLineHeight

@ocornut ocornut force-pushed the master branch 2 times, most recently from 8b83e0a to d735066 Compare December 13, 2021 11:31
@wolfpld
Copy link
Contributor

wolfpld commented Dec 22, 2021

There is some interaction with how clipper works with these changes.

Setting ExtraLineHeight to a negative value and drawing clipped text lines, like such:

ImGuiListClipper clipper;
clipper.Begin( (int)lines.size() );
while( clipper.Step() )
{
    for( auto i=clipper.DisplayStart; i<clipper.DisplayEnd; i++ )
    {
        ImGui::Text( "text" );
    }
}

Produces blank space at the bottom of the visible clipped area.

obraz

@thedmd
Copy link
Contributor Author

thedmd commented Dec 22, 2021

I cannot reproduce the issue.
I tried also putting list in child window.

Is your copy of ImGui older than 1.86 WIP (18510)?
Clipper had some bug fixed, if you did cherry-picked this feature to older version of ImGui you still may them.

image

@wolfpld
Copy link
Contributor

wolfpld commented Dec 22, 2021

Is your copy of ImGui older than 1.86 WIP (18510)?

1.86 + docking + this change. This requires resolving conflicts, maybe I did something wrong there?

I cannot reproduce the issue.

This is the font setup I do:

scale = 1.5f;
fixedWidth = io.Fonts->AddFontFromMemoryCompressedTTF( tracy::FiraCodeRetina_compressed_data, tracy::FiraCodeRetina_compressed_size, round( 15.0f * scale ), &configBasic );
fixedWidth->ExtraLineHeight = -5.0f * scale;

With FiraCode 5.2 (https://github.com/tonsky/FiraCode/releases/tag/5.2).

The number of lines has to be significant (2500 in my case).

@thedmd
Copy link
Contributor Author

thedmd commented Dec 22, 2021

Ok, i will try to reproduce with that.

In the mean time, you might be interested with a few commits from feature/hidpi-support. Mainly:

  • aa4e8ef - Adds float ImFontConfig::Density, you get more detailed atlas texture without breaking metrics (this add this and support in embedded stb_truetype)
  • 44e829f - Adds support float ImFontConfig::Density in imgui_freetype
  • 1819260 - Use io.DisplayFramebufferScale to correctly scale AA fringe

There is FiraCode v6.2 available, is there any particular reason to stick to v5.2?

@wolfpld
Copy link
Contributor

wolfpld commented Dec 22, 2021

In the mean time, you might be interested with a few commits from feature/hidpi-support. Mainly:

I have hi-dpi support on Windows and Linux by changing font size. What is this feature branch improving here?

There is FiraCode v6.2 available, is there any particular reason to stick to v5.2?

6.0 was not monospace with Freetype, don't know how 6.x works in that regard. I guess 6.2 fixes this?

@thedmd
Copy link
Contributor Author

thedmd commented Dec 22, 2021

I have hi-dpi support on Windows and Linux by changing font size. What is this feature branch improving here?

Main perk is you can keep font size constant. UI designed with 1.0f will scale to DPI while staying sharp and mostly where it suppose to be.
Photoshop did good job scaling image up, widget borders and primitive fringes are blurry on scaled image.

1.0 1.0 resized to 1.5 vs. native 1.5
image imgui_hidpi_resize

6.0 was not monospace with Freetype, don't know how 6.x works in that regard. I guess 6.2 fixes this?

Release 6.2 appears to fix exactly that: https://github.com/tonsky/FiraCode/releases/tag/6.2 :

  • Fixed monospaced property #1325

I reproduced bug, I'm working on a fix.

@wolfpld
Copy link
Contributor

wolfpld commented Dec 22, 2021

UI designed with 1.0f will scale to DPI while staying sharp and mostly where it suppose to be.

That's how it currently works. The only platform where it's broken is MacOS (because ImGui behaves differently there).

The image below is a composite of two similar views with different font sizes (50% and 200%). The font size can be dynamically adjusted in run time.

obraz

@thedmd
Copy link
Contributor Author

thedmd commented Dec 22, 2021

That's how it currently works. The only platform where it's broken is MacOS (because ImGui behaves differently there).

On my branch I made osx example work. Granted this was done with io.DisplayFramebufferScale and ImFontConfig::Density, which my go to solution for DPI problem regardless of platform.

The image below is a composite of two similar views with different font sizes (50% and 200%). The font size can be dynamically adjusted in run time.

Looks like everything in your app is scalable. Cool, this works too.


Problem is present on current master branch too. I think I will hunt it there first and then introduce line spacing into the mix.
That was totally my derp. It is fixed now.
ImGui isn't happy when font size / line height has fractional part, so scaled metrics are rounded.

@wolfpld
Copy link
Contributor

wolfpld commented Dec 23, 2021

Problem is present on current master branch too. I think I will hunt it there first and then introduce line spacing into the mix.

Now that you have said this, I think I had this exact problem on master some time ago. I resorted to just rounding the font size to an integer value.

Anyways, the solution works. I am just rounding the value passed to ExtraLineHeight, so I haven't tested the actual fix.

@wolfpld
Copy link
Contributor

wolfpld commented Apr 26, 2022

I have realized that for my use case I need just to set ItemSpacing to 0, e.g.:

ImGui::PushStyleVar( ImGuiStyleVar_ItemSpacing, ImVec2( 0, 0 ) );

@thedmd
Copy link
Contributor Author

thedmd commented Aug 19, 2022

Rebased on v1.89 WIP (18808)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants