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

More fixes for text rendering #693

Merged
merged 1 commit into from Oct 2, 2014

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented Aug 28, 2014

  • Fixed missplaced underline with some fonts (including Windows' Arial)
  • Fixed glyphs being 2 pixels too large in each dimension because of padding being applied to glyph bounding boxes as well
  • Moved underline and strike through a bit so their centre is where it is supposed to be
Show outdated Hide outdated src/SFML/Graphics/Font.cpp Outdated
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

Should be tested with different families of fonts and sizes of text (especially very small text), to make sure that everything is rendered at least as good as before ;)

Member

LaurentGomila commented Aug 28, 2014

Should be tested with different families of fonts and sizes of text (especially very small text), to make sure that everything is rendered at least as good as before ;)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

If you (@binary1248) provide a ZIP containing some fonts and a cpp that does what @LaurentGomila said, we'll be willing to test it, I guess. ;)

Member

TankOs commented Aug 28, 2014

If you (@binary1248) provide a ZIP containing some fonts and a cpp that does what @LaurentGomila said, we'll be willing to test it, I guess. ;)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 28, 2014

Member

You know... if you hurry up with the unit test branch, I can probably write a few tests to check for broken font rendering with a bunch of fonts 😉. It would be painful to have to manually check all of this every time some small things change... That is the point of automated tests anyway.

Member

binary1248 commented Aug 28, 2014

You know... if you hurry up with the unit test branch, I can probably write a few tests to check for broken font rendering with a bunch of fonts 😉. It would be painful to have to manually check all of this every time some small things change... That is the point of automated tests anyway.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

Since the branch already exists, and everybody is so far happy with the style of it, just go ahead and write the tests. ;)

Serious again: Many bugfixes ask to test stuff but don't provide a test case that I can simply compile and run. It would be much easier for us to test the bugfix if you attached the code you used to test your changes. Sure, everybody can rather quickly write a cpp file that reproduces the scenario, but in my opinion it would be more effective if the one who fixed the bug (and therefore already has a test -- otherwise he can't confirm the fix himself) simply attaches it, so that others can use it.

Member

TankOs commented Aug 28, 2014

Since the branch already exists, and everybody is so far happy with the style of it, just go ahead and write the tests. ;)

Serious again: Many bugfixes ask to test stuff but don't provide a test case that I can simply compile and run. It would be much easier for us to test the bugfix if you attached the code you used to test your changes. Sure, everybody can rather quickly write a cpp file that reproduces the scenario, but in my opinion it would be more effective if the one who fixed the bug (and therefore already has a test -- otherwise he can't confirm the fix himself) simply attaches it, so that others can use it.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

How do you test font rendering automatically? You'll have to check the pixels of rendered text, which cannot be done automatically since a different version of FreeType or graphics driver may result in a slight variation of the rendered text.

Member

LaurentGomila commented Aug 28, 2014

How do you test font rendering automatically? You'll have to check the pixels of rendered text, which cannot be done automatically since a different version of FreeType or graphics driver may result in a slight variation of the rendered text.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

@LaurentGomila Yes, that's very difficult. It's generally very difficult to test visual output. One could still go for an approximation test or something.

Here's my test: https://gist.github.com/TankOs/baad3ff67281a0de7df6 (if you want to run that yourself, you have to add the font files (check source) to the same directory; I can't include them due to copyright stuff)
Screenshot: http://i.imgur.com/iRXGVuZ.png

Small characters sizes tend to make the line invisible (minimum height 1px?), and for some fonts it's a little off.

Member

TankOs commented Aug 28, 2014

@LaurentGomila Yes, that's very difficult. It's generally very difficult to test visual output. One could still go for an approximation test or something.

Here's my test: https://gist.github.com/TankOs/baad3ff67281a0de7df6 (if you want to run that yourself, you have to add the font files (check source) to the same directory; I can't include them due to copyright stuff)
Screenshot: http://i.imgur.com/iRXGVuZ.png

Small characters sizes tend to make the line invisible (minimum height 1px?), and for some fonts it's a little off.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 28, 2014

Member

Sorry, didn't use the latest commit: http://i.imgur.com/IiZkiHA.png

Member

TankOs commented Aug 28, 2014

Sorry, didn't use the latest commit: http://i.imgur.com/IiZkiHA.png

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

minimum height 1px?

Sounds like a better result than no underline at all. But it may be tricky to implement: 1 unit can be more or less than one pixel. For example, if you use a zoomed view in your test, underlines of small texts should appear.

Member

LaurentGomila commented Aug 28, 2014

minimum height 1px?

Sounds like a better result than no underline at all. But it may be tricky to implement: 1 unit can be more or less than one pixel. For example, if you use a zoomed view in your test, underlines of small texts should appear.

@sywesk

This comment has been minimized.

Show comment
Hide comment
@sywesk

sywesk Aug 28, 2014

@TankOs @LaurentGomila Concerning #692, the picture of @TankOs explains everything, 13 for character size is too little to currently have an underline.

sywesk commented Aug 28, 2014

@TankOs @LaurentGomila Concerning #692, the picture of @TankOs explains everything, 13 for character size is too little to currently have an underline.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 28, 2014

Member

For example, if you use a zoomed view in your test, underlines of small texts should appear.

I don't think this will be the case. The underline returned by sf::Font for a given character size is an integer value. At some point converting the FreeType 26.6 value to an integer value will lose so much precision that it will constantly return 0. I guess that point is already reached at size 13 for Arial.

Since sf::Font is normally used for raster text rendering, I don't think changing the relevant functions to return floats instead makes much sense.

So, basically, everybody agrees that we just set the minimum underline thickness that is returned from sf::Font to be equal to 1 pixel right?

Member

binary1248 commented Aug 28, 2014

For example, if you use a zoomed view in your test, underlines of small texts should appear.

I don't think this will be the case. The underline returned by sf::Font for a given character size is an integer value. At some point converting the FreeType 26.6 value to an integer value will lose so much precision that it will constantly return 0. I guess that point is already reached at size 13 for Arial.

Since sf::Font is normally used for raster text rendering, I don't think changing the relevant functions to return floats instead makes much sense.

So, basically, everybody agrees that we just set the minimum underline thickness that is returned from sf::Font to be equal to 1 pixel right?

@sywesk

This comment has been minimized.

Show comment
Hide comment
@sywesk

sywesk Aug 28, 2014

I agree with you solution @binary1248 😄

sywesk commented Aug 28, 2014

I agree with you solution @binary1248 😄

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

I think it would be more correct to use a floating point value for the thickness. This way, the underline thickness will still match the text size if I zoom / unzoom, and not be stuck at 1 unit (not 1 pixel -- we have no idea how many pixels the rendered underline will take after being transformed).

Member

LaurentGomila commented Aug 28, 2014

I think it would be more correct to use a floating point value for the thickness. This way, the underline thickness will still match the text size if I zoom / unzoom, and not be stuck at 1 unit (not 1 pixel -- we have no idea how many pixels the rendered underline will take after being transformed).

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 28, 2014

Member

Meh... just pushed and saw your comment 😛. Since we are going for float thickness, I might try and implement line alpha blending in sf::Text so the lines fit better to small text which isn't rendered at full opacity. It still looks strange to me when the glyphs are anti-aliased but the lines not 😄.

Member

binary1248 commented Aug 28, 2014

Meh... just pushed and saw your comment 😛. Since we are going for float thickness, I might try and implement line alpha blending in sf::Text so the lines fit better to small text which isn't rendered at full opacity. It still looks strange to me when the glyphs are anti-aliased but the lines not 😄.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

Another solution is to treat underlined text the same way as bold text: let sf::Font pre-render underlined glyphs and directly use them. This is of course much less efficient in case of using both underlined and regular text with the same font, since it doubles the required amount of texture data.

Member

LaurentGomila commented Aug 28, 2014

Another solution is to treat underlined text the same way as bold text: let sf::Font pre-render underlined glyphs and directly use them. This is of course much less efficient in case of using both underlined and regular text with the same font, since it doubles the required amount of texture data.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 28, 2014

Member

Another solution is to treat underlined text the same way as bold text: let sf::Font pre-render underlined glyphs and directly use them. This is of course much less efficient in case of using both underlined and regular text with the same font, since it doubles the required amount of texture data.

Except that... FreeType can't render underlines for us 😁. There is no "underline" function like there is an "embolden". It provides us with the underline metrics exactly because we have to take care of it ourselves.

I just noticed: Kerning and line spacing currently also return ints. If we go ahead and support really small font sizes, they should probably also be changed to return floats as well.

Member

binary1248 commented Aug 28, 2014

Another solution is to treat underlined text the same way as bold text: let sf::Font pre-render underlined glyphs and directly use them. This is of course much less efficient in case of using both underlined and regular text with the same font, since it doubles the required amount of texture data.

Except that... FreeType can't render underlines for us 😁. There is no "underline" function like there is an "embolden". It provides us with the underline metrics exactly because we have to take care of it ourselves.

I just noticed: Kerning and line spacing currently also return ints. If we go ahead and support really small font sizes, they should probably also be changed to return floats as well.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 28, 2014

Member

Except that... FreeType can't render underlines for us 😁. There is no "underline" function like there is an "embolden". It provides us with the underline metrics exactly because we have to take care of it ourselves.

We can pre-render it ourselves into the font's texture, it's just a line ;)

I just noticed: Kerning and line spacing currently also return ints. If we go ahead and support really small font sizes, they should probably also be changed to return floats as well.

Yep.

Member

LaurentGomila commented Aug 28, 2014

Except that... FreeType can't render underlines for us 😁. There is no "underline" function like there is an "embolden". It provides us with the underline metrics exactly because we have to take care of it ourselves.

We can pre-render it ourselves into the font's texture, it's just a line ;)

I just noticed: Kerning and line spacing currently also return ints. If we go ahead and support really small font sizes, they should probably also be changed to return floats as well.

Yep.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 29, 2014

Member

The current version has sf::Font and sf::Glyph using floats instead of ints to store metric data.

I held off from rendering the underline/strikethrough into the texture data because, as you said, it would take up more memory (up to 3x more) and it wouldn't solve the issue with the underline being brighter than the glyphs. Unless we anti-alias the lines before combining them into the texture, it will still appear as a solid line even in the texture. When scaling the font up or down, the already anti-aliased glyphs would get filtered twice (if smoothing is enabled as well) but the lines would only get filtered once. Let's just stick to how it is now and stop worrying about such minor details that only I realize 😉.

Member

binary1248 commented Aug 29, 2014

The current version has sf::Font and sf::Glyph using floats instead of ints to store metric data.

I held off from rendering the underline/strikethrough into the texture data because, as you said, it would take up more memory (up to 3x more) and it wouldn't solve the issue with the underline being brighter than the glyphs. Unless we anti-alias the lines before combining them into the texture, it will still appear as a solid line even in the texture. When scaling the font up or down, the already anti-aliased glyphs would get filtered twice (if smoothing is enabled as well) but the lines would only get filtered once. Let's just stick to how it is now and stop worrying about such minor details that only I realize 😉.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Aug 29, 2014

Member

Yep.

Did using floats for the underline thickness solve the issue with small texts? Or do we still need a minimum value?

Member

LaurentGomila commented Aug 29, 2014

Yep.

Did using floats for the underline thickness solve the issue with small texts? Or do we still need a minimum value?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 29, 2014

Member

OpenGL emits fragments when faces have non-zero area. So as long as the lines form a non-zero area, they will appear as a 1 pixel thick line.

Even if the line vertices are scaled up, really thin lines will still appear as 1 pixel thick lines as long as the rasterizer thinks they should be 1 pixel thick 😉.

Member

binary1248 commented Aug 29, 2014

OpenGL emits fragments when faces have non-zero area. So as long as the lines form a non-zero area, they will appear as a 1 pixel thick line.

Even if the line vertices are scaled up, really thin lines will still appear as 1 pixel thick lines as long as the rasterizer thinks they should be 1 pixel thick 😉.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 18, 2014

Member

So what exactly is the conclusion here?

Member

eXpl0it3r commented Sep 18, 2014

So what exactly is the conclusion here?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 19, 2014

Member

I think everything is ok.

Member

LaurentGomila commented Sep 19, 2014

I think everything is ok.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 19, 2014

Member

Looking at the code, there's a minor API break with the change from IntRect to FloatRect of the public member in sf::Glyph.
Since sf::Glyph is more an "internal" class, the change is minor and it would be a shame to have this only after SFML 3, I think we could ignore the breaking factor for once.
What do you think, @LaurentGomila?

Member

eXpl0it3r commented Sep 19, 2014

Looking at the code, there's a minor API break with the change from IntRect to FloatRect of the public member in sf::Glyph.
Since sf::Glyph is more an "internal" class, the change is minor and it would be a shame to have this only after SFML 3, I think we could ignore the breaking factor for once.
What do you think, @LaurentGomila?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Sep 19, 2014

Member

Cases where this change actually breaks something are very specific, I don't think it will annoy anyone. We already did it in similar situations (the blending modes modification for example).

Member

LaurentGomila commented Sep 19, 2014

Cases where this change actually breaks something are very specific, I don't think it will annoy anyone. We already did it in similar situations (the blending modes modification for example).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Sep 24, 2014

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

Underlines don't just disappear when the character size is below a threshold, they sometimes disappear and then reappear again at lower sizes. I'm using Tank's test code with the latest commit on Windows 8.1, the result is here.

Looks like underlines are really dependent on "lucky" rasterization right now...

Member

Bromeon commented Sep 30, 2014

Underlines don't just disappear when the character size is below a threshold, they sometimes disappear and then reappear again at lower sizes. I'm using Tank's test code with the latest commit on Windows 8.1, the result is here.

Looks like underlines are really dependent on "lucky" rasterization right now...

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Sep 30, 2014

Member

Is the only way to avoid the irregular line rendering really to duplicate every glyph for an underline and a strikethrough version? Can't we render the line to a separate texture -- once per character size -- and then reuse that texture for multiple characters? Different character widths can be handled by rendering only a sub-rect of the texture, the alpha-blending overlay for transparent texts however remains. And it's of course a lot of effort for very small gain... I'm not saying we have to do that right now, but I'd like to understand the limitations more precisely :)

Member

Bromeon commented Sep 30, 2014

Is the only way to avoid the irregular line rendering really to duplicate every glyph for an underline and a strikethrough version? Can't we render the line to a separate texture -- once per character size -- and then reuse that texture for multiple characters? Different character widths can be handled by rendering only a sub-rect of the texture, the alpha-blending overlay for transparent texts however remains. And it's of course a lot of effort for very small gain... I'm not saying we have to do that right now, but I'd like to understand the limitations more precisely :)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 30, 2014

Member

Well... FreeType renders glyphs smoothed using hinting data into the textures that SFML ends up using. In order to add the underline to the texture data, we would have to disable smoothing by FreeType and do it ourself so that the glyphs don't end up being smoothed twice. This means that we are disregarding the hinting that is provided with the font (ugly) in addition to the fact that we would end up using double as much texture memory.

The glitches with the underline showing up at some sizes and not at others has to do with the rasterization process, yes. Thankfully it is deteministic, i.e. reproducible within the same environment, although different hardware and even driver versions might produce different results.

There is a simple and yet complicated solution for this. Wide line support (i.e. line widths > 1.0f) is deprecated in modern GL. In order to render wide lines, you would just render a quad that is aligned to the raster positions. For thin lines, your best bet is still to use GL_LINES since they translate directly into pixels in the rasterizer and don't cause as many surprises.

I guess as a first attempt to solve this behaviour, we can try locking the underline/strikethrough to proper raster coordinates in sf::Text so that the thin line doesn't have to be split among 2 pixels.

Member

binary1248 commented Sep 30, 2014

Well... FreeType renders glyphs smoothed using hinting data into the textures that SFML ends up using. In order to add the underline to the texture data, we would have to disable smoothing by FreeType and do it ourself so that the glyphs don't end up being smoothed twice. This means that we are disregarding the hinting that is provided with the font (ugly) in addition to the fact that we would end up using double as much texture memory.

The glitches with the underline showing up at some sizes and not at others has to do with the rasterization process, yes. Thankfully it is deteministic, i.e. reproducible within the same environment, although different hardware and even driver versions might produce different results.

There is a simple and yet complicated solution for this. Wide line support (i.e. line widths > 1.0f) is deprecated in modern GL. In order to render wide lines, you would just render a quad that is aligned to the raster positions. For thin lines, your best bet is still to use GL_LINES since they translate directly into pixels in the rasterizer and don't cause as many surprises.

I guess as a first attempt to solve this behaviour, we can try locking the underline/strikethrough to proper raster coordinates in sf::Text so that the thin line doesn't have to be split among 2 pixels.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 2, 2014

Member

Should I merge this and one can create another issue (if needed) for underline rendering?

Member

eXpl0it3r commented Oct 2, 2014

Should I merge this and one can create another issue (if needed) for underline rendering?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

I guess as a first attempt to solve this behaviour, we can try locking the underline/strikethrough to proper raster coordinates in sf::Text so that the thin line doesn't have to be split among 2 pixels.

I agree to that. Even if it looks a bit ugly for really small font sizes, it will solve the problem quickly at least. (And I wonder who seriously uses font sizes of 6pt or smaller ;))

Member

TankOs commented Oct 2, 2014

I guess as a first attempt to solve this behaviour, we can try locking the underline/strikethrough to proper raster coordinates in sf::Text so that the thin line doesn't have to be split among 2 pixels.

I agree to that. Even if it looks a bit ugly for really small font sizes, it will solve the problem quickly at least. (And I wonder who seriously uses font sizes of 6pt or smaller ;))

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2014

Member

And I wonder who seriously uses font sizes of 6pt or smaller ;)

You mean... who seriously uses font sizes of 6pt or smaller and has underline enabled 😉.

But yeah... just wait a bit... I will look into this maybe today and push a fix for the underline issue as part of this PR as well. We don't need to split it into a separate issue just because we can...

Member

binary1248 commented Oct 2, 2014

And I wonder who seriously uses font sizes of 6pt or smaller ;)

You mean... who seriously uses font sizes of 6pt or smaller and has underline enabled 😉.

But yeah... just wait a bit... I will look into this maybe today and push a fix for the underline issue as part of this PR as well. We don't need to split it into a separate issue just because we can...

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Yep, agreed.

Member

TankOs commented Oct 2, 2014

Yep, agreed.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2014

Member

Should be fixed now.

Once the lines disappear... they disappear forever (since they are too thin). 😀

Member

binary1248 commented Oct 2, 2014

Should be fixed now.

Once the lines disappear... they disappear forever (since they are too thin). 😀

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 2, 2014

Member

Looks good. Squash and merge? :)

Member

TankOs commented Oct 2, 2014

Looks good. Squash and merge? :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 2, 2014

Member

"floor is not a member of std"

Looks like you forgot to #include <cmath>

Member

eXpl0it3r commented Oct 2, 2014

"floor is not a member of std"

Looks like you forgot to #include <cmath>

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 2, 2014

Member

Looks good! The only "problem" I see is that if you move an underlined text by continuous float coordinates, the underline will jump from pixel to pixel. But it did not look more beautiful before (fluctuate between 1 and 2 pixels width), so that's ok.

Member

Bromeon commented Oct 2, 2014

Looks good! The only "problem" I see is that if you move an underlined text by continuous float coordinates, the underline will jump from pixel to pixel. But it did not look more beautiful before (fluctuate between 1 and 2 pixels width), so that's ok.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2014

Member

There... the <cmath> issue should be fixed now and all the commits have been squashed. Have fun merging. 😁

Member

binary1248 commented Oct 2, 2014

There... the <cmath> issue should be fixed now and all the commits have been squashed. Have fun merging. 😁

Fixed font glyphs always being 2 pixels larger than they are supposed…
… to be in each dimension, fixed wrong underline offset with some fonts, offset underline and strike through by half of their thickness so their center is positioned correctly, changed glyph and font metrics to use floats instead of ints to support scaling better.

@eXpl0it3r eXpl0it3r merged commit b27cbd5 into master Oct 2, 2014

@eXpl0it3r eXpl0it3r deleted the bugfix/font_fix branch Oct 2, 2014

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