3 metadata.pb files don't parse using the proto definition in gftools

google / fonts

Font files available from Google Fonts, and a public issue tracker for all things Google Fonts

https://fonts.google.com

17.89k stars 2.6k forks source link

3 metadata.pb files don't parse using the proto definition in gftools #6666

Open rsheeter opened 10 months ago

rsheeter commented 10 months ago

Attempting to parse METADATA.pb using fonts_public_pb2.FamilyProto() from gftools, as shown in https://github.com/rsheeter/google_fonts_sources/pull/4, fails to parse some files:

ofl/notocoloremoji/METADATA.pb
ofl/wixmadefortext/METADATA.pb
ofl/pushster/METADATA.pb

RosaWagner commented 10 months ago

cc @simoncozens @m4rc1e

simoncozens commented 10 months ago

Noto Color Emoji is not my font, you may want to try @rsheeter. ;-)

RosaWagner commented 10 months ago

He is the one who posted the issue

chrissimpkins commented 9 months ago

@rsheeter Do you have additional details on the fails that you encountered on the parse attempts?

m4rc1e commented 9 months ago

ofl/wixmadefortext/METADATA.pb fails because it has the field "position" which I believe exists in the internal family proto file but not in the public version

m4rc1e commented 9 months ago

Likewise ofl/notocoloremoji/METADATA.pb fails because it has an additional field which we do not have, ordered_sample_glyphs

RosaWagner commented 9 months ago

@rsheeter can we close this issue?

rsheeter commented 9 months ago

I don't think it's been fixed?

simoncozens commented 9 months ago

The fix is either to synchronise the gftools proto definitions with your internal ones, or to use the allow_unknown_field=True option when calling text_format.Parse.

rsheeter commented 9 months ago

The former is preferable. I thought we did that recently but perhaps it's a figment of my imagination.

rsheeter commented 9 months ago

ofl/wixmadefortext/METADATA.pb fails because it has the field "position"

That one is fascinating because the internal copy of fonts_public.proto doesn't have it either.

Likewise ofl/notocoloremoji/METADATA.pb fails because it has an additional field which we do not have, ordered_sample_glyphs

I see ordered_sample_glyphs in https://github.com/googlefonts/gftools/blob/main/Lib/gftools/fonts_public.proto

rsheeter commented 9 months ago

ofl/pushster/METADATA.pb is a legitimate missing field.

m4rc1e commented 9 months ago

ofl/wixmadefortext/METADATA.pb fails because it has the field "position"

That one is fascinating because the internal copy of fonts_public.proto doesn't have it either.

@rsheeter I've just forwarded you an email regarding this field.

chrissimpkins commented 7 months ago

@m4rc1e status here?

chrissimpkins commented 6 months ago

@m4rc1e @rsheeter what's the status here?

m4rc1e commented 4 months ago

Currently, the position field, which is used in WixMadeFor Text, doesn't exist in our METADATA.pb files. I was told to include this field so it could allow us to support an unhinted variable font + statics that have been hinted with VTT. I've asked for this field to be added to our external protobuf files a few times in the fonts+oncall group (search for "wix madefor position")

chrissimpkins commented 3 months ago

I've asked for this field to be added to our external protobuf files a few times in the fonts+oncall group (search for "wix madefor position")

Who did you ask?

rsheeter commented 3 months ago

Will followup internally. I believe that field was originally meant to be internal only so we're in a weird spot.