Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

Sketch #243

Closed
wants to merge 1 commit into from
Closed

Sketch #243

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
2 changes: 1 addition & 1 deletion fea-rs/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub use lookups::{
};
pub use opts::Opts;
pub use output::Compilation;
pub use variations::{AxisInfo, AxisLocation, VariationInfo};
pub use variations::{AxisInfo, AxisLocation, Location, VariationInfo};

#[cfg(any(test, feature = "test"))]
pub(crate) use variations::MockVariationInfo;
Expand Down
68 changes: 63 additions & 5 deletions fea-rs/src/compile/feature_writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,19 @@
use std::collections::{BTreeMap, HashMap};

use smol_str::SmolStr;
use write_fonts::tables::{
gpos::AnchorTable,
layout::{LookupFlag, PendingVariationIndex},
variations::VariationRegion,
use write_fonts::{
tables::{
gpos::AnchorTable,
layout::{LookupFlag, PendingVariationIndex},
variations::VariationRegion,
},
types::GlyphId,
};

use crate::common::{GlyphClass, MarkClass};
use crate::{
common::{GlyphClass, MarkClass},
compile::variations::Location,
};

use super::{
features::FeatureLookups,
Expand Down Expand Up @@ -93,10 +99,62 @@
.insert(class_name.into(), MarkClass { members });
}

/// Define a mark class with a variable anchor.
///
/// Roughly equivalent to `markClass glyph|class <anchor x y> @name{};`
/// [markClass](https://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#4.f)
/// with a variable anchor record.
pub fn add_mark_class_for_glyph(
Copy link
Owner

Choose a reason for hiding this comment

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

The function above is an attempt to provide this same functionality. And I agree there's room for improvement.

I find the relationship between FEA text and the actual GPOS/GSUB/GDEF tables eternally confusing. With that in mind, I'm just going to think through the differences out loud:

  • My version expects to be called once, with everything computed; your version expects to be called once per glyph, building up the class. both seem... fine.
  • My version requires you to construct an anchor table manually, and also allows for null anchors (since these are allowed in the final table, although I don't know what it means) and yours hides that; hiding it seems good
  • My version only supports glyph classes, and your version only supports individual glyphs. I took a look and under the covers I'm always just enumerating the glyph classes and cloning the anchors anyway, so I prefer your version here.
  • I agree that, overall, it would be nice to hide the details of anchor construction, but I would prefer to do this with a builder; in particular this will let us support AnchorFormat2.

&mut self,
gid: GlyphId,
default_anchor: (i16, i16),
class_name: impl Into<SmolStr>,
_anchor_variations: HashMap<Location, (i16, i16)>,
) {
// TODO: an error type
eprintln!(

Check failure on line 115 in fea-rs/src/compile/feature_writer.rs

View workflow job for this annotation

GitHub Actions / Clippy lints 

`format!` in `eprintln!` args
"TODO: {}",
format!(
"markClass {} <anchor {} {}> @{}; # TODO variable anchor",
gid,
default_anchor.0,
default_anchor.1,
class_name.into()
)
);
}

/// Setup a mark to base.
///
/// Pseudo-fea:
///
/// ```text
/// lookup name {
/// pos base base_name <anchor x y> @markclass;
/// // plus variations of anchor pos
/// }
///
/// <https://adobe-type-tools.github.io/afdko/OpenTypeFeatureFileSpecification.html#6.d>
/// ```
pub fn add_mark_base_pos(
&mut self,
lookup_name: impl Into<SmolStr>,
base_name: impl Into<SmolStr>,
class_name: impl Into<SmolStr>,
default_anchor: (i16, i16),
_anchor_variations: HashMap<Location, (i16, i16)>,
Comment on lines +139 to +145
Copy link
Owner

Choose a reason for hiding this comment

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

Here I'm a bit more sanguine. Using lookup names adds another layer of misdirection; I would prefer to have the caller construct lookups directly and then use the provided LookupId type to add lookups to a feature. My rationale here is that the whole idea of named lookups is a convenience in the FEA language, but is just a less efficient alias for a numerical ID, which we are going to need to create anyway.

Something I'm taking away from this, though, is that I think overall it would be nice if the ValueRecord and AnchorTable types used in fea-rs more naturally supported the inclusion of variation data, so I'll look into that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No objection to passing ids rather than names as long as we make acquiring an id easy. For example, perhaps here there need not be anything at all for lookup, I want a mark base, and the builder will generate a lookup to hold it.

) {
// TODO: an error type
let lookup_name = lookup_name.into();
let base_name = base_name.into();
let class_name = class_name.into();
eprintln!(" lookup {lookup_name} {{\n pos base {base_name} <anchor {} {}> @{class_name}; # TODO variable anchor;\n }} {lookup_name};", default_anchor.0, default_anchor.1);
}

/// Create a new lookup.
///
/// The `LookupId` that is returned can then be included in features via
/// the [`add_feature`] method.

Check failure on line 157 in fea-rs/src/compile/feature_writer.rs

View workflow job for this annotation

GitHub Actions / Rustfmt 

unresolved link to `add_feature`
pub fn add_lookup<T: GposSubtableBuilder>(
&mut self,
flags: LookupFlag,
Expand Down
5 changes: 3 additions & 2 deletions fea-rs/src/compile/variations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ pub trait VariationInfo {
) -> Result<(i16, Vec<(VariationRegion, i16)>), AnyError>;
}

type AnyError = Box<dyn std::error::Error>;
pub type AnyError = Box<dyn std::error::Error>;

// btreemap only because hashmap is not hashable
type Location = BTreeMap<Tag, AxisLocation>;
/// A position in variation space
pub type Location = BTreeMap<Tag, AxisLocation>;

/// A location on an axis, in one of three coordinate spaces
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
Expand Down
Loading