From 0ccce5b4b9ede1429b01a62fe65c7c9a12c2e2de Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 17 Nov 2023 12:47:14 -0500 Subject: [PATCH] [ttx_diff] Review feedback - less loops, more iterators - actually just use a BTreeSet for GlyphSet - better printing of anchors - improve integration into ttx_diff script (stop doing weird stdout redirection) --- layout-normalizer/src/common.rs | 109 +++++++++++++++++--------------- layout-normalizer/src/gpos.rs | 20 +++--- resources/scripts/ttx_diff.py | 38 +++++------ 3 files changed, 86 insertions(+), 81 deletions(-) diff --git a/layout-normalizer/src/common.rs b/layout-normalizer/src/common.rs index 8a342092c..356a86b1e 100644 --- a/layout-normalizer/src/common.rs +++ b/layout-normalizer/src/common.rs @@ -21,7 +21,7 @@ pub(crate) struct Feature { #[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord)] pub(crate) enum GlyphSet { Single(GlyphId), - Multiple(Vec), + Multiple(BTreeSet), } impl Feature { @@ -51,45 +51,47 @@ pub(crate) fn get_lang_systems( ) -> Vec { let data = script_list.offset_data(); - let lang_sys_iter = script_list.script_records().iter().flat_map(|script| { - let script_tag = script.script_tag(); - let script = script.script(data).unwrap(); - let maybe_default = script - .default_lang_sys() - .transpose() - .unwrap() - .map(|dflt| (script_tag, Tag::new(b"dflt"), dflt.feature_indices())); - let lang_sys_iter = script.lang_sys_records().iter().map(move |lang_sys| { - let lang_tag = lang_sys.lang_sys_tag(); - let lang = lang_sys.lang_sys(script.offset_data()).unwrap(); - (script_tag, lang_tag, lang.feature_indices()) - }); - maybe_default.into_iter().chain(lang_sys_iter) - }); - - let mut result = Vec::new(); - for (script, lang, feature_indices) in lang_sys_iter { - let data = feature_list.offset_data(); - - for idx in feature_indices { - let rec = feature_list - .feature_records() - .get(idx.get() as usize) - .unwrap(); - let feature = rec.feature(data).unwrap(); - let lookups = feature - .lookup_list_indices() - .iter() - .map(|x| x.get()) - .collect(); - result.push(Feature { - feature: rec.feature_tag(), - script, - lang, - lookups, + let mut result = script_list + .script_records() + .iter() + // first iterate all (script, lang, feature indices) + .flat_map(|script| { + let script_tag = script.script_tag(); + let script = script.script(data).unwrap(); + let maybe_default = script + .default_lang_sys() + .transpose() + .unwrap() + .map(|dflt| (script_tag, Tag::new(b"dflt"), dflt.feature_indices())); + let lang_sys_iter = script.lang_sys_records().iter().map(move |lang_sys| { + let lang_tag = lang_sys.lang_sys_tag(); + let lang = lang_sys.lang_sys(script.offset_data()).unwrap(); + (script_tag, lang_tag, lang.feature_indices()) + }); + maybe_default.into_iter().chain(lang_sys_iter) + }) + // then convert these into script/lang/feature/lookup indices + .flat_map(|(script, lang, indices)| { + indices.iter().map(move |idx| { + let rec = feature_list + .feature_records() + .get(idx.get() as usize) + .unwrap(); + let feature = rec.feature(feature_list.offset_data()).unwrap(); + let lookups = feature + .lookup_list_indices() + .iter() + .map(|x| x.get()) + .collect(); + Feature { + feature: rec.feature_tag(), + script, + lang, + lookups, + } }) - } - } + }) + .collect::>(); result.sort_unstable_by_key(|sys| sys.sort_key()); @@ -99,7 +101,7 @@ pub(crate) fn get_lang_systems( impl GlyphSet { pub(crate) fn make_set(&mut self) { if let GlyphSet::Single(gid) = self { - *self = GlyphSet::Multiple(vec![*gid]) + *self = GlyphSet::Multiple(BTreeSet::from([*gid])) } } @@ -109,8 +111,21 @@ impl GlyphSet { unreachable!() }; match other { - GlyphSet::Single(gid) => gids.push(gid), - GlyphSet::Multiple(multi) => gids.extend(multi), + GlyphSet::Single(gid) => { + gids.insert(gid); + } + GlyphSet::Multiple(mut multi) => gids.append(&mut multi), + } + } + + pub(crate) fn add(&mut self, gid: GlyphId) { + // if we're a single glyph, don't turn into a set if we're adding ourselves + if matches!(self, GlyphSet::Single(x) if *x == gid) { + return; + } + self.make_set(); + if let GlyphSet::Multiple(set) = self { + set.insert(gid); } } @@ -163,13 +178,3 @@ impl FromIterator for GlyphSet { GlyphSet::Multiple(iter.into_iter().collect()) } } - -impl From> for GlyphSet { - fn from(src: BTreeSet) -> GlyphSet { - if src.len() == 1 { - GlyphSet::Single(src.first().copied().unwrap()) - } else { - src.into_iter().collect() - } - } -} diff --git a/layout-normalizer/src/gpos.rs b/layout-normalizer/src/gpos.rs index e9637b5b2..d44d23d2b 100644 --- a/layout-normalizer/src/gpos.rs +++ b/layout-normalizer/src/gpos.rs @@ -1,6 +1,6 @@ use std::{ any::Any, - collections::{BTreeMap, BTreeSet, HashMap}, + collections::{BTreeMap, HashMap}, fmt::{Debug, Display}, io, }; @@ -655,8 +655,8 @@ fn get_mark_base_rules( let mark_anchor = ResolvedAnchor::new(&mark_anchor, delta_computer)?; marks .entry(mark_anchor) - .or_insert(BTreeSet::new()) - .insert(*mark_glyph); + .or_insert_with(|| GlyphSet::from(*mark_glyph)) + .add(*mark_glyph); } let group = MarkAttachmentRule { flags, @@ -664,7 +664,7 @@ fn get_mark_base_rules( base_anchor, marks: marks .into_iter() - .map(|(anchor, glyphs)| (anchor, glyphs.into())) + .map(|(anchor, glyphs)| (anchor, glyphs)) .collect(), kind: LookupType::MarkToBase, filter_set, @@ -714,8 +714,8 @@ fn get_mark_mark_rules( let mark_anchor = ResolvedAnchor::new(&mark_anchor, delta_computer)?; marks .entry(mark_anchor) - .or_insert(BTreeSet::new()) - .insert(*mark_glyph); + .or_insert_with(|| GlyphSet::from(*mark_glyph)) + .add(*mark_glyph); } let group = MarkAttachmentRule { flags, @@ -723,7 +723,7 @@ fn get_mark_mark_rules( base_anchor, marks: marks .into_iter() - .map(|(anchor, glyphs)| (anchor, glyphs.into())) + .map(|(anchor, glyphs)| (anchor, glyphs)) .collect(), kind: LookupType::MarkToMark, filter_set, @@ -780,7 +780,7 @@ impl Display for ResolvedValue { write!(f, " [({start})")?; for (i, adj) in values.iter().enumerate() { if i > 0 { - write!(f, ", ")?; + write!(f, ",")?; } write!(f, "{adj}")?; } @@ -790,7 +790,7 @@ impl Display for ResolvedValue { write!(f, " {{")?; for (i, var) in deltas.iter().enumerate() { if i > 0 { - write!(f, ", ")?; + write!(f, ",")?; } write!(f, "{var}")?; } @@ -804,6 +804,6 @@ impl Display for ResolvedValue { impl Display for ResolvedAnchor { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "@({},{})", self.x, self.y) + write!(f, "@(x: {}, y: {})", self.x, self.y) } } diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py index 81a7548d2..db26ca2fc 100755 --- a/resources/scripts/ttx_diff.py +++ b/resources/scripts/ttx_diff.py @@ -22,7 +22,7 @@ import shutil import subprocess import sys -from typing import MutableSequence, Optional +from typing import MutableSequence _COMPARE_DEFAULTS = "default" @@ -46,27 +46,17 @@ ) -def run( - cmd: MutableSequence, - working_dir: Path, - log_file: str, - redirect_stdout: Optional[str] = None, - **kwargs, -): +def run(cmd: MutableSequence, working_dir: Path, log_file: str, **kwargs): cmd_string = " ".join(cmd) print(f" (cd {working_dir} && {cmd_string} > {log_file} 2>&1)") log_file = working_dir / log_file with open(log_file, "w") as log_file: - if redirect_stdout is not None: - stdout = open(working_dir / redirect_stdout, "w") - else: - stdout = log_file subprocess.run( cmd, text=True, check=True, cwd=working_dir, - stdout=stdout, + stdout=log_file, stderr=log_file, **kwargs, ) @@ -80,19 +70,29 @@ def ttx(font_file: Path): ttx_file.name, font_file.name, ] - run(cmd, font_file.parent, "ttx.log", None) + run(cmd, font_file.parent, "ttx.log") return ttx_file # generate a simple text repr for gpos for this font def simple_gpos_output(font_file: Path, out_path: Path): - temppath = font_file.parent / "gpos.txt" - cmd = ["cargo", "run", "-p", "layout-normalizer", "--", font_file.name, "--table", "gpos"] + temppath = font_file.parent / "markkern.txt" + cmd = [ + "cargo", + "run", + "-p", + "layout-normalizer", + "--", + font_file.name, + "-o", + temppath.name, + "--table", + "gpos", + ] run( cmd, font_file.parent, - "gpos.log", - temppath.name, + "kernmark.log", ) copy(temppath, out_path) with open(out_path) as f: @@ -110,7 +110,7 @@ def build( if ttx_file.is_file(): print(f"skipping {build_tool}") return ttx_file - run(cmd, build_dir, build_tool + ".log", None, **kwargs) + run(cmd, build_dir, build_tool + ".log", **kwargs) ttfs = ttf_find_fn() assert len(ttfs) == 1, ttfs return ttfs[0]