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

[varLib] Add Feature Variations #1240

Merged
merged 12 commits into from
Jun 14, 2018
Merged

[varLib] Add Feature Variations #1240

merged 12 commits into from
Jun 14, 2018

Conversation

justvanrossum
Copy link
Collaborator

@justvanrossum justvanrossum commented Apr 16, 2018

This module contains a function that adds conditional substitutions to a variable font. It's pretty much an afterburner on an otherwise ready variable font.

Brief example:

>>> f = TTFont(srcPath)
>>> condSubst = [
...     # A list of (Region, Substitution) tuples.
...     ([{"wght": (0.5, 1.0)}], {"dollar": "dollar.rvrn"}),
...     ([{"wdth": (0.5, 1.0)}], {"cent": "cent.rvrn"}),
... ]
>>> addFeatureVariations(f, condSubst)
>>> f.save(dstPath)

Currently, the minimum and maximum values for a region are specified in user raw design coordinates, and are therefore normalized without going through avar. That makes most sense to me from a designers perspective. I can imagine to add a flag that would allow those values to be specified in normalized coordinates.

I have a test script based on existing test data within the fonttools test suite, that needs to be rewritten into proper tests. I may need help with that.
addFeatureVariationsTest.py.zip

@anthrotype
Copy link
Member

Thank you Just!

Currently, the minimum and maximum values for a region are specified in user coordinates, and are internally normalized without going through avar

the way currently varLib does the coordinate normalization is with a private class _DesignspaceAxis and its map_forward method (for going from user-space to internal designspace locations, whereas map_backward does the reverse). However, those should probably be moved to the designspaceLib.AxisDescriptor class once varLib makes use of it.

That makes most sense to me from a designers perspective. I can imagine to add a flag that would allow those values to be specified in normalized coordinates.

yes, a mapped=False argument like the one varLib.interpolate_layout uses, that when it's True it's assumed that the locations are in designspace internal coordinates and thus we don't "map_forward".

I'll complete my review soon, and also have a look at the test you provided.

@anthrotype
Copy link
Member

@justvanrossum I just added the avar-modified normalization with 081ca13, PTAL.
you mind if I push to the same PR branch?

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Apr 17, 2018

I'm not sure if going through avar is actually desirable here. Because avar mapping tends to be done later, to satisfy other requirements (I saw several examples during TYPO Labs), yet the point at which a glyph would switch from one form to the next should not change when such avar modifications are made. In the light of that it may be desirable to use normalized coordinates all the way.

@justvanrossum
Copy link
Collaborator Author

Curious about @LettError's opinion about this.

@justvanrossum
Copy link
Collaborator Author

I see this API at the level of a designspace document. The master locations there are also in user coordinates, that will be normalized without going through avar.

@anthrotype
Copy link
Member

I believe this is not about being desirable or not. FeatureVariation tables contain normalized coordinates, and by that I conclude that the avar should be used (when present) for converting from user-scale coordinates (i.e. axes location in fvar) to normalized coordinates (-1 to 1).

In https://docs.microsoft.com/en-us/typography/opentype/spec/chapter2#featurevariations-table

During processing for a particular variation instance, a normalization process is applied that maps user values in the range defined within the fvar table to a normalized scale with a range from -1 to 1. The values specified in a format 1 condition table are expressed in terms of the normalized scale, and so can be any value from -1 to 1.

Also, in Chapter "Coordinate Scales and Normalization", https://docs.microsoft.com/en-us/typography/opentype/spec/otvaroverview, there is a mention of "if avar table is present..." (step 4), followed by

A normalized value in 2.14 representation must be obtained exactly as specified in steps 1–5 above. ... If a font has OpenType Layout tables in which FeatureVariation tables are used, this exact value must be used when comparing with axis range values specified in a condition table.

Where did you get the impression that "avar mapping tends to be done later, to satisfy other requirements"?

@anthrotype
Copy link
Member

Actually, the master (and instances) locations in the designspace currently are not in user coordinates ("user" coordinates being those encoded in the fvar, with which the user interacts with). They are in internal "design" coordinates.
See LettError/designSpaceDocument#16

the terminology is very confusing, and my head is spinning right now... 😕

@anthrotype
Copy link
Member

anthrotype commented Apr 17, 2018

oh, perhaps you mean the min/max values in the FeatureVariation tables should be specified using the internal design-space coordinates -- just like the master locations are, e.g. for weight, it could be the thickness of the stem, etc. --, rather than the user coordinates (e.g. 1-1000 scale for wght registered axis)?
In which case, I think you're right we should not need to modify the normalization using the avar, the default normalization (min is -1, default is 0, max is 1, the rest linearly interpolated) is sufficient.

@behdad am I correct?

@LettError
Copy link
Collaborator

LettError commented Apr 17, 2018

Yes, the terminology is confusing and we should clarify them. Here's what I think we're looking at:

  1. raw: the neutral is not necessarily on 0, maximum values are not necessarily on 1 or -1. This is design data, still under construction and that means we might not know where the regular is, or what the extremes will be. So there is no avar table. This type of data must be stored somewhere.
  2. normalized: all neutral, default values are 0, maximum values are on 1 or -1. The default master is known. The extremes are known.
  3. user: all coordinates transformed or mapped by avar or xvar.
  • Currently the designspace axis descriptor map (ref) values go between user and raw. But after normalization these values should map between user and normalized.
  • The designspace object should be able to do a normalization of raw data, so from 1 to 2.

I'm assuming master and instance locations will predate the avar map. This means that substitution glyphs will be drawn before the avar map can be determined, which would make it necessary to store those coordinates in normalized coordinates.

@anthrotype
Copy link
Member

Thanks Erik for clarifying the terms. So the way Just originally wrote this, the addFeatureVariations function was accepting the "raw" or designspace's internal coordinates, whereas I understood it to use "user" coordinates, thus requiring to perform the avar mapping when converting them to "normalized" coordinates.

@justvanrossum I'm going to revert my last commit

@anthrotype
Copy link
Member

anthrotype commented Apr 17, 2018

The way the designspace spec defines the "rule" element seems to fit very nicely with the way this new addFeatureVariations function expects its input to be.

For example, given this rules definition, with locations expressed in raw internal coordinates (which may or may not be different from the user coordinates when axis maps are present):

<rules>
    <rule name="named.rule.1">
        <condition minimum="155" maximum="190" name="weight" />
        <condition minimum="50" maximum="100" name="width" />
        <sub name="dollar" byname="dollar.alt"/>
    </rule>
</rules>

the compiler (varLib.build) would call the function like this:

addFeatureVariations(
    ttFont,
    [  # list of rules
        (  # each rule is a tuple containing a Region (list) and a Substitution (dict)
            # regions contain a list of Spaces, dict mapping axis tags to min/max values
            [{"wght": (155, 190)},  # equivalent to "condition" element in designspace document
             {"wdth": (50, 100)}],
            {"dollar": "dollar.alt"}  # same as the "sub" element inside the "rule" element
        )
    ]
)

I think we can implement it like that for now, since we already have it the designspace spec, and deal with the further complications (extend FEA syntax) later.

@justvanrossum
Copy link
Collaborator Author

Sorry for messing up the terminology. I indeed meant raw design coordinates.

@justvanrossum
Copy link
Collaborator Author

I think we can implement it like that for now, since we already have it the designspace spec, and deal with the further complications (extend FEA syntax) later.

That would be fantastic. Perhaps my little test case can just become a designspace-based test.

and rename getPermutations to iterAllCombinations.

It's not really permutations we are after here, but more combinations
of indexes sorted by decreasing length, from more specific to less
@anthrotype
Copy link
Member

anthrotype commented Apr 17, 2018

@justvanrossum one thing I still don't understand is why do you have to "explode" all the combinations of rules. Can't we simply assume that the user writes such list of rules in the logical order in which they are meant to be applied, and we add one FeatureVariationRecord for each rule?
I'm probably missing something.. something big.

@anthrotype
Copy link
Member

in an inline comment you explain

Since the FeatureVariations table will only ever match one rule at a time,
we will make new rules for all possible combinations of our input, so we
can indirectly support overlapping rules.

but I still don't get it. Well, it's ok -- I'll sleep over it.

@LettError
Copy link
Collaborator

LettError commented Apr 17, 2018

Suppose a rule X which substitutes a to a.rvrn. And another rule Y that substitutes b to b.rvrn, but with different min and max values.

| - - - - - X X X X X X - - - - - - |
| - - Y Y Y Y Y Y - - - - - - - - - |

For the area in which rules X and Y overlap there needs to be a separate lookup feature record, Z in which both a and b are active. This will give the impression that X and Y are both active.
| - - Y Y Y Z Z Z X X X - - - - - - |

But then in n-dimensional space.

@anthrotype
Copy link
Member

Makes sense now, thanks Erik!

@justvanrossum
Copy link
Collaborator Author

something big

Yeah, it took me quite a while myself to see that the format is really quite dumb, and that we need all this to have a decent level of expressiveness.

Erik's explanation is spot on. You need three rules for that example case.

@PeterCon
Copy link

Note that feature variations can potentially be used with features other than rvrn. Also note that in Erik's example, you don't necessarily need three lookups. The effect can be achieved in two lookups: one that does substitution for a, and one that does substitution for b. You would still need an extra feature table, though:
FT1: Lookup_a_default, Lookup_b_default
FT2: Lookup_a_x, Lookup_b_default
FT3: Lookup_a_x, Lookup_b_y
FT4: Lookup_a_default, Lookup_b_y

I don't know of a less-dumb way.

@justvanrossum
Copy link
Collaborator Author

Note that feature variations can potentially be used with features other than rvrn

Hm yes, but I'm not sure how to specify conditions in the context of a feature that has been defined in a fea file. Especially since there is no .fea syntax for feature variations yet.

At the same time, to achieve such effects, I think they could always be built on top of rvrn, in that rvrn should be processed first, and other features can respond to that.

@justvanrossum
Copy link
Collaborator Author

Also note that in Erik's example, you don't necessarily need three lookups. The effect can be achieved in two lookups

The implementation actually does that as far as I can see. It's indeed the feature records that need "exploding". Replace the word "lookup" in Erik's comment with "feature record" and it matches the actual situation I think.

@LettError
Copy link
Collaborator

@justvanrossum I edited the example.

@justvanrossum
Copy link
Collaborator Author

(Btw. where I wrote "dumb" I meant "low level". It was certainly not meant as a judgment of the subtable format.)

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Apr 17, 2018

After writing #1243, I realized that "raw design coordinates" is a coordinate system we don't have access to here, as it's warped by the <map> elements.

I think the input of addFeatureVariations() should be in normalized coordinates after all.

@anthrotype
Copy link
Member

Why?... 🤯
The internal locations for masters, instances and thus for rules are all in raw design coordinates in the designspace document. Why now you say we don’t have access to them?

@robmck-ms
Copy link
Contributor

FYI: if this gets in, with some support for it in the designspace file (though, as noted above, that may need to change), it'd satisfy #1176.

@anthrotype
Copy link
Member

"raw design coordinates" is a coordinate system we don't have access to here, as it's warped by the elements.

oh... Is that because the addFeatureVariations function only takes the variable TTFont, thus does not have access to the designspace document itself where the axis map elements are defined (mapping user to raw, in Erik's terminology)?
The fvar axes's min, default and max are in user space by definition, whereas the avar mappings translate from default normalized coordinates to modified normalized coordinates, so those raw design coordinates cannot be retrieved any more without direct access to the designspace itself.

That's also probably why, e.g., varLib.interpolate_layout loads the designspace file and if mapped=True (meaning input locations is assumed to be in designspace raw internal coordinates) then it normalizes them using the internal_axis_supports, which is the min, default and max for each axis in raw design coordinates...

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Apr 18, 2018

Yeah, I wrote addFeatureVariations() as an agnostic bit of API, that is not tied to designspace in any way. I was confused (even more than I thought) about the various coordinate systems...

I think I will simply remove the normalization step from featureVars.py.

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Apr 18, 2018

Thinking about this once more:

Note that feature variations can potentially be used with features other than rvrn.

While this is true, it is impossible to combine such behavior with rvrn-based conditional substitutions, due to the first-match-only behavior of the FeatureVariations subtable. The exploding trickery that featureVars.py uses to implement more complex behavior can only work for one feature. We can manipulate the lookups for that one feature in arbitrary ways, but we can't do that for multiple features simultaneously. So I'm really not sure how feasible non-rvrn feature variations will be as a general principle.

@robmck-ms
Copy link
Contributor

Note that feature variations can potentially be used with features other than rvrn.

While this is true, it is impossible to combine such behavior with rvrn-based conditional substitutions, due to the first-match-only behavior of the FeatureVariations subtable.

I'm still trying to wrap my head around why this won't work (jetlag isn't helping). Do you have an example? A FeatureTableSubstitution table can list alternate feature records for multiple features, so a given condition can modify rvrn at the same time as other features. If you have ConditionSets with identical conditions, they could be collapsed. If you have one ConditionSet that matches a superset or overlapping set of another, then it can be decomposed into non-overlapping conditions. It would seem that any condition is computable, though it may take some pre-processing to save a more complex set of conditions in the file than specified in the parameters to addFeatureVariations().

@PeterCon
Copy link

A FeatureTableSubstitution table can list alternate feature records for multiple features...

Correction: there is no replacement of feature records. Feature records exist in the FeatureList table, and associate a feature tag with a default feature table. The FeatureTableSubstitution table provides an alternate feature table to replace the default table when certain conditions are matched.

But Rob's point is correct: the FeatureTableSubstitution table can substitute the default feature tables for several features with alternate feature tables.

@justvanrossum
Copy link
Collaborator Author

justvanrossum commented Apr 18, 2018

@PeterCon and @robmck-ms: sorry, you're absolutely right, I mixed things up. There can of course be multiple features affected by one condition, as the FeatureTableSubstitutionRecord field is an array.

@anthrotype
Copy link
Member

Following up from #1243, I think what's missing for merging this PR to master is:

  • Add tests to exercise the programmatic API
  • Hook it up to varLib.build, with the rule elements that are read from a designspace document translated into equivalent "Regions" and "Spaces" following featureVars terminology

Also, note that addFeatureVariations expects normalized coordinates whereas the designspace rule elements' conditions use non-normalized, internal designspace coordinates, so the caller needs to take responsibility of doing the normalization.

@behdad
Copy link
Member

behdad commented Jun 11, 2018

Can we merge and mark it absolutely experimental / subject-to-change/move? I can then try hooking it up to varLib and make any changes needed as I go.

Thanks.

@anthrotype
Copy link
Member

@behdad sgtm, thank you!

@twardoch
Copy link
Contributor

twardoch commented Sep 7, 2018

The three type of coordinates make sense. The type cycle is "designer -> machine -> user", so if we replace "designer" with "raw" and "machine" with "normalized", we're there.

I think there had been a lot of confusion about this because the OT spec only talks about the two latter steps while font tool developers also need to take the first one into account.

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.

None yet

7 participants