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

[feaLib] variable syntax conditionsets #3180

Open
cmyr opened this issue Jun 23, 2023 · 1 comment
Open

[feaLib] variable syntax conditionsets #3180

cmyr opened this issue Jun 23, 2023 · 1 comment
Assignees

Comments

@cmyr
Copy link
Contributor

cmyr commented Jun 23, 2023

I've just implemented support for the conditionset/variation syntax in fea-rs, and I have a bunch of notes I want to record while it's freshly in mind. Some of these things probably deserve issues of their own, but I'm going to write this up in one place and then we can break out issues for individual concerns where people think it is merited.

  • NULL conditionset: according to the draft spec, the syntax for specifying a feature variation is,

     variation test $condition {
         # ...
     } test;
    

    where $condition can be either the name of a defined conditionset, or the 'NULL' keyword. Currently fonttools does not support the NULL keyword. I think this is fine, and that the NULL keyword can be removed from the spec draft; a missing conditionset is semantically identical to an empty conditionset, and so if the user wants to specify this behaviour they can declare a conditionset with no conditions, and the compiler can handle replacing empty conditionsets with a null offset as needed. Alternatively we could agree to use the NULL keyword, in which case fonttools should support it.

  • sorting order: the order that FeatureVariationRecords are stored in the FeatureVariations table is significant, since the shaping implementation will stop at the first matching ConditionSet. Currently (modulo a funny case I will touch on next) fonttools appears to sort conditionsets on the basis of when they are first referenced by a feature. I would advocate for changing this behaviour so that conditionsets are sorted based on the declaration order. My rationale is simply that declaration order is explicit and unambiguous; and if we pair this with the point above (removing NULL in favour of an explicitly declared empty conditionset where desired) then we also get a mechanism for explicitly controlling the sort position of the null conditionset. (ref: Use stable ConditionSet sort order cmyr/fea-rs#164)

  • multiple features per conditionset A given FeatureVariationRecord can perform multiple feature substitutions. (This is one of the reasons I think conditionsets should be sorted in declaration order; they might be referenced in variation statements for different features, in a way that might be hard to keep track of.) Currently fonttools seems to choke on this. For instance, given the following FEA:

    conditionset LIGHT {
       wght 200 400;
    } LIGHT;
    
    lookup mylookup {
        pos a 5;
    } mylookup;
    
    variation test LIGHT {
        lookup mylookup;
    } test;
    
    variation what LIGHT {
        lookup mylookup;
    } what;
    

    fonttools creates a single FeatureVariationRecord that ignores the 'test' feature, but has a variation for the 'what' feature. If I swap the order of the last two statements, then 'what' is ignored, and we get a variation for 'test'. Unless I'm missing something, I assume this is just a bug?

  • meta question: is this syntax used anywhere, or do we have a sense of where it might be used? All the discussion I can see focuses on the rvrn feature, and if that is the only thing we are intending to support, maybe it would be worth special-casing it explicitly?

  • additional concerns about the syntax/semantics: while I'm writing about this I might as well touch on two other issues that are relevant: the one is that it is not clear how to make the current syntax proposal work with the proposed avar2 table, and the other is that the current syntax does not provide a mechanism for excluding a lookup that was in the base feature from a variation. I'm not sure if this matters, but I want to at least note it.

TL;DR: we need to decide if we are actually adopting this syntax or not, and if so we need to determine what the semantics are and write them down somewhere. I think that tossing or seriously revising this syntax may be a good idea, but I defer to the people who are closer to the design process on that discussion.

@behdad
Copy link
Member

behdad commented Jun 23, 2023

  • the one is that it is not clear how to make the current syntax proposal work with the proposed avar2 table,

Right. See harfbuzz/boring-expansion-spec#94 (comment)

@anthrotype anthrotype self-assigned this Jul 3, 2023
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