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

Bugfix/text underline #593

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@binary1248
Member

binary1248 commented May 12, 2014

No description provided.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 12, 2014

Member

This is actually 2 bugfixes in one PR.

Member

binary1248 commented May 12, 2014

This is actually 2 bugfixes in one PR.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 12, 2014

Member

Ok for me.

Member

LaurentGomila commented May 12, 2014

Ok for me.

@eXpl0it3r eXpl0it3r added this to the 2.2 milestone May 13, 2014

@eXpl0it3r eXpl0it3r added bug labels May 13, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.x, 2.2 May 13, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 14, 2014

Member

Out of curiosity, where did you find the magic numbers characterSize / 10 and characterSize / 14 for non-scalable fonts? Is it kind of standard, did you do some tests to find the best looking parameters, or did you throw a dice?

Member

LaurentGomila commented May 14, 2014

Out of curiosity, where did you find the magic numbers characterSize / 10 and characterSize / 14 for non-scalable fonts? Is it kind of standard, did you do some tests to find the best looking parameters, or did you throw a dice?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 14, 2014

Member

I think you can answer your own question better than I can ;)
https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Graphics/Text.cpp#L265
Those values are only used as a fallback, in which case it uses the "old" behaviour.

Member

binary1248 commented May 14, 2014

I think you can answer your own question better than I can ;)
https://github.com/LaurentGomila/SFML/blob/master/src/SFML/Graphics/Text.cpp#L265
Those values are only used as a fallback, in which case it uses the "old" behaviour.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 14, 2014

Member

Doh...

Shouldn't getUnderlineThickness depend on the bold style? It would make sense.

Member

LaurentGomila commented May 14, 2014

Doh...

Shouldn't getUnderlineThickness depend on the bold style? It would make sense.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 14, 2014

Member

That's what I initially thought as well... But considering that the fallback is only returned for non-scalable fonts, you won't be able to notice any difference between the underline thickness of bold and non-bold characters because well... non-scalable fonts don't support emboldening :P.

From what I understand, the font glyphs in a bitmap font should already be styled as you would use it in your application, so glyphs should already be in italics/bold if you want italic/bold text. I don't know if the same applies to underlines as well, but it might be a possibility.

Member

binary1248 commented May 14, 2014

That's what I initially thought as well... But considering that the fallback is only returned for non-scalable fonts, you won't be able to notice any difference between the underline thickness of bold and non-bold characters because well... non-scalable fonts don't support emboldening :P.

From what I understand, the font glyphs in a bitmap font should already be styled as you would use it in your application, so glyphs should already be in italics/bold if you want italic/bold text. I don't know if the same applies to underlines as well, but it might be a possibility.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 14, 2014

Member

I was not talking about non-scalable fonts ;)

Member

LaurentGomila commented May 14, 2014

I was not talking about non-scalable fonts ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch May 14, 2014

Member

IMO the font style shouldn't affect underlines or strikethroughs. It won't for classic typewriters either (for obvious reasons). If you're able to set the style or thickness of the underline, it shouldn't change based on font style (e.g. in Word you'll set the font size but afaik this won't have any influence on the underline thickness.

Member

MarioLiebisch commented May 14, 2014

IMO the font style shouldn't affect underlines or strikethroughs. It won't for classic typewriters either (for obvious reasons). If you're able to set the style or thickness of the underline, it shouldn't change based on font style (e.g. in Word you'll set the font size but afaik this won't have any influence on the underline thickness.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 14, 2014

Member

I was not talking about non-scalable fonts ;)

Well... since underline_thickness and underline_position are taken directly from the scaled FreeType face, it will scale them for us ;). No need to get our hands dirty when others already did so... especially since this value was available the whole time and SFML just didn't use it.

e.g. in Word you'll set the font size but afaik this won't have any influence on the underline thickness.

Actually, I tried when Laurent initially asked, and Word does scale the underline thickness based on font size. It kind of makes sense if you think of a hypothetical case where you would have a 1000 size font and only end up having 3 pixels of underline ;).

Member

binary1248 commented May 14, 2014

I was not talking about non-scalable fonts ;)

Well... since underline_thickness and underline_position are taken directly from the scaled FreeType face, it will scale them for us ;). No need to get our hands dirty when others already did so... especially since this value was available the whole time and SFML just didn't use it.

e.g. in Word you'll set the font size but afaik this won't have any influence on the underline thickness.

Actually, I tried when Laurent initially asked, and Word does scale the underline thickness based on font size. It kind of makes sense if you think of a hypothetical case where you would have a 1000 size font and only end up having 3 pixels of underline ;).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 15, 2014

Member

Well... since underline_thickness and underline_position are taken directly from the scaled FreeType face, it will scale them for us

But the FreeType face doesn't know you want bold glyphs. Bold is only applied when you render a glyph, not when you load a face. The loaded face has no style information. So when you request its underline thickness, it gives always the same value, regardless of the style.

Member

LaurentGomila commented May 15, 2014

Well... since underline_thickness and underline_position are taken directly from the scaled FreeType face, it will scale them for us

But the FreeType face doesn't know you want bold glyphs. Bold is only applied when you render a glyph, not when you load a face. The loaded face has no style information. So when you request its underline thickness, it gives always the same value, regardless of the style.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 15, 2014

Member

Hmm... Ok... I tried emboldening some text in Word with the Sansation font and the underline didn't change at all... The same applies for many other random fonts I tested. As examples, Arial and Times New Roman scale the thickness, Lucida Console and Handwriting don't. So I guess the style doesn't have to have an effect on the underline thickness? I have no idea what Word does with the fonts that embolden the underline, but in any case FreeType doesn't provide enough information to allow us to scale the thickness according to whether we want a bold underline or not. So I guess the implementation in this PR is as good as FreeType allows us to do unless I am missing something.

Member

binary1248 commented May 15, 2014

Hmm... Ok... I tried emboldening some text in Word with the Sansation font and the underline didn't change at all... The same applies for many other random fonts I tested. As examples, Arial and Times New Roman scale the thickness, Lucida Console and Handwriting don't. So I guess the style doesn't have to have an effect on the underline thickness? I have no idea what Word does with the fonts that embolden the underline, but in any case FreeType doesn't provide enough information to allow us to scale the thickness according to whether we want a bold underline or not. So I guess the implementation in this PR is as good as FreeType allows us to do unless I am missing something.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 15, 2014

Member

I guess it makes sense. Imagine an underlined sentence with only one bold word in the middle (like a Google search result), it would be really weird to have the underline thicker under this word.

So, this PR is definitely OK for me :p

Member

LaurentGomila commented May 15, 2014

I guess it makes sense. Imagine an underlined sentence with only one bold word in the middle (like a Google search result), it would be really weird to have the underline thicker under this word.

So, this PR is definitely OK for me :p

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 15, 2014

Member

Try typing "Welcome to SFML Pong!" into Word using Arial with underline and make sure it fits on one line. If you embolden all the text, the underline thickness is scaled. If you just set the "P" to bold, the underline thickness isn't scaled. Now for the fun part: If you break the line before the "P", so the bold "P" starts the next line, the underline thickness of the second line is scaled whereas the first isn't.

Hackish? Sure... Random? Possibly... I never tried to understand these things :P. I guess for professional printing that really puts value on these kinds of things, Word wouldn't be the prime candidate ^^.

Member

binary1248 commented May 15, 2014

Try typing "Welcome to SFML Pong!" into Word using Arial with underline and make sure it fits on one line. If you embolden all the text, the underline thickness is scaled. If you just set the "P" to bold, the underline thickness isn't scaled. Now for the fun part: If you break the line before the "P", so the bold "P" starts the next line, the underline thickness of the second line is scaled whereas the first isn't.

Hackish? Sure... Random? Possibly... I never tried to understand these things :P. I guess for professional printing that really puts value on these kinds of things, Word wouldn't be the prime candidate ^^.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila May 15, 2014

Member

Funny. Ok, let's keep things simple in SFML then 😄

Member

LaurentGomila commented May 15, 2014

Funny. Ok, let's keep things simple in SFML then 😄

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

I'd like to merge this and wonder if it can't be merged for 2.2 already? I don't see any API-breaking changes.

Member

TankOs commented Jun 11, 2014

I'd like to merge this and wonder if it can't be merged for 2.2 already? I don't see any API-breaking changes.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 11, 2014

Member

If it has been tested enough, yes, let's release it as soon as possible.

Member

LaurentGomila commented Jun 11, 2014

If it has been tested enough, yes, let's release it as soon as possible.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

No issues on my side, gonna merge this now, thanks.

Member

TankOs commented Jun 11, 2014

No issues on my side, gonna merge this now, thanks.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Merged, branch can be deleted.

Member

TankOs commented Jun 11, 2014

Merged, branch can be deleted.

@TankOs TankOs closed this Jun 11, 2014

@binary1248 binary1248 deleted the bugfix/text_underline branch Jun 11, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jun 12, 2014

Member

Don't forget to change the milestone to 2.2 if it's merged already now :)

Member

Bromeon commented Jun 12, 2014

Don't forget to change the milestone to 2.2 if it's merged already now :)

@Bromeon Bromeon modified the milestones: 2.2, 2.x Jun 12, 2014

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