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

WIP: Add "scaleUPM" filter #436

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jenskutilek
Copy link
Collaborator

This filter scales the font to a target upm value. Set the target upm in an UFO like this:

    <key>com.github.googlei18n.ufo2ft.filters</key>
    <array>
        <dict>
            <key>name</key>
            <string>scaleUPM</string>
            <key>kwargs</key>
            <dict>
                <key>unitsPerEm</key>
                <string>2048</string>
            </dict>
        </dict>
    </array>

Scaling values in feature code is not supported yet.

@jenskutilek jenskutilek marked this pull request as draft December 1, 2020 11:43
@anthrotype
Copy link
Member

I wonder if you came across this gist that I circulated sometime ago when @chrissimpkins
asked me about this https://gist.github.com/anthrotype/62d0bfe1d38b8f11a199bd3b66574bcc

@jenskutilek
Copy link
Collaborator Author

Ha! No, I didn't ... I adapted a RoboFont scaling script that I had.

@anthrotype
Copy link
Member

can you comment on why it would be desirable to be able to change UPEM at build time? It's non-obvious to me

@jenskutilek
Copy link
Collaborator Author

For TrueType output, a higher UPEM may be desirable, especially for variable fonts in order to minimize rounding errors in interpolation.

E.g. a nominal stem of 80 may round to 79, 80, or 81 depending on whether both or one of the related nodes round up or down. In relation to 80 units, this is an error of 2.5%. When scaling to double UPEM, the error is reduced to 2/160, or 1.25% and so on.

@jenskutilek
Copy link
Collaborator Author

Glyphs also has a custom parameter "Scale to UPM" for scaling on export, though it is unsupported by glyphsLib and Glyphs' own UFO export currently.

@anthrotype
Copy link
Member

to minimize rounding errors in interpolation.

you are referring to this fonttools/fontbakery#2971

I thought we had marked that as "dogma" since noone could provide evidence of this making any visible differences in real world fonts. At least I still haven't seen with my own eyes.

@anthrotype
Copy link
Member

Glyphs also has a custom parameter "Scale to UPM" for scaling on export

that could be an argument to add support here, provided there's user demand.

@anthrotype
Copy link
Member

anthrotype commented Dec 1, 2020

also, about the alleged rounding errors, I think you only see those when you cut a static instance from a VF, where you have to round to be able to encode the glyf table coordinates. But for VF instances interpolated at runtime, I believe coordinates don't get rounded to integers by the renderers. I'd be surprised of the contrary.

@jenskutilek
Copy link
Collaborator Author

also, about the alleged rounding errors, I think you only see those when you cut a static instance from a VF, where you have to round to be able to encode the glyf table coordinates. But for VF instances interpolated at runtime, I believe coordinates don't get rounded to integers by the renderers. I'd be surprised of the contrary.

I had missed the discussion you linked above. I kind of assumed that VF instances would always be "normal" rounded TTFs. At least they have to be when they are embedded in PDFs ...? When I convert a VF to outlines in InDesign, all original points are also rounded to integer (the former implicit oncurve points may fall on .5 positions).

@Lorp
Copy link

Lorp commented Dec 2, 2020

Rounding of instance point coordinates is implicit in the spec. Take a look at OpenType Font Variations Overview: Interpolation Example. Point 0 moves from original coordinates (621,512) with a delta of (162.3,-28.4) to final coordinates (783,484). Although in-memory instances could in theory ignore this rounding, different rounding behaviour from the equivalent static TrueType font seems undesirable so I would be surprised if implementations returned non-integral instance outlines.

BTW Samsa offers a non-rounding mode for returning instance coordinates (pass {roundDeltas:false} to the extra parameter in SamsaGlyph.instantiate()). Unrounded values are essential for plotting loci without obvious stair-stepping.

@Lorp
Copy link

Lorp commented Dec 2, 2020

It would be a useful improvement to the spec to clarify exactly when coordinate rounding takes place. The phrase “It is recommended … that any rounding be done at the last point before a value is used” is not clear enough.

Specifically in the section Algorithm for Interpolation of Instance Values, the pseudocode should specify whether netAdjustment:

  • is rounded every interation of the loop,
  • is rounded once after the loop,
  • is not necessarily rounded at all.

/cc @PeterConstable

@PeterCon
Copy link

PeterCon commented Dec 2, 2020

See earlier on the page:

When processing variation instance coordinates and variation data, the amount of precision used and the handling of rounding can potentially have noticeable impacts on visual results. In order to ensure consistent behavior for a given font across implementations, implementations must observe the following requirements in relation to precision and rounding...

Does that provide the details you're looking for?

@Lorp
Copy link

Lorp commented Dec 2, 2020

Not really. In that section, “In order to ensure consistent behavior for a given font across implementations” introduces a list of 7 musts. Yet in point 7, we read a recommendation that “any rounding be done at the last point before a value is used”. It is not specified which rounding this might be, and indeed it implies rounding is optional, conflicting with the consistency requirement stated at the outset. Rounding of scaled deltas is implemented in the example, but that doesn’t by itself make it mandatory.

@PeterCon
Copy link

PeterCon commented Dec 2, 2020

Please open an issue using the feedback link on that page. If you have a suggestion for what changes are needed, please mention that.

@Lorp
Copy link

Lorp commented Dec 2, 2020

Will do.

@anthrotype
Copy link
Member

Base automatically changed from master to main March 1, 2021 09:01
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.

4 participants