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

Added auto hinting compensation to fix too wide gaps between small glyphs #1746

Merged
merged 6 commits into from Feb 10, 2021

Conversation

oomek
Copy link
Contributor

@oomek oomek commented Jan 20, 2021

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Description

Forced Autohint requires left and right bearings to be taken into acccount during kerning calculation. Without this the gaps between small glyph sizes are twice as wide on a lot of fonts.

This PR is related to the issue #1642 (comment)

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

This improves glyph spacing by subtracting glyph position deltas from glyph advance generated by forced autohinting
Apply deltas even if the font has no kerning table
getKerning() now passes the bold flag to getGlyph() function, so already cached glyph can be used.
@oomek oomek changed the title Bugfix/autohint compensation implemented fixing too wide gaps between small glyphs Added auto hinting compensation to fix too wide gaps between small glyphs Jan 21, 2021
Copy link
Member

@LaurentGomila LaurentGomila left a comment

Choose a reason for hiding this comment

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

Looks ok, except some minor adjustments

include/SFML/Graphics/Glyph.hpp Outdated Show resolved Hide resolved
src/SFML/Graphics/Font.cpp Show resolved Hide resolved
@oomek
Copy link
Contributor Author

oomek commented Jan 21, 2021

Regarding that added function parameter:

float getKerning(Uint32 first, Uint32 second, unsigned int characterSize, bool bold = false) const;

Shall I update the brief as well? If so, do we need to indicate that when different bold flag is used than already cached the glyph will be loaded?

@eXpl0it3r
Copy link
Member

Shall I update the brief as well?

If you think the brief could need an update/change, then certainly.
What you certainly need to add is /param for the new parameter.

If so, do we need to indicate that when different bold flag is used than already cached the glyph will be loaded?

Does the user of the function need to care about this implementation detail?
If only in some very special cases, then no, I wouldn't add that information.

@oomek
Copy link
Contributor Author

oomek commented Jan 21, 2021

Maybe it's just me being paranoid, but I found 2 fonts out of 50 I tested with unusually narrow whitespace.
It's not relevant to kerning compensation, but how whitespaceWidth is calculated. Maybe it's also a good opportunity to address it aswell by clamping the whitespaceWidth to at least 2 pixels by:

    float whitespaceWidth = m_font->getGlyph(L' ', m_characterSize, isBold).advance;
    if (whitespaceWidth < 2.f) whitespaceWidth = 2.f;
    float letterSpacing   = ( whitespaceWidth / 3.f ) * ( m_letterSpacingFactor - 1.f );
    whitespaceWidth      += letterSpacing;

Here is the screenshot of the affected font before and after:
image
Please open it in 100% browser zoom to avoid any potential scaling.

Here is the current state for comparison:
image

@oomek
Copy link
Contributor Author

oomek commented Jan 22, 2021

Would you like me to commit that whiteSpaceWidth compensation as well?

@LaurentGomila
Copy link
Member

Since it's a different issue, let's address it in another PR.

@oomek
Copy link
Contributor Author

oomek commented Jan 23, 2021

Half of the CI tasks did not start. Do they have to be triggered manually?

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Feb 10, 2021
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Feb 10, 2021
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Feb 10, 2021
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Feb 10, 2021

Half of the CI tasks did not start. Do they have to be triggered manually?

Our own CI setup only executes on demand on third-party PRs for security reasons.
I have since triggered them.

@LaurentGomila any additional comment for the PR?

SFML 2.6.0 automation moved this from Review & Testing to Ready Feb 10, 2021
@eXpl0it3r eXpl0it3r merged commit 88451a0 into SFML:master Feb 10, 2021
SFML 2.6.0 automation moved this from Ready to Done Feb 10, 2021
@oomek oomek deleted the bugfix/autohint_compensation branch October 13, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants