fix - Some glyph are not visually centered when patching monospaced variation#1998
Draft
OzelotVanilla wants to merge 1 commit intoryanoasis:masterfrom
Draft
Conversation
Author
|
This PR is made as drafted until the discussion is done in #1997. |
…ariation See issue ryanoasis#1997 for detail. Some glyph in font `Noto`, has some double width glyph with the bounding box width around single width. This commit fixes that by re-centering glyph that could be visually centered while creating monospaced variation. Some debug code is preserved, and would be deleted once the PR is ready to be merged.
99f1beb to
2f9b563
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
See issue #1997 for detail.
Some glyph in font
Noto, has some double width glyph with the bounding box width around single width.Some debug code is preserved, and would be deleted once the PR is ready to be merged.
If this PR could be merged, it will:
fixes #1997
Requirements / Checklist
Issue number where discussion took place:
--monoturns NotoSansMonoU+25C6into a broken single-width glyph with negative RBearing #1997What does this Pull Request (PR) do?
This PR fixes that by re-centering glyph that could be visually centered while creating monospaced variation.
A variable
glyph_centering_threshold(currently set to20) is used to decide whether a glyph will go into the "visually center" process or not. When the glyph's bounding box is less than single glyph width plus threshold, it will be considered able to be visually centered.Notice: This threshold is currently empirical, and may need to be derived from font metrics, weight, or be exposed as a parameter and then defined in config.
How should this be manually tested?
Use fontforge to compare the previous font and newly patched font, the new one would have a correct bearing for glyph listed in #1997 (
affected_charsat the bottom of issue, not bottom of discussion).Any background context you can provide?
When I am using
jj-vcs, I found the "◆" (U+25C6) for immutable commit injj log, is not horizontally centered with the verticle bar in the line above/below.I pasted the char into Google Fonts and saw the tofu, so I misunderstood and thinking about this is caused by
copy_glyphwhen I read the source code.However, when I see the file in the unpatched font (
src/unpatched-fonts/Noto/Sans-Mono/NotoSansMono-Regular.ttf) by fontforge, I found they were having a width of 1200 (double width), and the RBearing becomes negative afterfont_patcher.set_glyph_width_mono.I checked all the glyph in the unpatched font, and found some glyphs having a bounding box width less or around 600, but all having a negative RBearing (
affected_chars).So I tried to visually center those glyph that:
glyph.widthwas set.Of course, there are also some glyph that should not be vertically centered into monospace width, such as:
In order to not introduce compability changes, those kind of glyph is kept their original metrics in this PR.
What are the relevant tickets (if any)?
Screenshots (if appropriate or helpful)
This image, taken from fontforge metrics window, shows the result of patched font, L/R Bearing fixed by this PR.
See also #1997 for more screenshots explaning why this bug exists and how it could be solved.