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

yMax and usWinAscent calculation error #105

Closed
yisibl opened this issue May 9, 2021 · 31 comments
Closed

yMax and usWinAscent calculation error #105

yisibl opened this issue May 9, 2021 · 31 comments

Comments

@yisibl
Copy link
Contributor

yisibl commented May 9, 2021

This will cause the line height in Windows to be too large. thx/iconfont-plus#1953 (comment)

image

I guess it is a problem introduced by(v2.1.0) #25 , and there is a problem with the conversion of the cubic2quad module.

In the result returned by cubic2quad, 32147.26312251112 is abnormal.

[ 'Q', 566.970047, 32147.26312251112, 567.27723, 693.5669409999996 ]

svg2ttf/lib/svg.js

Lines 96 to 111 in df30be9

function cubicToQuad(segment, index, x, y) {
if (segment[0] === 'C') {
var quadCurves = math.bezierCubicToQuad(
new math.Point(x, y),
new math.Point(segment[1], segment[2]),
new math.Point(segment[3], segment[4]),
new math.Point(segment[5], segment[6]),
0.3
);
var res = [];
_.forEach(quadCurves, function(curve) {
res.push(['Q', curve[1].x, curve[1].y, curve[2].x, curve[2].y]);
});
return res;
}

Test file and test case in:

ae425be

<?xml version="1.0" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" >
<svg>
    <metadata>
        iconfont
    </metadata>
    <defs>
        <font id="iconfont" horiz-adv-x="1024">
            <font-face font-family="iconfont" font-weight="500" font-stretch="normal" units-per-em="1024" ascent="896" descent="-128" />
            <missing-glyph />
            <glyph glyph-name="up" unicode="&#58880;" d="M566.970047-68.244874V693.362153c0 0.204788 0.307182 0.307182 0.307183 0.204788l259.773733-287.42013c13.61841-14.949533 33.79004-21.195571 52.835335-16.178261a56.521522 56.521522 0 0 1 39.524108 40.548048 61.02686 61.02686 0 0 1-13.208834 56.623916L552.634879 878.285827A53.654488 53.654488 0 0 1 513.110771 896a53.654488 53.654488 0 0 1-39.524107-17.816567L117.767301 485.092633A60.207708 60.207708 0 0 1 102.408192 443.41825a59.798132 59.798132 0 0 1 16.89502-40.957625c21.912329-22.219511 56.521522-21.502753 77.614698 1.638305L456.691644 691.314272h0.307182v-760.787875c0-31.230189 22.833876-56.521522 51.401818-58.364614 15.359109-0.819152 30.308642 5.119703 41.367201 16.383049a61.231649 61.231649 0 0 1 17.099808 43.210294z" horiz-adv-x="1024" />
        </font>
    </defs>
</svg>

TTX file

<?xml version="1.0" encoding="UTF-8"?>
<ttFont sfntVersion="\x00\x01\x00\x00" ttLibVersion="4.22">
  <GlyphOrder>
    <GlyphID id="0" name="glyph00000"/>
    <GlyphID id="1" name="up"/>
  </GlyphOrder>

  <head>
    <tableVersion value="1.0"/>
    <fontRevision value="1.0"/>
    <checkSumAdjustment value="0xd03dc53e"/>
    <magicNumber value="0x5f0f3cf5"/>
    <flags value="00000000 00001011"/>
    <unitsPerEm value="1024"/>
    <created value="Sat May  8 11:35:08 2021"/>
    <modified value="Sat May  8 11:35:08 2021"/>
    <xMin value="0"/>
    <yMin value="-130"/>
    <xMax value="1024"/>
    <yMax value="32148"/>
    <macStyle value="00000000 00000000"/>
    <lowestRecPPEM value="8"/>
    <fontDirectionHint value="2"/>
    <indexToLocFormat value="0"/>
    <glyphDataFormat value="0"/>
  </head>

  <hhea>
    <tableVersion value="0x00010000"/>
    <ascent value="896"/>
    <descent value="-128"/>
    <lineGap value="0"/>
    <advanceWidthMax value="1024"/>
    <minLeftSideBearing value="0"/>
    <minRightSideBearing value="0"/>
    <xMaxExtent value="1024"/>
    <caretSlopeRise value="1"/>
    <caretSlopeRun value="0"/>
    <caretOffset value="0"/>
    <reserved0 value="0"/>
    <reserved1 value="0"/>
    <reserved2 value="0"/>
    <reserved3 value="0"/>
    <metricDataFormat value="0"/>
    <numberOfHMetrics value="2"/>
  </hhea>

  <OS_2>
    <version value="1"/>
    <xAvgCharWidth value="1024"/>
    <usWeightClass value="400"/>
    <usWidthClass value="5"/>
    <fsType value="00000000 00000000"/>
    <ySubscriptXSize value="649"/>
    <ySubscriptYSize value="716"/>
    <ySubscriptXOffset value="0"/>
    <ySubscriptYOffset value="143"/>
    <ySuperscriptXSize value="649"/>
    <ySuperscriptYSize value="716"/>
    <ySuperscriptXOffset value="0"/>
    <ySuperscriptYOffset value="491"/>
    <yStrikeoutSize value="50"/>
    <yStrikeoutPosition value="264"/>
    <sFamilyClass value="0"/>
    <panose>
      <bFamilyType value="2"/>
      <bSerifStyle value="0"/>
      <bWeight value="5"/>
      <bProportion value="3"/>
      <bContrast value="0"/>
      <bStrokeVariation value="0"/>
      <bArmStyle value="0"/>
      <bLetterForm value="0"/>
      <bMidline value="0"/>
      <bXHeight value="0"/>
    </panose>
    <ulUnicodeRange1 value="00000000 00000000 00000000 00000000"/>
    <ulUnicodeRange2 value="00000000 00000000 00000000 00000000"/>
    <ulUnicodeRange3 value="00000000 00000000 00000000 00000000"/>
    <ulUnicodeRange4 value="00000000 00000000 00000000 00000000"/>
    <achVendID value="PfEd"/>
    <fsSelection value="00000000 01000000"/>
    <usFirstCharIndex value="58880"/>
    <usLastCharIndex value="58880"/>
    <sTypoAscender value="896"/>
    <sTypoDescender value="-128"/>
    <sTypoLineGap value="92"/>
    <usWinAscent value="32148"/>
    <usWinDescent value="130"/>
    <ulCodePageRange1 value="00000000 00000000 00000000 00000001"/>
    <ulCodePageRange2 value="00000000 00000000 00000000 00000000"/>
  </OS_2>
  <glyf>
    <TTGlyph name="glyph00000"/><!-- contains no outline data -->
    <TTGlyph name="up" xMin="0" yMin="-130" xMax="924" yMax="32148">
      <contour>
        <pt x="567" y="-68" on="1"/>
        <pt x="567" y="693" on="1"/>
        <pt x="567" y="32147" on="0"/>
        <pt x="567" y="694" on="1"/>
        <pt x="827" y="406" on="1"/>
        <pt x="837" y="395" on="0"/>
        <pt x="866" y="386" on="0"/>
        <pt x="894" y="394" on="0"/>
        <pt x="916" y="416" on="0"/>
        <pt x="923" y="445" on="0"/>
        <pt x="916" y="476" on="0"/>
        <pt x="906" y="487" on="1"/>
        <pt x="553" y="878" on="1"/>
        <pt x="537" y="896" on="0"/>
        <pt x="489" y="896" on="0"/>
        <pt x="474" y="878" on="1"/>
        <pt x="118" y="485" on="1"/>
        <pt x="102" y="467" on="0"/>
        <pt x="103" y="420" on="0"/>
        <pt x="136" y="386" on="0"/>
        <pt x="181" y="387" on="0"/>
        <pt x="197" y="404" on="1"/>
        <pt x="457" y="691" on="1"/>
        <pt x="457" y="-69" on="1"/>
        <pt x="457" y="-93" on="0"/>
        <pt x="487" y="-126" on="0"/>
        <pt x="508" y="-128" on="1"/>
        <pt x="532" y="-129" on="0"/>
        <pt x="567" y="-93" on="0"/>
      </contour>
      <instructions/>
    </TTGlyph>
  </glyf>
</ttFont>
@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

I found that if the accuracy is greater than 0.153, there will be problems. Can we use a smaller accuracy?

svg2ttf/index.js

Lines 167 to 171 in d8a4262

// Calculate accuracy for cubicToQuad transformation
// For glyphs with height and width smaller than 500 use relative 0.06% accuracy,
// for larger glyphs use fixed accuracy 0.3.
var glyphSize = Math.max(glyph.width, glyph.height);
var accuracy = (glyphSize > 500) ? 0.3 : glyphSize * 0.0006;

@puzrin
Copy link
Member

puzrin commented May 9, 2021

I found that if the accuracy is greater than 0.153, there will be problems. Can we use a smaller accuracy?

There are no clear criteria how to select accuracy. Without criteria, this is "magical number". Tuning magical numbers is not good idea.

I'd suggest 2 alternate approaches:

  1. Update source file, if possible (split that bad curve manually to 2-4 segments)
  2. Increase resolution from 1000px to 2000px or 4000px

Increasing existing accuracy demand will cause increase of segments (=> font size) and is not nice.

@puzrin
Copy link
Member

puzrin commented May 9, 2021

Or... if you know better accuracy criteria - that's discussable. I'm sure, better algorythm possible, but had no time to investigate.

I mean, visual criteria is ~ obvious. Visual error should be < 0.5px for 1000*1000px bounding box. But turning that into math metrics is not so easy.

@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

Are there any side effects of smaller accuracy? I cannot change the SVG source files, because these are uploaded by other users. There should be many similar SVG files on our platform, and many of them are historical data.

@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

Maybe we can change the calculation method of usWinAscent, in nanoemoji os2.usWinAscent = ascender + linegap, yMax is not currently considered.

In terms of vertical measurement, do icons need to consider clipping?

    # These are ufo2ft's fallback WinAscent/WinDescent, good enough for now.
    # https://github.com/googlefonts/ufo2ft/blob/a5267d135d5a8dba7e6a5d3ac44139afa6159429/Lib/ufo2ft/fontInfoData.py#L241-L247
    # TODO: Set to the actual global yMin/yMax to prevent any clipping?
    assert os2.usWinAscent == ascender + linegap
    assert os2.usWinDescent == abs(descender)  # always positive

This is svg2ttf:

buf.writeInt16(font.ascent); // sTypoAscender
buf.writeInt16(font.descent); // sTypoDescender
buf.writeInt16(font.lineGap); // lineGap
// Enlarge win acscent/descent to avoid clipping
buf.writeInt16(Math.max(font.yMax, font.ascent)); // usWinAscent
buf.writeInt16(-Math.min(font.yMin, font.descent)); // usWinDescent

In addition, cu2qu uses 0.001 as the default approximation error: googlefonts/cu2qu#76

@anthrotype is an expert in fonts, I hope he can provide some help.

@puzrin
Copy link
Member

puzrin commented May 9, 2021

Are there any side effects of smaller accuracy?

Yes. Smaller accuracy => more splits => higher font size. Problem is, everybody will be affected, and you will not give any guarantees. It will be still possible to draw crazy arc or third-order bezier curve to break everything.

Ideally, "accuracy" should depend on concrete segment.

I cannot change the SVG source files, because these are uploaded by other users. There should be many similar SVG files on our platform, and many of them are historical data.

If you don't care about font size, you can set TTF glyph height to ~ 4000 instead of 500-1000 as you have now. Result will be similar as accuracy value change. You can scale your src by svgpath without loss of precision.


I'm open to any alternate suggestions with strong math background. It would be nice to drop use of current "magical values".

@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

This is the original SVG file. Is it because the size is too small (8x10)? If so, can we use different accuracy number when the SVG size is small?

<svg width="8" height="10" viewBox="0 0 8 10" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <g fill="none" fill-rule="evenodd">
        <path d="M4.537 9.417V1.979c0-.002.003-.003.003-.002l2.537 2.807c.133.146.33.207.516.158a.552.552 0 0 0 .386-.396.596.596 0 0 0-.129-.553L4.397.173A.524.524 0 0 0 4.011 0a.524.524 0 0 0-.386.174L.15 4.013A.588.588 0 0 0 0 4.42a.584.584 0 0 0 .165.4c.214.217.552.21.758-.016L3.46 1.999h.003v7.43c0 .305.223.552.502.57.15.008.296-.05.404-.16a.598.598 0 0 0 .167-.422z" fill="#41D2D9" />
    </g>
</svg>

@yisibl
Copy link
Contributor Author

yisibl commented May 9, 2021

Another idea is that we can get the SVG BoundingBox, calculate the height, and then make usWinAscent not greater than the BoundingBox's height. Is this feasible?

@puzrin
Copy link
Member

puzrin commented May 9, 2021

This is the original SVG file. Is it because the size is too small (8x10)? If so, can we use different accuracy number when the SVG size is small?

Since font glyph usually are 500*500...4000*4000 int px, better approach would be to scale up your image BEFORE converting to second order bezier and rounding. Use svgpath for that.

Another idea is that we can get the SVG BoundingBox, calculate the height, and then make usWinAscent not greater than the BoundingBox's height. Is this feasible?

Problem is not with bounding box, but with processing of third order bezier & arcs. It's technically possible write those such way to break any static limit (then, after rounding you will get something bad).

@yisibl
Copy link
Contributor Author

yisibl commented May 10, 2021

Found a new issue, this font cannot be displayed in Mac Chrome and Firefox. https://at.alicdn.com/t/project/37546/21049af9-0257-43a6-8c7e-2fd08733737d.html

Chrome DevTools says: OTS parsing error: head: Failed to parse table

@puzrin
Copy link
Member

puzrin commented May 10, 2021

@yisibl if that's not related to current issue, please fill new one. With minimalistic sample how to reproduce (source SVG + reduce glyphs count as much as possible). You provided only final result, but without proof why this is caused by svg2ttf (i don't decline, but there may be multiple reasons of broken fonts).

PS. FF shows more concrete error: downloadable font: head: Bad x dimension in the font bounding box (-39, -13440) (font-family: "iconfont" style:normal weight:400 stretch:100 src index:2) source: https://at.alicdn.com/t/font_37546_hgtmiiq9vft.woff?t=1620616818531

@yisibl
Copy link
Contributor Author

yisibl commented May 14, 2021

@yisibl if that's not related to current issue, please fill new one. With minimalistic sample how to reproduce (source SVG + reduce glyphs count as much as possible). You provided only final result, but without proof why this is caused by svg2ttf (i don't decline, but there may be multiple reasons of broken fonts).

PS. FF shows more concrete error: downloadable font: head: Bad x dimension in the font bounding box (-39, -13440) (font-family: "iconfont" style:normal weight:400 stretch:100 src index:2) source: https://at.alicdn.com/t/font_37546_hgtmiiq9vft.woff?t=1620616818531

I have tried some fixes here. If the above problem cannot be solved, I will reopen an issue. 46c0df2

In any case, too large usWinAscent and usWinDescent is definitely a problem.

@yisibl
Copy link
Contributor Author

yisibl commented May 14, 2021

fontbakery check-notofonts iconfont.ttf output:

Rationale:
A font's winAscent and winDescent values should be greater than the head
table's yMax, abs(yMin) values. If they are less than these values,
clipping can occur on Windows platforms
(RedHatOfficial/Overpass#33).

  If the font includes tall/deep writing systems such as Arabic or
  Devanagari, the winAscent and winDescent can be greater than the yMax and
  abs(yMin) to accommodate vowel marks.

  When the win Metrics are significantly greater than the upm, the
  linespacing can appear too loose. To counteract this, enabling the OS/2
  fsSelection bit 7 (Use_Typo_Metrics), will force Windows to use the OS/2
  typo values instead. This means the font developer can control the
  linespacing with the typo values, whilst avoiding clipping by setting the
  win values to values greater than the yMax and abs(yMin).

FAIL: OS/2.usWinDescent value 38262 is too large. It should be less than double the yMin. Current absolute yMin value is 727 [code: descent]
FAIL: ots-sanitize returned an error code (1). Output follows:
ERROR: head: Bad x dimension in the font bounding box (-39, -13440)
ERROR: head: Failed to parse table
Failed to sanitize file!

@yisibl
Copy link
Contributor Author

yisibl commented May 14, 2021

Some users have encountered similar problems a long time ago: thx/iconfont-plus#661

@rlidwka
Copy link
Member

rlidwka commented May 17, 2021

In the result returned by cubic2quad, 32147.26312251112 is abnormal.
[ 'Q', 566.970047, 32147.26312251112, 567.27723, 693.5669409999996 ]

That is an error in cubic2quad error detection algorithm. Right over here:

https://github.com/fontello/cubic2quad/blob/master/index.js#L198-L199

We check every(ish) point on the cubic curve to match quadratic. We do not check any point on the quadratic curve to match cubic.

The problem is: if middle point of cubic curve is within error from its origin, approximation will be good no matter what (quadratic control point may be on the moon for all it cares).

Nice picture here:

image

Your SVG file has all 4 cubic curve points very close together, so the difference between them falls well within the accuracy.

console.log(require('cubic2quad')(566.970047,693.362153,566.970047,693.566941,567.277229,693.669335,567.27723,693.566941,1))

@rlidwka
Copy link
Member

rlidwka commented May 17, 2021

Here's the exact curve:

  • red - cubic source (black dots - 2 control points of the cubic curve)
  • blue - quadratic approximation (1 control point is off the screen)

image

<!DOCTYPE html>
<html>
<body>

<svg height="600" width="600" viewBox="566.5 693 1 2">
    <circle id="pointA" cx="566.970047" cy="693.566941" r="0.01" />
    <circle id="pointB" cx="567.277229" cy="693.669335" r="0.01" />  </g>

  <path d="M 566.970047 693.362153 C 566.970047 693.566941 567.277229 693.669335 567.27723 693.566941" stroke="red" stroke-width="0.005" fill="none" />
  <path d="M 566.970047 693.362153 Q 566.970047 32147.26312251112 567.27723 693.5669409999996" stroke="blue" stroke-width="0.005" fill="none" />
</svg>

</body>
</html>

try here: https://www.w3schools.com/graphics/tryit.asp?filename=trysvg_path2

@rlidwka
Copy link
Member

rlidwka commented May 19, 2021

fixed in fontello/cubic2quad@44c53ad

@yisibl
Copy link
Contributor Author

yisibl commented May 19, 2021

fixed in fontello/cubic2quad@44c53ad

Awesome, thank you for your amazing work!

@yisibl
Copy link
Contributor Author

yisibl commented May 20, 2021

I update a test case that caused the error:

head.yMin = 27274
head.xMax = -13440

shenheweitongguo.svg.zip

image

@rlidwka
Copy link
Member

rlidwka commented May 20, 2021

I update a test case that caused the error:

What is this test for? Does it still show any issues after cubic2quad update?

@yisibl
Copy link
Contributor Author

yisibl commented May 20, 2021

@rlidwka This is used to test the version of cubic2quad before 1.2.0. see also: https://github.com/yisibl/svg2ttf/pull/1/files#diff-a561630bb56b82342bc66697aee2ad96efddcbc9d150665abd6fb7ecb7c0ab2fR246-R260

svg2ttf must have more test cases based on font tables.

@yisibl
Copy link
Contributor Author

yisibl commented May 20, 2021

I have deployed to pre-release for testing(usWinAscent/usWinDescent does not depend on the value of yMax/yMin), and it seems to be completely resolved. The generated ttf file is also smaller.

before: https://at.alicdn.com/t/project/37546/21049af9-0257-43a6-8c7e-2fd08733737d.html
after: https://at.alicdn.com/t/project/37546/f49c07e1-448c-4e92-80a0-7544b91a29e8.html

image

@puzrin
Copy link
Member

puzrin commented May 20, 2021

Can we close this?

@yisibl
Copy link
Contributor Author

yisibl commented May 21, 2021

Can we close this?

Need to wait, please let me test it online for a few days.

I encountered a new failed test case: yisibl@56b2736

@rlidwka PTAL, thanks!

@rlidwka
Copy link
Member

rlidwka commented May 21, 2021

@yisibl in your example glyph has the following instruction near the end of the path:

a86173505653221920 86173505653221920 0 0 1 34.50966791-4.60128904

Does this arc have a good reason to be there? Looks like an error in the test file.

@yisibl
Copy link
Contributor Author

yisibl commented May 26, 2021

@yisibl in your example glyph has the following instruction near the end of the path:

a86173505653221920 86173505653221920 0 0 1 34.50966791-4.60128904

Does this arc have a good reason to be there? Looks like an error in the test file.

Yes, we have some icons with very strange paths. This causes the generated font to fail the ots check, and an error will be reported in Chrome.

I think for this kind of SVG should throw an error.

Here is another example: https://at.alicdn.com/t/project/2567398/0794db68-7c83-4d28-8e2f-0dc819833051.html
Test SVG font in: https://github.com/yisibl/svg2ttf/pull/2/files#diff-3ec56eb2d7d4c0768bcdb3b4db2508775c2efb3cfb976c7f09f8618aa2ab291d

@puzrin
Copy link
Member

puzrin commented May 26, 2021

I think for this kind of SVG should throw an error.

I do not agree.

  1. That's a validation process, without strict criterias. Not related to font generation. Font generator just do what was ordered.
  2. This can be implemented separate, on app level. I would reconsider join only after this feature became stable (if that's technically possible at all).

Also, i'd suggest to focus on release, instead of "new features". Let's solve minimal mandatory set - svg2ttf should produce good result from good data. Is current master ok or something was missed?

@yisibl
Copy link
Contributor Author

yisibl commented May 27, 2021

@puzrin I think 5.3 can be released. Special SVG errors can wait for me to write test cases more concisely and concretely.

@puzrin
Copy link
Member

puzrin commented May 27, 2021

Thanks. I will release.

After thinking a bit, we could use some of chrome/mozilla criteria to check obvious (most annoying) artefacts:

  • how much yMin/Max should be out of default bounding box to cause error?

Can you test that somehow? I mean, create dummy svg with small square and move that square to up/down/left/right until chrome/mozilla fails?

@puzrin
Copy link
Member

puzrin commented May 27, 2021

Released 6.0.0, also added special thanks to changelog.

As far as i understand, we have 3 pending issues with notable added value:

If nothing critical missed, I'd suggest consider close existing issues/prs as "too noisy" and create new ones (more organized) to discuss details.

@yisibl
Copy link
Contributor Author

yisibl commented Jun 18, 2021

Thank you very much for your summary, let us discuss it in a separate issue.

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

No branches or pull requests

3 participants