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

scale height to (ascender-descender), preserve aspect, center width #255

Merged
merged 9 commits into from
Mar 11, 2021

Conversation

anthrotype
Copy link
Member

@anthrotype anthrotype commented Mar 9, 2021

Fixes #2 (comment) and #230

Previously we were mapping the SVG viewBox to UPEM square (0, 0, upem, upem), stretching it when the aspect ratio was not square, and setting the advance width to the same monospaced value.

After this PR:

  1. the viewbox height is scaled to match the sum of font (ascender - descender), and the width is scaled by the same factor (thus preserving the aspect ratio); the design centered horizontally within the glyph advance width. This is what NotoColorEmoji also does, and it makes the emoji designs look more visually centered when set alongside normal text (feature request: option to shift vertically the "baseline" of SVGs #2).
  2. changed the default FontConfig.width to the same value of NotoColorEmoji: this is wider than default upem, and a bit wider than the default (ascender - descender). Rationale: if it looks good with noto-emoji, should look good enough for other emoji designs too
  3. the advance width is now proportional to the aspect ratio: i.e. if viewbox width == height (square viewbox), then the nominal width doesn't change, and if all input SVGs have square ratio the font will be monospaced (all advance widths == config.width); if a viewbox is non-square (like the example duck in The shape is stretched after the font is generated #230), then the advance width is also scaled to fit the scaled viewbox; thus a glyph advance width can be wider or narrower than the nominal width depending on the input SVG aspect ratio.

@anthrotype anthrotype marked this pull request as draft March 9, 2021 19:52
@anthrotype anthrotype changed the title WIP: scale viewbox to match (ascender-descender) font height scale height to (ascender-descender), preserve aspect, center width Mar 10, 2021
@anthrotype anthrotype marked this pull request as ready for review March 10, 2021 17:13
tests/color_glyph_test.py Outdated Show resolved Hide resolved
@yisibl
Copy link
Contributor

yisibl commented Mar 11, 2021

A great improvement, with PR #256, the generated color icons have almost the same effect as the black and white icons in iconfont.

image

@rsheeter
Copy link
Collaborator

ty for sharing that example!

@drott
Copy link

drott commented Mar 11, 2021

Yeyh for fixing #2, big thanks!

@yisibl
Copy link
Contributor

yisibl commented Mar 11, 2021

@drott Next is waiting for you to fix Chrome's COLR font blurring problem.

@drott
Copy link

drott commented Mar 11, 2021

@drott Next is waiting for you to fix Chrome's COLR font blurring problem.

Let's stay on topic on this PR. If you are referring to https://bugs.chromium.org/p/chromium/issues/detail?id=1112051 - please comment there (and optionally attach screenshots) if you observe this issue for COLRv1 as well.

looks like I'm only using the default --width when parsing gradients, and not taking into account a potentially scaled glyph advance widths (by aspect ratio) that we apply for the outlines when the viewbox is non-square. I'll fix that in a follow-up commit.
I deliberately did not use absl.flags validators because those would not check if an invalid value was passed via config file, as opposed to CLI flags. There's probably a cleverer way, but at least this works for both.
Previously, I was simply scaling the FontConfig.width by the aspect ratio. But that can produce unintended consequences if the default width != font height (ascender-descender): e.g. if one wanted to pad glyphs on both sides (like we do with noto-emoji) by setting a wider default width than the font height, the impact of non-square viewbox on that padding would be even greater as the viewbox gets wider.
It's better to do this: given the font height and the viewbox aspect ratio, scale font heigth by aspect ratio to obtain the scaled advance width; ise the latter if it's larger than the default width, otherwise use the default width (and place things centered).
A user can play with the default --width (even set it to 0) until they like it. This effectively becomes the minimum default advance width, to be used only used when the proportional advance width that is computed from the viewbox width (relative to its height which is fixed to ascender-descender) is smaller. Yeah, I think I like that.
@anthrotype
Copy link
Member Author

anthrotype commented Mar 11, 2021

Two new notable changes:

  1. there was a bug in that I was not taking into account the particular glyph advance width when mapping gradient coordinates to font space, but was always assuming the default config.width (I fixed it with 09c8af0, test 20e45db).

  2. I changed the logic for computing the glyph advance width given the font height (ascender-descender) and the viewbox aspect ratio. We now use that only when it exceeds the default advance width. If smaller (e.g. viewbox taller than wider, or the default width more generous than the font height), we use the default width and place things centered horizontally. The way I set it up initially did not make sense in all cases, because simply scaling the default width by the aspect ratio would produce an outsized effect on that extra left/right padding, which I believe is not what one really wants. The logic now is this:

def _color_glyph_advance_width(view_box: Rect, config: FontConfig) -> int:
    # Scale advance width proportionally to viewbox aspect ratio.
    # Use the default advance width if it's larger than the proportional one.
    font_height = config.ascent - config.descent  # descent <= 0
    return max(config.width, round(font_height * view_box.w / view_box.h))

@rsheeter
Copy link
Collaborator

LGTM

The ascender/descender forms are more common in typography. Also UFO and the Oxford English Dictionary agree with me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: option to shift vertically the "baseline" of SVGs
4 participants