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

maximum_color is unhappy with > 1 palette #421

Closed
rsheeter opened this issue Jul 28, 2022 · 15 comments · Fixed by #423
Closed

maximum_color is unhappy with > 1 palette #421

rsheeter opened this issue Jul 28, 2022 · 15 comments · Fixed by #423

Comments

@rsheeter
Copy link
Collaborator

In a clone of google/fonts:

maximum_color ofl/reemkufiink/ReemKufiInk-Regular.ttf

nanoemoji/colr_to_svg.py", line 410, in colr_to_svg
    assert len(ttfont["CPAL"].palettes) == 1, "We assume one palette"
AssertionError: We assume one palette

This is far less helpful than building a functional font with only one - presumably the first - palette.

@anthrotype
Copy link
Member

anthrotype commented Jul 28, 2022

hm no good.

(LOL how the line just above that says """For testing only, don't use for real!""")

To fully support this we need #422

@anthrotype
Copy link
Member

building a functional font with only one - presumably the first - palette.

That's trivial, we can maybe do that as first round

@rsheeter
Copy link
Collaborator Author

I like the idea of doing the trivial fix first :)

@anthrotype
Copy link
Member

if I apply the following patch, I can get past that assertion and the subsequent code will use font["CPAL"].palettes[0]

diff --git a/src/nanoemoji/colr_to_svg.py b/src/nanoemoji/colr_to_svg.py
index 9c514e9..4778558 100644
--- a/src/nanoemoji/colr_to_svg.py
+++ b/src/nanoemoji/colr_to_svg.py
@@ -407,7 +407,13 @@ def colr_to_svg(
     rounding_ndigits: Optional[int] = None,
 ) -> Dict[str, SVG]:
     """For testing only, don't use for real!"""
-    assert len(ttfont["CPAL"].palettes) == 1, "We assume one palette"
+    num_palettes = len(ttfont["CPAL"].palettes)
+    if num_palettes > 1:
+        logging.warning(
+            "Multiple CPAL palettes are not supported! Only using the first"
+        )
+    elif num_palettes < 1:
+        raise ValueError("No CPAL palettes found, at least one required")

     colr_version = ttfont["COLR"].version
     if colr_version == 0:

But... then I hit a new assertion error because some glyphs have zero advance width (which is normal and expected for glyphs used as combining marks).

W0729 15:25:50.514565 4340401536 colr_to_svg.py:412] Multiple CPAL palettes are not supported! Only using the first
Traceback (most recent call last):
...
  File "/Users/clupo/Github/nanoemoji/src/nanoemoji/colr_to_svg.py", line 329, in _view_box_and_transform
    view_box = view_box_callback(glyph_name)
  File "/Users/clupo/Github/nanoemoji/src/nanoemoji/generate_svgs_from_colr.py", line 56, in <lambda>
    for glyph_name, svg in colr_to_svg(lambda gn: _view_box(font, gn), font).items():
  File "/Users/clupo/Github/nanoemoji/src/nanoemoji/generate_svgs_from_colr.py", line 41, in _view_box
    assert region.w > 0, f"0-width region for {glyph_name}"
AssertionError: 0-width region for behDotless-ar.init.Hah

I forgot why we do that, but we should support converting glyphs with 0 advance somehow

@anthrotype
Copy link
Member

there's at least 3 places that assert that width be 0 in the code that converts COLR => SVG

$ git grep "assert.*0-width"
src/nanoemoji/colr_to_svg.py:330:    assert view_box.w > 0, f"0-width viewBox for {glyph_name}?!"
src/nanoemoji/colr_to_svg.py:333:    assert region.w > 0, f"0-width region for {glyph_name}?!"
src/nanoemoji/generate_svgs_from_colr.py:41:    assert region.w > 0, f"0-width region for {glyph_name}"

@anthrotype
Copy link
Member

if I comment-out all three of those asserts, I can run maximum_color tool on ReemKufiInk-Regular.ttf without crashing.

Now, I'll see whether the 0-width glyphs display correctly, and most importantly why those assertions are there in the first place.

@anthrotype
Copy link
Member

hm, it looks like we had removed a similar assertion about width==0 in #412

in that conversation, it appeared these asserts have something to do with bitmap conversion..

Is it because when width is 0, a bitmap CBDT glyph would otherwise completely disappear

I think this was true but iirc should no longer be. In any case, definitely a bad assert, good catch.

@anthrotype
Copy link
Member

anthrotype commented Jul 29, 2022

Would be nice to summon @khaledhosny to confirm that we haven't crippled his Arabic font.
If I understand correctly, one way to activate that "behDotless-ar.init.Hah" glyph (which has 0 advance width) is to write a text like this "ٮح"

‎066E ARABIC LETTER DOTLESS BEH
‎062D ARABIC LETTER HAH

$ hb-shape /tmp/reemkufiink/Font.ttf --unicodes=066E,062D
[gid83=1+515|gid43=0@80,0+80]

This is how the COLRv1 looks in Chrome 103 on macOS:

Screen Shot 2022-07-29 at 16 35 10

And the OT-SVG font (as converted by maximum_color tool after removing the above-mentioned assertions) appears like this in FireFox 102 (Safari 15.5 also using OT-SVG table appears the same as FireFox)

Screen Shot 2022-07-29 at 16 46 21

I don't know Arabic, so I can't tell which one is the correct one (even though "visually" the second one [FireFox & Safari's] looks better to me as the letter shapes don't overlap but again I'm not arabic expert)

@khaledhosny
Copy link

The COLRv1 rendering is the correct one, the two glyphs should be attached. The font does not use cursive anchors to attach the two glyphs, but relied on the behDotless-ar.init.Hah having a -ve left side bearing.

@anthrotype
Copy link
Member

The first one (COLRv1 on Chrome) seems to be the correct rendering, because I tried to render the same string with the black-and-white ReemKufi.otf and all three browsers agree on this:

Screen Shot 2022-07-29 at 16 51 43

Therefore something must have gone wrong in the COLR=>OT-SVG conversion

@anthrotype
Copy link
Member

@khaledhosny thanks for confirming!

@anthrotype
Copy link
Member

Ha! After commenting out another couple of lines I can make the ReemKufiInk OT-SVG work with the 0-width glyphs:

diff --git a/src/nanoemoji/colr_to_svg.py b/src/nanoemoji/colr_to_svg.py
index 9c514e9..903fe9c 100644
--- a/src/nanoemoji/colr_to_svg.py
+++ b/src/nanoemoji/colr_to_svg.py
@@ -312,8 +312,8 @@ def glyph_region(ttfont: ttLib.TTFont, glyph_name: str) -> Rect:

     map_font_space_to_viewbox handles font +y goes up => svg +y goes down."""
     width = ttfont["hmtx"][glyph_name][0]
-    if width == 0:
-        width = ttfont["glyf"][glyph_name].xMax
+    # if width == 0:
+    #     width = ttfont["glyf"][glyph_name].xMax
     return Rect(
         0,
         -ttfont["OS/2"].sTypoAscender,

it now looks correct both in FireFox and Safari (the latter shown below)

Screen Shot 2022-07-29 at 16 51 43

I think all these hacks were put in place in order to be able to not chop off CBDT glyphs when we rasterize SVG=>PNG (this bug basically #402)

We should either get rid of them of put them behind a flag somehow

@anthrotype
Copy link
Member

anthrotype commented Jul 29, 2022

attempting to maximum_color "CairoPlay[slnt,wght].ttf" fails with

  File "/Users/clupo/Github/nanoemoji/src/nanoemoji/generate_svgs_from_colr.py", line 40, in _view_box
    region = glyph_region(font, glyph_name)
  File "/Users/clupo/Github/nanoemoji/src/nanoemoji/colr_to_svg.py", line 316, in glyph_region
    width = ttfont["glyf"][glyph_name].xMax
AttributeError: 'Glyph' object has no attribute 'xMax'

Yet one more reason to get rid of those lines.

The error is probably because the glyph is not only 0-width but also has 0 contours (it's a whitespace glyph), so has no bounding box attributes

Conversion works after #423

anthrotype added a commit that referenced this issue Aug 17, 2022
this will make zero-width glyphs disappear when rasterized via resvg to PNG, because anything outside viewbox gets clipped (#402). Thus, running maximum_color --bitmaps with a font that has such zero-width glyphs will produce invisible empty CBDT/sbix glyphs until that issue is resolved.
However, removing this assertions allows us to support zero-width glyphs in vector color formats like OT-SVG/COLR (cf. #421)
@anthrotype
Copy link
Member

anthrotype commented Aug 18, 2022

running maximum_color on ReemKufiInk-Regular using #423 branch (in turn, based on #424) completes successfully and the "ٮح" string containing zero-width glyph displays the same on both Chrome (COLRv1) and FireFox/Safari (OT-SVG)

@anthrotype
Copy link
Member

I suggest we go on and merge those two PRs, cut a release and use the updated nanoemoji to onboard the COLRv1 fonts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants