diff --git a/fontbe/src/cmap.rs b/fontbe/src/cmap.rs index 337636d8..9c3551c9 100644 --- a/fontbe/src/cmap.rs +++ b/fontbe/src/cmap.rs @@ -25,12 +25,15 @@ impl Work for CmapWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::GlyphOrder) | AnyWorkId::Fe(FeWorkId::Glyph(..)) - ) - })) + Access::Custom( + "Cmap::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::GlyphOrder) | AnyWorkId::Fe(FeWorkId::Glyph(..)) + ) + }), + ) } /// Generate [cmap](https://learn.microsoft.com/en-us/typography/opentype/spec/cmap) diff --git a/fontbe/src/glyphs.rs b/fontbe/src/glyphs.rs index 0e9593bf..13afda3f 100644 --- a/fontbe/src/glyphs.rs +++ b/fontbe/src/glyphs.rs @@ -853,26 +853,32 @@ impl Work for GlyfLocaWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::StaticMetadata) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - | AnyWorkId::Be(WorkId::GlyfFragment(..)) - ) - })) + Access::Custom( + "GlyfLoca::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::StaticMetadata) + | AnyWorkId::Fe(FeWorkId::GlyphOrder) + | AnyWorkId::Be(WorkId::GlyfFragment(..)) + ) + }), + ) } fn write_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Be(WorkId::Glyf) - | AnyWorkId::Be(WorkId::Loca) - | AnyWorkId::Be(WorkId::LocaFormat) - | AnyWorkId::Be(WorkId::GlyfFragment(..)) - ) - })) + Access::Custom( + "GlyfLoca::Write", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Be(WorkId::Glyf) + | AnyWorkId::Be(WorkId::Loca) + | AnyWorkId::Be(WorkId::LocaFormat) + | AnyWorkId::Be(WorkId::GlyfFragment(..)) + ) + }), + ) } fn also_completes(&self) -> Vec { diff --git a/fontbe/src/gvar.rs b/fontbe/src/gvar.rs index 45d3365e..cfacae5e 100644 --- a/fontbe/src/gvar.rs +++ b/fontbe/src/gvar.rs @@ -42,14 +42,17 @@ impl Work for GvarWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::StaticMetadata) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - | AnyWorkId::Be(WorkId::GvarFragment(..)) - ) - })) + Access::Custom( + "Gvar::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::StaticMetadata) + | AnyWorkId::Fe(FeWorkId::GlyphOrder) + | AnyWorkId::Be(WorkId::GvarFragment(..)) + ) + }), + ) } /// Generate [gvar](https://learn.microsoft.com/en-us/typography/opentype/spec/gvar) diff --git a/fontbe/src/hvar.rs b/fontbe/src/hvar.rs index 8d45794e..c0fd524c 100644 --- a/fontbe/src/hvar.rs +++ b/fontbe/src/hvar.rs @@ -154,14 +154,17 @@ impl Work for HvarWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::Glyph(..)) - | AnyWorkId::Fe(FeWorkId::StaticMetadata) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - ) - })) + Access::Custom( + "Hvar::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::Glyph(..)) + | AnyWorkId::Fe(FeWorkId::StaticMetadata) + | AnyWorkId::Fe(FeWorkId::GlyphOrder) + ) + }), + ) } /// Generate [HVAR](https://learn.microsoft.com/en-us/typography/opentype/spec/HVAR) diff --git a/fontbe/src/metrics_and_limits.rs b/fontbe/src/metrics_and_limits.rs index db6c87b3..099125e9 100644 --- a/fontbe/src/metrics_and_limits.rs +++ b/fontbe/src/metrics_and_limits.rs @@ -199,28 +199,28 @@ impl Work for MetricAndLimitWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::Glyph(..)) - | AnyWorkId::Fe(FeWorkId::GlobalMetrics) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - | AnyWorkId::Be(WorkId::GlyfFragment(..)) - | AnyWorkId::Be(WorkId::Head) - ) - })) + Access::Custom( + "MetricsAndLimits::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::Glyph(..)) + | AnyWorkId::Fe(FeWorkId::GlobalMetrics) + | AnyWorkId::Fe(FeWorkId::GlyphOrder) + | AnyWorkId::Be(WorkId::GlyfFragment(..)) + | AnyWorkId::Be(WorkId::Head) + ) + }), + ) } fn write_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Be(WorkId::Hmtx) - | AnyWorkId::Be(WorkId::Hhea) - | AnyWorkId::Be(WorkId::Maxp) - | AnyWorkId::Be(WorkId::Head) - ) - })) + Access::set(HashSet::from([ + AnyWorkId::Be(WorkId::Hmtx), + AnyWorkId::Be(WorkId::Hhea), + AnyWorkId::Be(WorkId::Maxp), + AnyWorkId::Be(WorkId::Head), + ])) } fn also_completes(&self) -> Vec { diff --git a/fontbe/src/os2.rs b/fontbe/src/os2.rs index 7553174c..51b03a0f 100644 --- a/fontbe/src/os2.rs +++ b/fontbe/src/os2.rs @@ -797,19 +797,22 @@ impl Work for Os2Work { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - AnyWorkId::Fe(FeWorkId::Glyph(..)) - | AnyWorkId::Fe(FeWorkId::StaticMetadata) - | AnyWorkId::Fe(FeWorkId::GlyphOrder) - | AnyWorkId::Fe(FeWorkId::GlobalMetrics) - | AnyWorkId::Be(WorkId::Hhea) - | AnyWorkId::Be(WorkId::Hmtx) - | AnyWorkId::Be(WorkId::Gpos) - | AnyWorkId::Be(WorkId::Gsub) - ) - })) + Access::Custom( + "Os2::Read", + Arc::new(|id| { + matches!( + id, + AnyWorkId::Fe(FeWorkId::Glyph(..)) + | AnyWorkId::Fe(FeWorkId::StaticMetadata) + | AnyWorkId::Fe(FeWorkId::GlyphOrder) + | AnyWorkId::Fe(FeWorkId::GlobalMetrics) + | AnyWorkId::Be(WorkId::Hhea) + | AnyWorkId::Be(WorkId::Hmtx) + | AnyWorkId::Be(WorkId::Gpos) + | AnyWorkId::Be(WorkId::Gsub) + ) + }), + ) } /// Generate [OS/2](https://learn.microsoft.com/en-us/typography/opentype/spec/os2) diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs index bd8370c2..1b6a9b7a 100644 --- a/fontc/src/lib.rs +++ b/fontc/src/lib.rs @@ -42,7 +42,10 @@ use fontbe::{ }; use fontdrasil::types::GlyphName; -use fontir::{glyph::create_glyph_order_work, source::DeleteWork}; +use fontir::{ + glyph::{create_glyph_order_work, create_glyph_touchup_work}, + source::DeleteWork, +}; use fontbe::orchestration::Context as BeContext; use fontbe::paths::Paths as BePaths; @@ -209,8 +212,9 @@ fn add_glyph_ir_jobs(workload: &mut Workload) -> Result<(), Error> { .change_detector .ir_source() .create_glyph_ir_work(&glyphs_changed, workload.change_detector.current_inputs())?; - for work in glyph_work { + for (work, glyph_name) in glyph_work.into_iter().zip(glyphs_changed) { workload.add(work.into(), true); + workload.add(create_glyph_touchup_work(glyph_name).into(), true); } Ok(()) @@ -670,6 +674,11 @@ mod tests { .iter() .map(|n| FeWorkIdentifier::Glyph((*n).into()).into()), ); + expected.extend( + glyphs + .iter() + .map(|n| FeWorkIdentifier::GlyphTouchup((*n).into()).into()), + ); expected.extend( glyphs .iter() @@ -747,6 +756,7 @@ mod tests { assert_eq!( vec![ AnyWorkId::Fe(FeWorkIdentifier::Glyph("bar".into())), + AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup("bar".into())), FeWorkIdentifier::Anchor("bar".into()).into(), BeWorkIdentifier::Features.into(), BeWorkIdentifier::Cmap.into(), @@ -2503,4 +2513,47 @@ mod tests { // We had a bug where it was 2 assert_eq!(1, mark_base_lookups(&gpos).len()); } + + #[test] + fn compile_nested_components_prefer_simple() { + let temp_dir = tempdir().unwrap(); + let build_dir = temp_dir.path(); + let mut args = Args::for_test(build_dir, "glyphs3/ComponentComponents.glyphs"); + args.prefer_simple_glyphs = true; + let result = compile(Args::for_test( + build_dir, + "glyphs3/ComponentComponents.glyphs", + )); + + // Our deeply nested component w/contours should have become simple + let glyph = result.fe_context.glyphs.get(&FeWorkIdentifier::Glyph( + "component_component_and_contour".into(), + )); + let instance = glyph.default_instance(); + assert_eq!( + (instance.contours.is_empty(), instance.components.is_empty()), + (false, true), + "{instance:?} should have only contours" + ); + } + + #[test] + fn compile_nested_components_prefer_components() { + let temp_dir = tempdir().unwrap(); + let build_dir = temp_dir.path(); + let mut args = Args::for_test(build_dir, "glyphs3/ComponentComponents.glyphs"); + args.prefer_simple_glyphs = false; + let result = compile(args); + + // Our deeply nested component w/contours should have had it's contours hoisted out + let glyph = result.fe_context.glyphs.get(&FeWorkIdentifier::Glyph( + "component_component_and_contour".into(), + )); + let instance = glyph.default_instance(); + assert_eq!( + (instance.contours.is_empty(), instance.components.is_empty()), + (true, false), + "{instance:?} should have only components" + ); + } } diff --git a/fontc/src/timing.rs b/fontc/src/timing.rs index d4315666..ba85dccd 100644 --- a/fontc/src/timing.rs +++ b/fontc/src/timing.rs @@ -119,6 +119,7 @@ fn short_name(id: &AnyWorkId) -> &'static str { AnyWorkId::Fe(FeWorkIdentifier::Features) => "fea", AnyWorkId::Fe(FeWorkIdentifier::GlobalMetrics) => "metrics", AnyWorkId::Fe(FeWorkIdentifier::Glyph(..)) => "glyph", + AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup(..)) => "touchup", AnyWorkId::Fe(FeWorkIdentifier::GlyphIrDelete(..)) => "rm ir", AnyWorkId::Fe(FeWorkIdentifier::GlyphOrder) => "glyphorder", AnyWorkId::Fe(FeWorkIdentifier::Kerning) => "kern", @@ -159,6 +160,7 @@ fn color(id: &AnyWorkId) -> &'static str { AnyWorkId::Fe(FeWorkIdentifier::Features) => "#e18707", AnyWorkId::Fe(FeWorkIdentifier::GlobalMetrics) => "gray", AnyWorkId::Fe(FeWorkIdentifier::Glyph(..)) => "#830356", + AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup(..)) => "#720345", AnyWorkId::Fe(FeWorkIdentifier::GlyphIrDelete(..)) => "gray", AnyWorkId::Fe(FeWorkIdentifier::GlyphOrder) => "gray", AnyWorkId::Fe(FeWorkIdentifier::Kerning) => "gray", diff --git a/fontc/src/work.rs b/fontc/src/work.rs index 5a00d7ef..3c9a6f71 100644 --- a/fontc/src/work.rs +++ b/fontc/src/work.rs @@ -166,10 +166,9 @@ impl AnyContext { AnyWorkId::Be(..) => AnyContext::Be( be_root.copy_for_work(read_access.unwrap_be(), write_access.unwrap_be()), ), - AnyWorkId::Fe(..) => AnyContext::Fe(fe_root.copy_for_work( - Access::custom(move |id: &WorkId| read_access.check(&AnyWorkId::Fe(id.clone()))), - Access::custom(move |id: &WorkId| write_access.check(&AnyWorkId::Fe(id.clone()))), - )), + AnyWorkId::Fe(..) => AnyContext::Fe( + fe_root.copy_for_work(read_access.unwrap_fe(), write_access.unwrap_fe()), + ), AnyWorkId::InternalTiming(..) => { panic!("Should never create a context for internal timing") } diff --git a/fontc/src/workload.rs b/fontc/src/workload.rs index 68639352..ad0341e3 100644 --- a/fontc/src/workload.rs +++ b/fontc/src/workload.rs @@ -45,18 +45,37 @@ pub struct Workload<'a> { #[derive(Debug)] pub(crate) struct Job { pub(crate) id: AnyWorkId, - // The actual task. Exec takes work and sets the running flag. + /// The actual task. Exec takes work and sets the running flag. pub(crate) work: Option, - // Things our job needs read access to. Job won't run if anything it can read is pending. - pub(crate) read_access: AnyAccess, - // Things our job needs write access to + /// Things that must complete before this job can run + pub(crate) dependencies: AnyAccess, + /// Things our job needs read access to. + /// + /// If None, use dependencies as read access. + pub(crate) read_access: Option, + /// Things our job needs write access to pub(crate) write_access: AnyAccess, - // Does this job actually need to execute? + /// Does this job actually need to execute? pub(crate) run: bool, - // is this job running right now? + /// is this job running right now? pub(crate) running: bool, } +impl Job { + fn create_context(&self, fe_root: &FeContext, be_root: &BeContext) -> AnyContext { + AnyContext::for_work( + fe_root, + be_root, + &self.id, + self.read_access + .as_ref() + .unwrap_or(&self.dependencies) + .clone(), + self.write_access.clone(), + ) + } +} + enum RecvType { Blocking, NonBlocking, @@ -77,11 +96,11 @@ impl<'a> Workload<'a> { /// True if job might read what other produces fn might_read(&self, job: &Job, other: &Job) -> bool { - let result = job.read_access.check(&other.id) + let result = job.dependencies.check(&other.id) || self .also_completes .get(&other.id) - .map(|also| also.iter().any(|other_id| job.read_access.check(other_id))) + .map(|also| also.iter().any(|other_id| job.dependencies.check(other_id))) .unwrap_or_default(); result } @@ -100,7 +119,8 @@ impl<'a> Workload<'a> { Job { id, work: Some(work), - read_access, + dependencies: read_access, + read_access: None, write_access, run: should_run, running: false, @@ -134,8 +154,26 @@ impl<'a> Workload<'a> { } } - /// When BE glyph jobs are initially created they don't know enough to set fine grained dependencies - /// so they depend on *all* IR glyphs. Once IR for a glyph completes we can refine that: + fn update_dependencies( + &mut self, + job_id: &AnyWorkId, + dependencies: AnyAccess, + read_access: Option, + ) { + let job = self + .jobs_pending + .get_mut(job_id) + .unwrap_or_else(|| panic!("{job_id:?} should exist")); + trace!( + "Updating {job_id:?} deps from {:?} to {dependencies:?}, read {read_access:?} (None means the same as deps)", + job.dependencies + ); + job.dependencies = dependencies; + job.read_access = read_access; + } + + /// When Glyph Touchup and BE glyph jobs are initially created they don't know enough to + /// set fine grained dependencies. Once IR for a glyph completes we can set them: /// /// * If the glyph doesn't emit to binary we don't need to do the BE work at all /// * If the glyph has no components the BE for it doesn't use glyph order and needn't block on it @@ -143,44 +181,66 @@ impl<'a> Workload<'a> { /// * For example, flatten /// /// By minimizing dependencies we allow jobs to start earlier and execute with greater concurrency. - fn update_be_glyph_work(&mut self, fe_root: &FeContext, glyph_name: GlyphName) { - let glyph = fe_root - .glyphs - .get(&FeWorkIdentifier::Glyph(glyph_name.clone())); - let be_id = AnyWorkId::Be(BeWorkIdentifier::GlyfFragment(glyph_name)); - let be_job = self - .jobs_pending - .get_mut(&be_id) - .expect("{be_id} should exist"); + fn update_glyph_work( + &mut self, + fe_root: &FeContext, + glyph_name: GlyphName, + generated_glyph: bool, + ) { + let glyph_ir_id = FeWorkIdentifier::Glyph(glyph_name.clone()); + let glyph = fe_root.glyphs.get(&glyph_ir_id); + let touchup_id = AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup(glyph_name.clone())); + let be_id = AnyWorkId::Be(BeWorkIdentifier::GlyfFragment(glyph_name.clone())); if !glyph.emit_to_binary { - trace!("Skipping execution of {be_id:?}; it does not emit to binary"); - self.jobs_pending.remove(&be_id); - self.mark_also_completed(&be_id); - self.success.insert(be_id); + for id in [touchup_id, be_id] { + trace!("Skipping execution of {id:?}; it does not emit to binary"); + self.jobs_pending.remove(&id); + self.mark_also_completed(&id); + self.success.insert(id); + } return; } - let mut deps = HashSet::from([AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata)]); + let mut touchup_deps: HashSet = HashSet::from([glyph_ir_id.clone()]); + + let mut be_deps = HashSet::from([ + AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata), + AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup(glyph_name.clone())), + ]); - let mut has_components = false; + let mut components_visited = HashSet::new(); for inst in glyph.sources().values() { for component in inst.components.iter() { - has_components = true; - deps.insert(FeWorkIdentifier::Glyph(component.base.clone()).into()); + if components_visited.insert(component.base.clone()) { + let work_id = FeWorkIdentifier::Glyph(component.base.clone()); + be_deps.insert(work_id.clone().into()); + touchup_deps.insert(work_id.clone()); + } } } - // We don't *have* to wait on glyph order, but if we don't it delays the critical path - if has_components { - deps.insert(FeWorkIdentifier::GlyphOrder.into()); + if !components_visited.is_empty() { + be_deps.insert(FeWorkIdentifier::GlyphOrder.into()); } - trace!( - "Updating {be_id:?} deps from {:?} to {deps:?}", - be_job.read_access - ); - be_job.read_access = Access::Set(deps).into(); + // We don't touchup generated glyphs + if !generated_glyph { + let read_access = AnyAccess::Fe(Access::Custom( + "touchup", + Arc::new(|id| { + matches!( + id, + // Touchup needs to see the transitive dependencies of the glyph + // We don't know what those are so just allow access to any glyph + FeWorkIdentifier::Glyph(..) + ) + }), + )); + let touchup_deps = AnyAccess::Fe(Access::set(touchup_deps)); + self.update_dependencies(&touchup_id, touchup_deps, Some(read_access)); + } + self.update_dependencies(&be_id, Access::Set(be_deps).into(), None); } fn handle_success(&mut self, fe_root: &FeContext, success: AnyWorkId, timing: JobTime) { @@ -202,12 +262,12 @@ impl<'a> Workload<'a> { super::add_glyph_be_job(self, glyph_name.clone()); // Glyph order is done so all IR must be done. Copy dependencies from the IR for the same name. - self.update_be_glyph_work(fe_root, glyph_name.clone()); + self.update_glyph_work(fe_root, glyph_name.clone(), true); } } if let AnyWorkId::Fe(FeWorkIdentifier::Glyph(glyph_name)) = success { - self.update_be_glyph_work(fe_root, glyph_name); + self.update_glyph_work(fe_root, glyph_name, false); } } @@ -222,7 +282,7 @@ impl<'a> Workload<'a> { fn can_run(&self, job: &Job) -> bool { // Custom access requires a scan across pending jobs, hence avoidance is nice - match &job.read_access { + match &job.dependencies { AnyAccess::Fe(access) => match access { Access::None => true, Access::Unknown => false, @@ -288,24 +348,21 @@ impl<'a> Workload<'a> { while self.success.len() < self.job_count { // Spawn anything that is currently executable (has no unfulfilled dependencies) self.update_launchable(&mut launchable); - if launchable.is_empty() && !self.jobs_pending.values().any(|j| j.running) { - if log::log_enabled!(log::Level::Warn) { - warn!("{}/{} jobs have succeeded, nothing is running, and nothing is launchable", self.success.len(), self.job_count); - for pending in self.jobs_pending.keys() { - warn!(" blocked: {pending:?}"); + if launchable.is_empty() { + if !self.jobs_pending.values().any(|j| j.running) { + if log::log_enabled!(log::Level::Warn) { + warn!("{}/{} jobs have succeeded, nothing is running, and nothing is launchable", self.success.len(), self.job_count); + for pending in self.jobs_pending.keys() { + warn!(" blocked: {pending:?}"); + } } + return Err(Error::UnableToProceed(self.jobs_pending.len())); } - return Err(Error::UnableToProceed(self.jobs_pending.len())); } // Get launchables ready to run if !launchable.is_empty() { nth_wave += 1; - } - - // count of launchable jobs that are runnable, i.e. excluding !run jobs - let mut runnable_count = launchable.len(); - { let mut run_queue = run_queue.lock().unwrap(); for id in launchable.iter() { let timing = create_timer(id.clone(), nth_wave); @@ -330,13 +387,7 @@ impl<'a> Workload<'a> { runnable_count -= 1; continue; } - let work_context = AnyContext::for_work( - fe_root, - be_root, - id, - job.read_access.clone(), - job.write_access.clone(), - ); + let work_context = job.create_context(fe_root, be_root); let timing = timing.queued(); run_queue.push((work, timing, work_context)); @@ -348,17 +399,19 @@ impl<'a> Workload<'a> { // Higher priority sorts last, which means run first due to pop // We basically want things that block the glyph order => kern => fea sequence to go asap match work.id() { + AnyWorkId::Be(BeWorkIdentifier::Features) => 99, + AnyWorkId::Be(BeWorkIdentifier::Kerning) => 99, + AnyWorkId::Be(BeWorkIdentifier::GlyfFragment(..)) => 0, + AnyWorkId::Be(BeWorkIdentifier::GvarFragment(..)) => 0, AnyWorkId::Fe(FeWorkIdentifier::Features) => 99, AnyWorkId::Fe(FeWorkIdentifier::Kerning) => 99, AnyWorkId::Fe(FeWorkIdentifier::GlyphOrder) => 99, AnyWorkId::Fe(FeWorkIdentifier::PreliminaryGlyphOrder) => 99, AnyWorkId::Fe(FeWorkIdentifier::StaticMetadata) => 99, AnyWorkId::Fe(FeWorkIdentifier::GlobalMetrics) => 99, - AnyWorkId::Be(BeWorkIdentifier::Kerning) => 99, - AnyWorkId::Be(BeWorkIdentifier::Features) => 99, - AnyWorkId::Be(BeWorkIdentifier::GlyfFragment(..)) => 0, - AnyWorkId::Be(BeWorkIdentifier::GvarFragment(..)) => 0, - _ => 32, + AnyWorkId::Fe(FeWorkIdentifier::Glyph(..)) => 1, + AnyWorkId::Fe(FeWorkIdentifier::GlyphTouchup(..)) => 1, + _ => 0, } }); } @@ -550,8 +603,7 @@ impl<'a> Workload<'a> { let job = self.jobs_pending.remove(id).unwrap(); if job.run { let timing = timing.queued(); - let context = - AnyContext::for_work(fe_root, be_root, id, job.read_access, job.write_access); + let context = job.create_context(fe_root, be_root); log::debug!("Exec {:?}", id); let timing = timing.run(); job.work diff --git a/fontdrasil/src/orchestration.rs b/fontdrasil/src/orchestration.rs index 83f112fa..4090181a 100644 --- a/fontdrasil/src/orchestration.rs +++ b/fontdrasil/src/orchestration.rs @@ -29,7 +29,7 @@ pub enum Access { /// Access to multiple resources is permitted Set(HashSet), /// A closure is used to determine access - Custom(Arc bool + Send + Sync>), + Custom(&'static str, Arc bool + Send + Sync>), } impl Debug for Access { @@ -40,7 +40,7 @@ impl Debug for Access { Self::All => write!(f, "All"), Self::One(arg0) => f.debug_tuple("One").field(arg0).finish(), Self::Set(arg0) => f.debug_tuple("Set").field(arg0).finish(), - Self::Custom(..) => write!(f, "Custom"), + Self::Custom(desc, ..) => write!(f, "Custom({desc})"), } } } @@ -135,8 +135,8 @@ impl AccessControlList { impl Access { /// Create a new access rule with custom logic - pub fn custom bool + Send + Sync + 'static>(func: F) -> Self { - Access::Custom(Arc::new(func)) + pub fn custom bool + Send + Sync + 'static>(desc: &'static str, func: F) -> Self { + Access::Custom(desc, Arc::new(func)) } pub fn all() -> Self { @@ -154,6 +154,14 @@ impl Access { Self::One(allow_id) } + pub fn set(allow_ids: HashSet) -> Self { + match allow_ids.len() { + 0 => Access::None, + 1 => Access::One(allow_ids.into_iter().next().unwrap()), + _ => Access::Set(allow_ids), + } + } + /// Check whether a given id is allowed per this rule. pub fn check(&self, id: &I) -> bool { match self { @@ -162,7 +170,7 @@ impl Access { Access::All => true, Access::One(allow) => id == allow, Access::Set(ids) => ids.contains(id), - Access::Custom(f) => f(id), + Access::Custom(_, f) => f(id), } } } @@ -192,14 +200,14 @@ fn assert_access_many( AccessCheck::Any => ids.iter().any(|id| access.check(id)), }; if !allow { - panic!("Illegal {desc} of {demand} {ids:?}"); + panic!("Illegal {desc} of {demand} {ids:?}, allowed {access:?}"); } } fn assert_access_one(access: &Access, id: &I, desc: &str) { let allow = access.check(id); if !allow { - panic!("Illegal {desc} of {id:?}"); + panic!("Illegal {desc} of {id:?}, allowed {access:?}"); } } diff --git a/fontir/src/glyph.rs b/fontir/src/glyph.rs index 8b7310eb..5f80b39b 100644 --- a/fontir/src/glyph.rs +++ b/fontir/src/glyph.rs @@ -27,6 +27,19 @@ pub fn create_glyph_order_work() -> Box { Box::new(GlyphOrderWork {}) } +/// Apply common adjustments to the glyph created by [`crate::source::Source`]. +/// +/// Used primarily to move work from occurring serially in glyph order to occurring +/// on per-glyph threads. +pub fn create_glyph_touchup_work(glyph_name: GlyphName) -> Box { + Box::new(GlyphTouchupIrWork { glyph_name }) +} + +#[derive(Debug)] +struct GlyphTouchupIrWork { + glyph_name: GlyphName, +} + #[derive(Debug)] struct GlyphOrderWork {} @@ -336,21 +349,26 @@ impl Work for GlyphOrderWork { } fn read_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!( - id, - WorkId::Glyph(..) - | WorkId::StaticMetadata - | WorkId::PreliminaryGlyphOrder - | WorkId::GlobalMetrics - ) - })) + Access::Custom( + "GlyphOrder::Read", + Arc::new(|id| { + matches!( + id, + WorkId::Glyph(..) + | WorkId::GlyphTouchup(..) + | WorkId::StaticMetadata + | WorkId::PreliminaryGlyphOrder + | WorkId::GlobalMetrics + ) + }), + ) } fn write_access(&self) -> Access { - Access::Custom(Arc::new(|id| { - matches!(id, WorkId::Glyph(..) | WorkId::GlyphOrder) - })) + Access::Custom( + "GlyphOrder::Write", + Arc::new(|id| matches!(id, WorkId::Glyph(..) | WorkId::GlyphOrder)), + ) } fn exec(&self, context: &Context) -> Result<(), WorkError> { @@ -373,30 +391,18 @@ impl Work for GlyphOrderWork { } } - // Glyphs with paths and components, and glyphs whose component 2x2 transforms vary over designspace - // are not directly supported in fonts. To resolve we must do one of: - // 1) need to push their paths to a new glyph that is a component - // 2) collapse such glyphs into a simple (contour-only) glyph - // fontmake (Python) prefers option 2. + // Glyph touchup already converted components to contours where necessary so here we need only + // extract contours to new components where appropriate. for glyph_name in new_glyph_order.clone().iter() { let glyph = original_glyphs.get(glyph_name).unwrap(); - let inconsistent_components = !glyph.has_consistent_components(); - if inconsistent_components || has_components_and_contours(glyph) { - if inconsistent_components { - debug!( - "Coalescing'{0}' into a simple glyph because component 2x2s vary across the designspace", - glyph.name - ); - convert_components_to_contours(context, glyph)?; - } else if context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) { - debug!( - "Coalescing '{0}' into a simple glyph because it has contours and components and prefer simple glyphs is set", - glyph.name - ); - convert_components_to_contours(context, glyph)?; - } else { - move_contours_to_new_component(context, &mut new_glyph_order, glyph)?; - } + assert!( + glyph.has_consistent_components(), + "{glyph_name} component consistency wasn't fixed by touchup" + ); + if has_components_and_contours(glyph) + && !context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) + { + move_contours_to_new_component(context, &mut new_glyph_order, glyph)?; } } drop(original_glyphs); // lets not accidentally use that from here on @@ -456,6 +462,51 @@ impl Work for GlyphOrderWork { } } +impl Work for GlyphTouchupIrWork { + fn id(&self) -> WorkId { + WorkId::GlyphTouchup(self.glyph_name.clone()) + } + + /// We need access to the glyph IR plus it's components IR but we don't yet know what those are yet + fn read_access(&self) -> Access { + Access::Unknown + } + + /// We write to glyph ir not the touchup bucket + fn write_access(&self) -> Access { + Access::one(WorkId::Glyph(self.glyph_name.clone())) + } + + fn exec(&self, context: &Context) -> Result<(), WorkError> { + let glyph = context.glyphs.get(&WorkId::Glyph(self.glyph_name.clone())); + + // Glyphs with paths and components, and glyphs whose component 2x2 transforms vary over designspace + // are not directly supported in fonts. To resolve we must do one of: + // 1) need to push their paths to a new glyph that is a component + // 2) collapse such glyphs into a simple (contour-only) glyph + // We handle case 2 here. Case 1 is done by the glyph order job. + // fontmake (Python) prefers option 2. + if !glyph.has_consistent_components() { + debug!( + "Coalescing'{0}' into a simple glyph because component 2x2s vary across the designspace", + glyph.name + ); + convert_components_to_contours(context, &glyph)?; + } else if has_components_and_contours(&glyph) + && context.flags.contains(Flags::PREFER_SIMPLE_GLYPHS) + { + debug!( + "Coalescing '{0}' into a simple glyph because it has contours and components and prefer simple glyphs is set", + glyph.name + ); + convert_components_to_contours(context, &glyph)?; + } + // If we have components and contours and do *not* prefer simple then we need to generate a new component + // That is handled by the glyph order job + Ok(()) + } +} + #[cfg(test)] mod tests { use std::{collections::HashSet, path::Path}; diff --git a/fontir/src/orchestration.rs b/fontir/src/orchestration.rs index 2b07e81d..51a0fdf2 100644 --- a/fontir/src/orchestration.rs +++ b/fontir/src/orchestration.rs @@ -314,6 +314,7 @@ pub enum WorkId { /// Build potentially variable font-wide metrics. GlobalMetrics, Glyph(GlyphName), + GlyphTouchup(GlyphName), GlyphIrDelete(GlyphName), /// Glyph order from source, prior to adjustment /// diff --git a/fontir/src/paths.rs b/fontir/src/paths.rs index d7bf9554..cee65db9 100644 --- a/fontir/src/paths.rs +++ b/fontir/src/paths.rs @@ -11,6 +11,7 @@ pub struct Paths { build_dir: PathBuf, anchor_ir_dir: PathBuf, glyph_ir_dir: PathBuf, + glyph_ir_touchup_dir: PathBuf, ir_input_file: PathBuf, } @@ -19,11 +20,13 @@ impl Paths { let build_dir = build_dir.to_path_buf(); let anchor_ir_dir = build_dir.join("anchor_ir"); let glyph_ir_dir = build_dir.join("glyph_ir"); + let glyph_ir_touchup_dir = build_dir.join("glyph_ir_touchup"); let ir_input_file = build_dir.join("irinput.yml"); Paths { build_dir, anchor_ir_dir, glyph_ir_dir, + glyph_ir_touchup_dir, ir_input_file, } } @@ -52,6 +55,10 @@ impl Paths { self.glyph_ir_dir.join(safe_filename(name, ".yml")) } + fn glyph_ir_touchup_file(&self, name: &str) -> PathBuf { + self.glyph_ir_touchup_dir.join(safe_filename(name, ".yml")) + } + pub fn target_file(&self, id: &WorkId) -> PathBuf { match id { WorkId::Anchor(name) => self.anchor_ir_file(name.as_str()), @@ -60,6 +67,7 @@ impl Paths { WorkId::GlyphOrder => self.build_dir.join("glyph_order.yml"), WorkId::GlobalMetrics => self.build_dir.join("global_metrics.yml"), WorkId::Glyph(name) => self.glyph_ir_file(name.as_str()), + WorkId::GlyphTouchup(name) => self.glyph_ir_touchup_file(name.as_str()), WorkId::GlyphIrDelete(name) => { self.build_dir.join(format!("delete-{}.yml", name.as_str())) } diff --git a/resources/testdata/glyphs3/ComponentComponents.glyphs b/resources/testdata/glyphs3/ComponentComponents.glyphs new file mode 100644 index 00000000..a0fe343a --- /dev/null +++ b/resources/testdata/glyphs3/ComponentComponents.glyphs @@ -0,0 +1,134 @@ +{ +.appVersion = "3151"; +.formatVersion = 3; +DisplayStrings = ( +".", +"," +); +date = "2023-01-20 20:20:30 +0000"; +familyName = "New Font"; +fontMaster = ( +{ +id = m01; +metricValues = ( +{ +over = 16; +pos = 800; +}, +{ +over = 16; +pos = 700; +}, +{ +over = 16; +pos = 500; +}, +{ +over = -16; +}, +{ +over = -16; +pos = -200; +}, +{ +over = -16; +} +); +name = Regular; +} +); +glyphs = ( +{ +glyphname = simple; +lastChange = "2023-01-20 20:20:51 +0000"; +layers = ( +{ +layerId = m01; +shapes = ( +{ +closed = 1; +nodes = ( +(238,0,l), +(362,0,l), +(362,112,l), +(238,112,l) +); +} +); +width = 600; +} +); +unicode = 46; +}, +{ +glyphname = simple_component; +lastChange = "2023-01-20 20:22:39 +0000"; +layers = ( +{ +layerId = m01; +shapes = ( +{ +angle = 34.88155; +pos = (-233,-129); +ref = simple; +scale = (2.0303,2.0303); +} +); +width = 600; +} +); +unicode = 44; +}, +{ +glyphname = component_component_and_contour; +lastChange = "2023-01-20 20:22:39 +0000"; +layers = ( +{ +layerId = m01; +shapes = ( +{ +closed = 1; +nodes = ( +(238,0,l), +(362,0,l), +(362,112,l), +(238,112,l) +); +}, +{ +angle = 34.88155; +pos = (-233,-129); +ref = simple_component; +scale = (1.0303,2.0303); +} +); +width = 600; +} +); +unicode = 44; +} +); +metrics = ( +{ +type = ascender; +}, +{ +type = "cap height"; +}, +{ +type = "x-height"; +}, +{ +type = baseline; +}, +{ +type = descender; +}, +{ +type = "italic angle"; +} +); +unitsPerEm = 1000; +versionMajor = 1; +versionMinor = 0; +}