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

Fix DFKai-SB and KaiU font rendering #340

Merged
merged 5 commits into from
Jun 29, 2023
Merged

Conversation

JimBobSquarePants
Copy link
Member

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes #337

There's a lot going on here.

The font in question requires hinting in order to be displayed properly (Thanks @behdad !!) but we were not loading the instructions for composite glyphs from the font. As such, I ended up doing a significant refactoring (massively simplifying) of our glyph loading mechanism to make it possible to do so. There is a lot less cloning going on now so far fewer allocations will take place.

TrueTypeInterpreter changes are based on Freetype's implementation and all tests visual and unit remain unaffected.

Here's the correctly rendered font.

sb

Questions.

We do not hint by default as it creates overhead and legacy fonts such as Arial can have minor errors at some font sizes yet this font specifically requires hinting. Should we do as FreeType do and explicitly force hinting if the font is in a known list?

https://github.com/behdad/freetype/blob/08f66322e32cc882733dfae408e040f5057ee2ac/src/truetype/ttobjs.c#L181

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #340 (a84df2c) into main (b2396dc) will increase coverage by 0%.
The diff coverage is 77%.

@@          Coverage Diff          @@
##            main    #340   +/-   ##
=====================================
  Coverage     83%     83%           
=====================================
  Files        227     225    -2     
  Lines      12859   12807   -52     
  Branches    1839    1837    -2     
=====================================
- Hits       10772   10757   -15     
+ Misses      1652    1613   -39     
- Partials     435     437    +2     
Flag Coverage Δ
unittests 83% <77%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nts/Tables/AdvancedTypographic/GPos/AnchorTable.cs 33% <0%> (ø)
...abors.Fonts/Tables/TrueType/Glyphs/ControlPoint.cs 40% <40%> (ø)
...nts/Tables/TrueType/Hinting/TrueTypeInterpreter.cs 55% <48%> (+4%) ⬆️
src/SixLabors.Fonts/Bounds.cs 86% <88%> (-4%) ⬇️
src/SixLabors.Fonts/Tables/Woff/Woff2Utils.cs 78% <93%> (+<1%) ⬆️
...nts/Tables/TrueType/Glyphs/CompositeGlyphLoader.cs 98% <94%> (-2%) ⬇️
src/SixLabors.Fonts/StreamFontMetrics.TrueType.cs 97% <100%> (ø)
...s.Fonts/Tables/TrueType/Glyphs/EmptyGlyphLoader.cs 100% <100%> (ø)
...Labors.Fonts/Tables/TrueType/Glyphs/GlyphVector.cs 94% <100%> (-1%) ⬇️
....Fonts/Tables/TrueType/Glyphs/SimpleGlyphLoader.cs 96% <100%> (-1%) ⬇️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@tocsoft tocsoft left a comment

Choose a reason for hiding this comment

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

code as it stand look good to me.

The question on overriding the hinting I feel is a little thorny. It feels wrong each font shaper/renderer having to maintain magic lists of special case fonts all over the place... having a global resource tracking these, arguably broken, fonts might be a useful tool to maintain (something that all the libraries can benefit from).. or users can see if they are using specific fonts need specific override settings.

We should then provide a mechanism at the FontCollection level to declare these sorts of override (forcing hinting on/off etc) forcing some specific shapings in other situations maybe too.

These sorts of font issues should be a thing that developers are made aware of, which is why I think an online tool/resource tracking these things and what the overrides shoud be is real answer here. It will end up being a never ending loop of 'I have a boken font, please add the overrides to lib x & lib y etc'.... it should be something that a user of Fonts can self discover and apply the fix in thier code themselves... which will be espcially important for those that are using it in applications where their users are supplying thier own fonts.

If the tracker/online tool is built right it could also be used as a source of metadata needed to auto produce a nuget that can apply all its defaults even... but built/released independently to Font as and when fonts with issues are discovered and configured with their fixes.

@JimBobSquarePants
Copy link
Member Author

code as it stand look good to me.

The question on overriding the hinting I feel is a little thorny. It feels wrong each font shaper/renderer having to maintain magic lists of special case fonts all over the place... having a global resource tracking these, arguably broken, fonts might be a useful tool to maintain (something that all the libraries can benefit from).. or users can see if they are using specific fonts need specific override settings.

We should then provide a mechanism at the FontCollection level to declare these sorts of override (forcing hinting on/off etc) forcing some specific shapings in other situations maybe too.

These sorts of font issues should be a thing that developers are made aware of, which is why I think an online tool/resource tracking these things and what the overrides shoud be is real answer here. It will end up being a never ending loop of 'I have a boken font, please add the overrides to lib x & lib y etc'.... it should be something that a user of Fonts can self discover and apply the fix in thier code themselves... which will be espcially important for those that are using it in applications where their users are supplying thier own fonts.

If the tracker/online tool is built right it could also be used as a source of metadata needed to auto produce a nuget that can apply all its defaults even... but built/released independently to Font as and when fonts with issues are discovered and configured with their fixes.

Yep. Greate initiative!!

I've created SixLabors/docs#60 for now so we at least document them.

@JimBobSquarePants JimBobSquarePants merged commit 7816350 into main Jun 29, 2023
@JimBobSquarePants JimBobSquarePants deleted the js/fix-composite-glyphs branch June 29, 2023 22:40
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.

DrawText generates text with DFKai-SB or KaiU fonts differently than expected
2 participants