set lookupflag and mark filtering set for kerning lookups #733
Conversation
cmyr
force-pushed
the
split-kern-by-script
branch
from
da37594
to
d4160af
Compare
Cargo.toml
Outdated
| @@ -52,3 +51,6 @@ members = [ | |||
| "layout-normalizer", | |||
| ] | |||
|
|
|||
| [patch.crates-io] | |||
There was a problem hiding this comment.
Presumably we should move to unpatched deps before merge.
fontbe/src/features/kern.rs
Outdated
| .as_ref() | ||
| .map(write_fonts::dump_table) | ||
| .transpose() | ||
| .expect("if this doesn't compile we will already panic when we try to add it to the context"); |
There was a problem hiding this comment.
Not quite sure I follow. This seems like it should return an Error and if that causes a downstream panic that's a bug?
There was a problem hiding this comment.
what this comment is saying is that if we panic here (because GSUB fails to pack) it's a case where we are already going to panic, because the way contexts work we unwrap this when we save it to the context.
fontbe/src/features/properties.rs
Outdated
| pub const COMMON_SCRIPT: UnicodeShortName = | ||
| unsafe { UnicodeShortName::from_bytes_unchecked(*b"Zyyy") }; | ||
| pub const INHERITED_SCRIPT: UnicodeShortName = | ||
| unsafe { UnicodeShortName::from_bytes_unchecked(*b"Zinh") }; |
There was a problem hiding this comment.
Having just gone through considerable delay as a result of unsafe I would very much like to avoid the unsafe.
Perhaps a function with a static OnceCell hidden inside would suffice?
There was a problem hiding this comment.
Do we anticipate trying to get fontc into google3 in the near future? I've opened unicode-org/icu4x#4691 to add a safe const constructor for tinystr, which will let us remove this; in the meantime the safety of these blocks is trivially verifiable. A function feels bad, but if this is an issue I can always just declare them as needed where they're used.
There was a problem hiding this comment.
If this is temporary it's fine to file an issue for it and submit. I wouldn't expect to put fontc in google3 soon.
There was a problem hiding this comment.
The Rust largely LGTM. I think @anthrotype better have a look before we merge as he has greater understanding of the nuances of the Python.
cmyr
force-pushed
the
split-kern-by-script
branch
from
b687af6
to
e1e743d
Compare
fontbe/src/features/kern.rs
Outdated
| @@ -585,7 +619,8 @@ fn debug_ordered_lookups( | |||
|
|
|||
| /// All the state needed for splitting kern pairs by script & writing direction | |||
| struct KernSplitContext { | |||
| gdef: Option<HashMap<GlyphId, GlyphClassDef>>, | |||
| /// map of all mark glyphs; bool indicates if mark is spacing | |||
There was a problem hiding this comment.
the MarkSpacing enum is not actually a "bool" though, is it? Should it be? maybe you changed it to enum and the comment is stale
There was a problem hiding this comment.
your deduction is correct 🕵️
This will let us pre-compute and store the lookupflags and any mark filtering set.
This also adds tests that we are generating the expected lookups & flags for mark & non-mark kerning lookups.
This adds a
PendingLookuptype so that we can compute and store the lookup flag and the mark filtering sets when we generate the kerning lookups.based on #731, which should go in first.