GSUB stylistic set names not being added to name table

cmyr commented 5 days ago

noticed in Questrial (repo with python resources/scripts/ttx_diff.py 'https://github.com/googlefonts/questrial#sources/Questrial.glyphspackage').

I think this might've been brought up before, but can't find the issue.

I haven't dug into this but I suspect we're just generally not even looking at the name table that is produced by fea-rs. A larger problem is that fea-rs is assigning its own name values to things without knowing what is going on elsewhere, which means it's very easy for it to use nameIds that collide with those used elsewhere in the compilation process.

There are various possible solutions; one might be to have a 'PendingNameId' type that is used during compilation, and then these all get assigned their final values at the end; alternatively we could possible remap name values generated by fea-rs once we know what ids are used elsewhere.

khaledhosny commented 4 days ago

This was brought up in #969.

cmyr commented 4 days ago

right, I remembered this being mentioned at some point, thanks!

cmyr commented 3 days ago

Okay so after working on this a bunch I've run into something I'm extremely confused about:

I have a patch now in fontc that I believe is correct.
I'm still seeing a diff in GSUB (name matches, though!)
but the fontmake side of the diff is very confusing. Specifically, I get the following:

<Feature>
  <FeatureParamsStylisticSet>
    <Version value="0"/>
    <UINameID value="35"/>  <!-- Alternative capital forms -->
  </FeatureParamsStylisticSet>
  <!-- LookupCount=1 -->
  <LookupListIndex index="0" value="256"/>
</Feature>

This is confusing for two reasons:

35 is a reserved name id, and should not be assigned
256 does not reference any lookup
but 256 does happen to be the first non-reserved name id??

And by coincidence, my impl in fontc has these values reversed.

I tried to dig into fonttools a bit but found the logic very hard to follow, and I'm really not sure how this could happen, but it looks like these two values were somehow swapped? But I'm extremely unsure how this would happen, or not be noticed before now?

okay more info: the name has the correct value of 256 in the name table, for fontmake, and ttx is also clearly able to find the actual name for this record, so is this possibly some bug in ttx_diff? or something?

💡 ... this must be the result of us sorting values in lookups??

khaledhosny commented 3 days ago

Looks like a bug in ttx_diff, or somewhere else. I built Questrial locally and this is what I get in the .ttx file:

        <Feature>
          <FeatureParamsStylisticSet>
            <Version value="0"/>
            <UINameID value="256"/>  <!-- Alternative capital forms -->
          </FeatureParamsStylisticSet>
          <!-- LookupCount=1 -->
          <LookupListIndex index="0" value="35"/>
        </Feature>

The numbers of the lookup index and the name ID are swapped in your version.

cmyr commented 3 days ago

yea I tracked it down, thanks :)

googlefonts / fontc

GSUB stylistic set names not being added to name table #1130