Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of fea-rs output #306

Merged
merged 1 commit into from
May 26, 2023
Merged

Improve handling of fea-rs output #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 38 additions & 24 deletions fontbe/src/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ use fea_rs::{
Compiler, GlyphMap, GlyphName as FeaRsGlyphName,
};
use fontir::{ir::Features, orchestration::Flags};
use log::{debug, error, trace, warn};
use write_fonts::FontBuilder;
use log::{debug, error, warn};
use read_fonts::{FontRef, TableProvider};
use write_fonts::{from_obj::ToOwnedTable, FontBuilder};

use fontdrasil::orchestration::Work;

use crate::{
error::Error,
orchestration::{BeWork, Context},
orchestration::{BeWork, Context, WorkId},
};

pub struct FeatureWork {}
Expand Down Expand Up @@ -111,29 +112,42 @@ fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &s
impl Work<Context, Error> for FeatureWork {
fn exec(&self, context: &Context) -> Result<(), Error> {
let features = context.ir.get_features();
if let Features::Empty = *features {
// set a default in place so subsequent compiles skip this step
trace!("No fea file, dull compile");
context.set_features(FontBuilder::default());
return Ok(());
}
let glyph_order = &context.ir.get_final_static_metadata().glyph_order;
if glyph_order.is_empty() {
warn!("Glyph order is empty; feature compile improbable");
}
let glyph_map = glyph_order
.iter()
.map(|n| Into::<FeaRsGlyphName>::into(n.as_str()))
.collect();

let result = self.compile(&features, glyph_map);
if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) {
if let Features::Memory(fea_content) = &*features {
write_debug_fea(context, result.is_err(), "compile failed", fea_content);
if !matches!(*features, Features::Empty) {
let glyph_order = &context.ir.get_final_static_metadata().glyph_order;
if glyph_order.is_empty() {
warn!("Glyph order is empty; feature compile improbable");
}
let glyph_map = glyph_order
.iter()
.map(|n| Into::<FeaRsGlyphName>::into(n.as_str()))
.collect();

let result = self.compile(&features, glyph_map);
if result.is_err() || context.flags.contains(Flags::EMIT_DEBUG) {
if let Features::Memory(fea_content) = &*features {
write_debug_fea(context, result.is_err(), "compile failed", fea_content);
}
}
let buf = result?.build();
let font = FontRef::new(&buf).unwrap();
debug!(
"Built features, gpos? {} gsub? {}",
font.gpos().is_ok(),
font.gsub().is_ok()
);
if let Ok(gpos) = font.gpos() {
context.set_gpos(gpos.to_owned_table());
}
if let Ok(gsub) = font.gsub() {
context.set_gsub(gsub.to_owned_table());
}
Comment on lines +132 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very convoluted way of trying to do what we want, I think? Wouldn't it be nicer if we just had some way of retrieving the bytes of a specific table from the FontBuilder? (we'd need to add a method there, but nothing crazy)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way it's typed which is nice perhaps? - I agree it feels a bit clunky but ... it does express the intent in fairly straight forward terms

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

although maybe there's no good work around from this? it certainly doesn't change if I'm returning a set of write-fonts tables instead of a write-fonts TableBuilder. But it still might be a better API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a sign that fea-rs should be returning something other than a FontBuilder?

It would also be nice if fea-rs did not return a type from write-fonts directly, since this is a reason why every breaking change in write-fonts is currently a breaking change in fea-rs...

Returning any type from write-fonts has the same effect. Raw bytes, or something close to raw bytes (map of tag to raw bytes?) would work. I don't want to have multiple write-fonts in my dependency tree but perhaps it would bumping through versions simpler. Or we could just live with it; I would expect the rate of write-fonts breaks to decline over time as it stabilizes.

} else {
debug!("No fea file, dull compile");
}
// Enables the assumption that if the file exists features were compiled
if context.flags.contains(Flags::EMIT_IR) {
fs::write(context.paths.target_file(&WorkId::Features), "1").map_err(Error::IoError)?;
}
let font = result?;
context.set_features(font);
Ok(())
}
}
35 changes: 33 additions & 2 deletions fontbe/src/font.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ use fontdrasil::orchestration::Work;
use log::debug;
use read_fonts::{
tables::{
avar::Avar, cmap::Cmap, fvar::Fvar, glyf::Glyf, gvar::Gvar, head::Head, hhea::Hhea,
hmtx::Hmtx, loca::Loca, maxp::Maxp, name::Name, os2::Os2, post::Post,
avar::Avar, cmap::Cmap, fvar::Fvar, glyf::Glyf, gpos::Gpos, gsub::Gsub, gvar::Gvar,
head::Head, hhea::Hhea, hmtx::Hmtx, loca::Loca, maxp::Maxp, name::Name, os2::Os2,
post::Post,
},
types::Tag,
TopLevelTable,
Expand Down Expand Up @@ -36,6 +37,8 @@ const TABLES_TO_MERGE: &[(WorkId, Tag, TableType)] = &[
(WorkId::Hhea, Hhea::TAG, TableType::Static),
(WorkId::Hmtx, Hmtx::TAG, TableType::Static),
(WorkId::Glyf, Glyf::TAG, TableType::Static),
(WorkId::Gpos, Gpos::TAG, TableType::Static),
(WorkId::Gsub, Gsub::TAG, TableType::Static),
(WorkId::Gvar, Gvar::TAG, TableType::Variable),
(WorkId::Loca, Loca::TAG, TableType::Static),
(WorkId::Maxp, Maxp::TAG, TableType::Static),
Expand All @@ -44,6 +47,27 @@ const TABLES_TO_MERGE: &[(WorkId, Tag, TableType)] = &[
(WorkId::Post, Post::TAG, TableType::Static),
];

fn has(context: &Context, id: WorkId) -> bool {
match id {
WorkId::Avar => context.has_avar(),
WorkId::Cmap => context.has_cmap(),
WorkId::Fvar => context.has_fvar(),
WorkId::Head => context.has_head(),
WorkId::Hhea => context.has_hhea(),
WorkId::Hmtx => context.has_hmtx(),
WorkId::Glyf => context.has_glyf_loca(),
WorkId::Gpos => context.has_gpos(),
WorkId::Gsub => context.has_gsub(),
WorkId::Gvar => context.has_gvar(),
WorkId::Loca => context.has_glyf_loca(),
WorkId::Maxp => context.has_maxp(),
WorkId::Name => context.has_name(),
WorkId::Os2 => context.has_os2(),
WorkId::Post => context.has_post(),
_ => false,
}
}

fn bytes_for(context: &Context, id: WorkId) -> Result<Vec<u8>, Error> {
let bytes = match id {
WorkId::Avar => to_bytes(context.get_avar().as_ref()),
Expand All @@ -53,6 +77,8 @@ fn bytes_for(context: &Context, id: WorkId) -> Result<Vec<u8>, Error> {
WorkId::Hhea => to_bytes(&*context.get_hhea()),
WorkId::Hmtx => context.get_hmtx().get().to_vec(),
WorkId::Glyf => context.get_glyf_loca().glyf.clone(),
WorkId::Gpos => to_bytes(&*context.get_gpos()),
WorkId::Gsub => to_bytes(&*context.get_gsub()),
WorkId::Gvar => context.get_gvar().get().to_vec(),
WorkId::Loca => context.get_glyf_loca().raw_loca.clone(),
WorkId::Maxp => to_bytes(&*context.get_maxp()),
Expand Down Expand Up @@ -81,11 +107,16 @@ impl Work<Context, Error> for FontWork {
debug!("Skip {tag} because this is a static font");
continue;
}
if !has(context, work_id.clone()) {
debug!("Skip {tag} because we don't have it");
continue;
}
debug!("Grabbing {tag} for final font");
let bytes = bytes_for(context, work_id.clone())?;
builder.add_table(*tag, bytes);
}

debug!("Building font");
let font = builder.build();
debug!("Assembled {} byte font", font.len());
context.set_font(Bytes::new(font));
Expand Down
91 changes: 43 additions & 48 deletions fontbe/src/orchestration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ use write_fonts::{
cmap::Cmap,
fvar::Fvar,
glyf::{Bbox, SimpleGlyph},
gpos::Gpos,
gsub::Gsub,
gvar::GlyphDeltas,
head::Head,
hhea::Hhea,
Expand All @@ -35,7 +37,7 @@ use write_fonts::{
variations::Tuple,
},
validate::Validate,
FontBuilder, FontWrite, OtRound,
FontWrite, OtRound,
};
use write_fonts::{from_obj::FromTableRef, tables::glyf::CompositeGlyph};

Expand All @@ -61,6 +63,8 @@ pub enum WorkId {
Fvar,
Glyf,
GlyfFragment(GlyphName),
Gpos,
Gsub,
Gvar,
GvarFragment(GlyphName),
Head,
Expand Down Expand Up @@ -346,15 +350,15 @@ pub struct Context {

// work results we've completed or restored from disk
// We create individual caches so we can return typed results from get fns
features: Arc<RwLock<Option<Arc<Vec<u8>>>>>,

glyphs: Arc<RwLock<HashMap<GlyphName, Arc<Glyph>>>>,
gvar_fragments: Arc<RwLock<HashMap<GlyphName, Arc<GvarFragment>>>>,

glyf_loca: ContextItem<GlyfLoca>,
avar: ContextItem<Avar>,
cmap: ContextItem<Cmap>,
fvar: ContextItem<Fvar>,
gsub: ContextItem<Gsub>,
gpos: ContextItem<Gpos>,
gvar: ContextItem<Bytes>,
post: ContextItem<Post>,
loca_format: ContextItem<LocaFormat>,
Expand All @@ -374,13 +378,14 @@ impl Context {
paths: self.paths.clone(),
ir: self.ir.clone(),
acl,
features: self.features.clone(),
glyphs: self.glyphs.clone(),
gvar_fragments: self.gvar_fragments.clone(),
glyf_loca: self.glyf_loca.clone(),
avar: self.avar.clone(),
cmap: self.cmap.clone(),
fvar: self.fvar.clone(),
gsub: self.gsub.clone(),
gpos: self.gpos.clone(),
gvar: self.gvar.clone(),
post: self.post.clone(),
loca_format: self.loca_format.clone(),
Expand All @@ -400,13 +405,14 @@ impl Context {
paths: Arc::from(paths),
ir: Arc::from(ir.read_only()),
acl: AccessControlList::read_only(),
features: Arc::from(RwLock::new(None)),
glyphs: Arc::from(RwLock::new(HashMap::new())),
gvar_fragments: Arc::from(RwLock::new(HashMap::new())),
glyf_loca: Arc::from(RwLock::new(None)),
avar: Arc::from(RwLock::new(None)),
cmap: Arc::from(RwLock::new(None)),
fvar: Arc::from(RwLock::new(None)),
gpos: Arc::from(RwLock::new(None)),
gsub: Arc::from(RwLock::new(None)),
gvar: Arc::from(RwLock::new(None)),
post: Arc::from(RwLock::new(None)),
loca_format: Arc::from(RwLock::new(None)),
Expand Down Expand Up @@ -437,13 +443,6 @@ impl Context {
self.paths.debug_dir()
}

fn maybe_persist(&self, file: &Path, content: &[u8]) {
if !self.flags.contains(Flags::EMIT_IR) {
return;
}
self.persist(file, content);
}

// we need a self.persist for macros
fn persist(&self, file: &Path, content: &[u8]) {
persist(file, content);
Expand All @@ -454,29 +453,6 @@ impl Context {
fs::read(self.paths.target_file(&id))
}

pub fn get_features(&self) -> Arc<Vec<u8>> {
let id = WorkId::Features;
self.acl.assert_read_access(&id.clone().into());
{
let rl = self.features.read();
if rl.is_some() {
return rl.as_ref().unwrap().clone();
}
}
let font = read_entire_file(&self.paths.target_file(&id));
set_cached(&self.features, font);
let rl = self.features.read();
rl.as_ref().expect(MISSING_DATA).clone()
}

pub fn set_features(&self, mut font: FontBuilder) {
let id = WorkId::Features;
self.acl.assert_write_access(&id.clone().into());
let font = font.build();
self.maybe_persist(&self.paths.target_file(&id), &font);
set_cached(&self.features, font);
}

fn set_cached_glyph(&self, glyph_name: GlyphName, glyph: Glyph) {
let mut wl = self.glyphs.write();
wl.insert(glyph_name, Arc::from(glyph));
Expand Down Expand Up @@ -568,6 +544,23 @@ impl Context {
rl.as_ref().expect(MISSING_DATA).clone()
}

pub fn has_glyf_loca(&self) -> bool {
let ids = [
WorkId::Glyf.into(),
WorkId::Loca.into(),
WorkId::LocaFormat.into(),
];
self.acl.assert_read_access_to_all(&ids);
{
let rl = self.glyf_loca.read();
if rl.is_some() {
return true;
}
}
ids.iter()
.all(|id| self.paths.target_file(id.unwrap_be()).is_file())
}

pub fn set_glyf_loca(&self, glyf_loca: GlyfLoca) {
let ids = [
WorkId::Glyf.into(),
Expand All @@ -586,21 +579,23 @@ impl Context {
}

// Lovely little typed accessors
context_accessors! { get_avar, set_avar, avar, Avar, WorkId::Avar, from_file, to_bytes }
context_accessors! { get_cmap, set_cmap, cmap, Cmap, WorkId::Cmap, from_file, to_bytes }
context_accessors! { get_fvar, set_fvar, fvar, Fvar, WorkId::Fvar, from_file, to_bytes }
context_accessors! { get_loca_format, set_loca_format, loca_format, LocaFormat, WorkId::LocaFormat, loca_format_from_file, loca_format_to_bytes }
context_accessors! { get_maxp, set_maxp, maxp, Maxp, WorkId::Maxp, from_file, to_bytes }
context_accessors! { get_name, set_name, name, Name, WorkId::Name, from_file, to_bytes }
context_accessors! { get_os2, set_os2, os2, Os2, WorkId::Os2, from_file, to_bytes }
context_accessors! { get_post, set_post, post, Post, WorkId::Post, from_file, to_bytes }
context_accessors! { get_head, set_head, head, Head, WorkId::Head, from_file, to_bytes }
context_accessors! { get_hhea, set_hhea, hhea, Hhea, WorkId::Hhea, from_file, to_bytes }
context_accessors! { get_avar, set_avar, has_avar, avar, Avar, WorkId::Avar, from_file, to_bytes }
context_accessors! { get_cmap, set_cmap, has_cmap, cmap, Cmap, WorkId::Cmap, from_file, to_bytes }
context_accessors! { get_fvar, set_fvar, has_fvar, fvar, Fvar, WorkId::Fvar, from_file, to_bytes }
context_accessors! { get_loca_format, set_loca_format, has_loca_format, loca_format, LocaFormat, WorkId::LocaFormat, loca_format_from_file, loca_format_to_bytes }
context_accessors! { get_maxp, set_maxp, has_maxp, maxp, Maxp, WorkId::Maxp, from_file, to_bytes }
context_accessors! { get_name, set_name, has_name, name, Name, WorkId::Name, from_file, to_bytes }
context_accessors! { get_os2, set_os2, has_os2, os2, Os2, WorkId::Os2, from_file, to_bytes }
context_accessors! { get_post, set_post, has_post, post, Post, WorkId::Post, from_file, to_bytes }
context_accessors! { get_head, set_head, has_head, head, Head, WorkId::Head, from_file, to_bytes }
context_accessors! { get_hhea, set_hhea, has_hhea, hhea, Hhea, WorkId::Hhea, from_file, to_bytes }
context_accessors! { get_gpos, set_gpos, has_gpos, gpos, Gpos, WorkId::Gpos, from_file, to_bytes }
context_accessors! { get_gsub, set_gsub, has_gsub, gsub, Gsub, WorkId::Gsub, from_file, to_bytes }

// Accessors where value is raw bytes
context_accessors! { get_gvar, set_gvar, gvar, Bytes, WorkId::Gvar, raw_from_file, raw_to_bytes }
context_accessors! { get_hmtx, set_hmtx, hmtx, Bytes, WorkId::Hmtx, raw_from_file, raw_to_bytes }
context_accessors! { get_font, set_font, font, Bytes, WorkId::Font, raw_from_file, raw_to_bytes }
context_accessors! { get_gvar, set_gvar, has_gvar, gvar, Bytes, WorkId::Gvar, raw_from_file, raw_to_bytes }
context_accessors! { get_hmtx, set_hmtx, has_hmtx, hmtx, Bytes, WorkId::Hmtx, raw_from_file, raw_to_bytes }
context_accessors! { get_font, set_font, has_font, font, Bytes, WorkId::Font, raw_from_file, raw_to_bytes }
}

fn set_cached<T>(lock: &Arc<RwLock<Option<Arc<T>>>>, value: T) {
Expand Down
4 changes: 3 additions & 1 deletion fontbe/src/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ impl Paths {

pub fn target_file(&self, id: &WorkId) -> PathBuf {
match id {
WorkId::Features => self.build_dir.join("features.ttf"),
WorkId::Features => self.build_dir.join("features.marker"),
WorkId::GlyfFragment(name) => self.glyph_glyf_file(name.as_str()),
WorkId::GvarFragment(name) => self.glyph_gvar_file(name.as_str()),
WorkId::Avar => self.build_dir.join("avar.table"),
WorkId::Glyf => self.build_dir.join("glyf.table"),
WorkId::Gsub => self.build_dir.join("gsub.table"),
WorkId::Gpos => self.build_dir.join("gpos.table"),
WorkId::Gvar => self.build_dir.join("gvar.table"),
WorkId::Loca => self.build_dir.join("loca.table"),
WorkId::LocaFormat => self.build_dir.join("loca.format"),
Expand Down
22 changes: 14 additions & 8 deletions fontc/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,22 @@ impl Args {
/// Manually create args for testing
#[cfg(test)]
pub fn for_test(build_dir: &std::path::Path, source: &str) -> Args {
fn testdata_dir() -> PathBuf {
let path = PathBuf::from("../resources/testdata")
.canonicalize()
.unwrap();
assert!(path.is_dir(), "{path:#?} isn't a dir");
path
}
// cargo test seems to run in the project directory
// VSCode test seems to run in the workspace directory
// probe for the file we want in hopes of finding it regardless
let potential_paths = vec!["./resources/testdata", "../resources/testdata"];
let source = potential_paths
.iter()
.map(PathBuf::from)
.find(|pb| pb.exists())
.unwrap()
.join(source)
.canonicalize()
.unwrap();

Args {
glyph_name_filter: None,
source: testdata_dir().join(source),
source,
emit_ir: true,
emit_debug: false,
build_dir: build_dir.to_path_buf(),
Expand Down
Loading