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

Jump to bottom

Explicitly distinguish design and user coordinates and locations #47

Closed
wants to merge 1 commit into from
Closed

Explicitly distinguish design and user coordinates and locations #47

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
6 changes: 3 additions & 3 deletions fontir/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{error, io, path::PathBuf};

use thiserror::Error;

use crate::ir::DesignSpaceLocation;
use crate::coords::UserSpaceLocation;

#[derive(Debug, Error)]
pub enum Error {
Expand All @@ -27,9 +27,9 @@ pub enum Error {
#[error("Unexpected state encountered in a state set")]
UnexpectedState,
#[error("Duplicate location for {what}: {loc:?}")]
DuplicateLocation {
DuplicateUserSpaceLocation {
what: String,
loc: DesignSpaceLocation,
loc: UserSpaceLocation,
},
#[error("Global metadata very bad, very very bad")]
InvalidGlobalMetadata,
Expand Down
39 changes: 22 additions & 17 deletions fontir/src/ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
//! Serde types for font IR.

use crate::{error::Error, serde::StaticMetadataSerdeRepr};
use ordered_float::OrderedFloat;
use crate::{
coords::{UserSpaceCoord, UserSpaceLocation},
error::Error,
serde::StaticMetadataSerdeRepr,
};
use serde::{Deserialize, Serialize};
use std::collections::{BTreeMap, HashMap};
use std::collections::HashMap;

/// Global font info that cannot vary.
///
Expand Down Expand Up @@ -42,16 +45,12 @@ impl StaticMetadata {
pub struct Axis {
pub name: String,
pub tag: String,
pub min: OrderedFloat<f32>,
pub default: OrderedFloat<f32>,
pub max: OrderedFloat<f32>,
pub min: UserSpaceCoord,
pub default: UserSpaceCoord,
pub max: UserSpaceCoord,
rsheeter marked this conversation as resolved.
Show resolved Hide resolved
pub hidden: bool,
}

// Using BTreeMap instead of HashMap and OrderedFloat instead of f32 so that
// the location is hashable and can be used as a key in Glyph::sources HashMap
pub type DesignSpaceLocation = BTreeMap<String, OrderedFloat<f32>>;

/// A variable definition of a single glyph.
///
/// Defined in at least once position in designspace. If defined in
Expand All @@ -60,7 +59,7 @@ pub type DesignSpaceLocation = BTreeMap<String, OrderedFloat<f32>>;
#[derive(Serialize, Deserialize, Debug, PartialEq)]
pub struct Glyph {
pub name: String,
pub sources: HashMap<DesignSpaceLocation, GlyphInstance>,
Copy link
Member

Choose a reason for hiding this comment

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

actually I think for these we don't want UserSpaceLocation. Sources in designspace are always in design-space, not user-space. Instances can avail themselves of being optionally defined in user-space, but sources (aka the designer's "masters") are always design locations because that's what the designer works with, and nobody requested to define sources with user-space location so far.

So I don't think it helps anything to have Glyph::sources keyed by user-space location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had in mind to leave designspace behind when creating IR and work exclusively in user and normalized space. Is that a bad idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder for self, per f2f I believe the user of internal coordinates was acceptable here.

pub sources: HashMap<UserSpaceLocation, GlyphInstance>,
}

impl Glyph {
Expand All @@ -73,11 +72,11 @@ impl Glyph {

pub fn try_add_source(
&mut self,
unique_location: &DesignSpaceLocation,
unique_location: &UserSpaceLocation,
source: GlyphInstance,
) -> Result<(), Error> {
if self.sources.contains_key(unique_location) {
return Err(Error::DuplicateLocation {
return Err(Error::DuplicateUserSpaceLocation {
what: format!("glyph '{}' source", self.name),
loc: unique_location.clone(),
});
Expand Down Expand Up @@ -149,15 +148,21 @@ pub struct Affine2x3 {

#[cfg(test)]
mod tests {
use crate::ir::Axis;
use ordered_float::OrderedFloat;

use crate::{coords::UserSpaceCoord, ir::Axis};

fn user_coord(v: f32) -> UserSpaceCoord {
UserSpaceCoord::new(OrderedFloat(v))
}

fn test_axis() -> Axis {
Axis {
name: String::from("Weight"),
tag: String::from("wght"),
min: 100_f32.into(),
default: 400_f32.into(),
max: 900_f32.into(),
min: user_coord(100_f32),
default: user_coord(400_f32),
max: user_coord(900_f32),
hidden: false,
}
}
Expand Down
5 changes: 5 additions & 0 deletions glyphs2fontir/src/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use fontir::coords::{temporary_design_to_user_conversion, DesignSpaceCoord};
use fontir::error::{Error, WorkError};
use fontir::ir;
use fontir::ir::{Axis, StaticMetadata};
Expand Down Expand Up @@ -195,6 +196,10 @@ impl Work for StaticMetadataWork {
.unwrap();
let default = OrderedFloat::<f32>(defaults[idx].into_inner() as f32);

let min = temporary_design_to_user_conversion(DesignSpaceCoord::new(min));
let max = temporary_design_to_user_conversion(DesignSpaceCoord::new(max));
let default = temporary_design_to_user_conversion(DesignSpaceCoord::new(default));

Axis {
name: a.name.clone(),
tag: a.tag.clone(),
Expand Down
56 changes: 33 additions & 23 deletions ufo2fontir/src/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ use std::{
};

use fontir::{
coords::{
temporary_design_to_user_conversion, DesignSpaceCoord, UserSpaceCoord, UserSpaceLocation,
},
error::{Error, WorkError},
ir::{Axis, DesignSpaceLocation, StaticMetadata},
ir::{Axis, StaticMetadata},
orchestration::Context,
source::{Input, Source, Work},
stateset::{StateIdentifier, StateSet},
};
use log::{debug, warn};
use log::debug;
use norad::designspace::{self, DesignSpaceDocument};
use ordered_float::OrderedFloat;

Expand Down Expand Up @@ -40,7 +43,7 @@ impl DesignSpaceIrSource {
// A cache of locations, valid provided no global metadata changes
struct Cache {
global_metadata_sources: StateSet,
locations: HashMap<PathBuf, Vec<DesignSpaceLocation>>,
locations: HashMap<PathBuf, Vec<UserSpaceLocation>>,
glyph_names: Arc<HashSet<String>>,
designspace_file: PathBuf,
designspace: Arc<DesignSpaceDocument>,
Expand All @@ -50,7 +53,7 @@ impl Cache {
fn new(
global_metadata_sources: StateSet,
glyph_names: HashSet<String>,
locations: HashMap<PathBuf, Vec<DesignSpaceLocation>>,
locations: HashMap<PathBuf, Vec<UserSpaceLocation>>,
Copy link
Member

Choose a reason for hiding this comment

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

so yeah that's incorrect, the locations of sources in designspace (in this particular case, locations contains the location of each glif file) are in design-space coordinates, not user-space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My rationale was that for IR generation the userspace location would be what you want?

designspace_file: PathBuf,
designspace: DesignSpaceDocument,
) -> Cache {
Expand All @@ -67,7 +70,7 @@ impl Cache {
self.global_metadata_sources == *global_metadata_sources
}

fn location_of(&self, glif_file: &Path) -> Option<&Vec<DesignSpaceLocation>> {
fn location_of(&self, glif_file: &Path) -> Option<&Vec<UserSpaceLocation>> {
self.locations.get(glif_file)
}
}
Expand Down Expand Up @@ -189,7 +192,7 @@ impl Source for DesignSpaceIrSource {

// UFO filename => map of layer
let mut layer_cache = HashMap::new();
let mut glif_locations: HashMap<PathBuf, Vec<DesignSpaceLocation>> = HashMap::new();
let mut glif_locations: HashMap<PathBuf, Vec<UserSpaceLocation>> = HashMap::new();
let mut glyph_names = HashSet::new();

let Some((default_master_idx, default_master)) = default_master(&designspace) else {
Expand Down Expand Up @@ -319,14 +322,11 @@ struct StaticMetadataWork {
glyph_names: Arc<HashSet<String>>,
}

fn default_master(designspace: &DesignSpaceDocument) -> Option<(usize, &designspace::Source)> {
// The master at the default location on all axes
// TODO: fix me; ref https://github.com/googlefonts/fontmake-rs/issues/22
warn!("Using an incorrect algorithm to determine the default master");
let default_location: DesignSpaceLocation = designspace
fn default_master(designspace: &DesignSpaceDocument) -> Option<&designspace::Source> {
let default_location: UserSpaceLocation = designspace
.axes
.iter()
.map(|a| (a.name.clone(), OrderedFloat::from(a.default)))
.map(|a| (a.name.clone(), UserSpaceCoord::new(OrderedFloat(a.default))))
.collect();
designspace
.sources
Expand Down Expand Up @@ -386,6 +386,11 @@ fn glyph_order(
Ok(glyph_order)
}

fn design_to_user(design_coord: f32) -> UserSpaceCoord {
let design_coord = DesignSpaceCoord::new(OrderedFloat(design_coord));
temporary_design_to_user_conversion(design_coord)
}

impl Work for StaticMetadataWork {
fn exec(&self, context: &Context) -> Result<(), WorkError> {
debug!("Static metadata for {:#?}", self.designspace_file);
Expand All @@ -397,9 +402,9 @@ impl Work for StaticMetadataWork {
name: a.name.clone(),
tag: a.tag.clone(),
hidden: a.hidden,
min: a.minimum.unwrap().into(),
default: a.default.into(),
max: a.maximum.unwrap().into(),
min: design_to_user(a.minimum.unwrap()),
default: design_to_user(a.default),
max: design_to_user(a.maximum.unwrap()),
})
.collect();

Expand All @@ -415,7 +420,7 @@ impl Work for StaticMetadataWork {

struct GlyphIrWork {
glyph_name: String,
glif_files: HashMap<PathBuf, Vec<DesignSpaceLocation>>,
glif_files: HashMap<PathBuf, Vec<UserSpaceLocation>>,
}

impl Work for GlyphIrWork {
Expand All @@ -442,8 +447,10 @@ mod tests {
path::{Path, PathBuf},
};

use fontir::ir::DesignSpaceLocation;
use fontir::source::{Input, Source};
use fontir::{
coords::{UserSpaceCoord, UserSpaceLocation},
source::{Input, Source},
};
use norad::designspace;
use ordered_float::OrderedFloat;

Expand Down Expand Up @@ -502,17 +509,17 @@ mod tests {
}

fn add_location(
add_to: &mut HashMap<PathBuf, Vec<DesignSpaceLocation>>,
add_to: &mut HashMap<PathBuf, Vec<UserSpaceLocation>>,
glif_file: &str,
axis: &str,
pos: f32,
) {
add_to
.entry(testdata_dir().join(glif_file))
.or_default()
.push(DesignSpaceLocation::from([(
.push(UserSpaceLocation::from([(
axis.to_string(),
OrderedFloat(pos),
UserSpaceCoord::new(OrderedFloat(pos)),
)]));
}

Expand Down Expand Up @@ -578,8 +585,11 @@ mod tests {
pub fn find_default_master() {
let (source, _) = test_source();
let ds = source.load_designspace().unwrap();
let mut expected = DesignSpaceLocation::new();
expected.insert("Weight".to_string(), 400_f32.into());
let mut expected = UserSpaceLocation::new();
expected.insert(
"Weight".to_string(),
UserSpaceCoord::new(OrderedFloat(400_f32)),
);
assert_eq!(
expected,
to_ir_location(&default_master(&ds).unwrap().1.location)
Expand Down
48 changes: 31 additions & 17 deletions ufo2fontir/src/toir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
use std::collections::HashMap;
use std::path::PathBuf;

use fontir::ir;
use fontir::{
coords::{
temporary_design_to_user_conversion, DesignSpaceCoord, UserSpaceCoord, UserSpaceLocation,
},
ir,
};
use norad::designspace::{self, DesignSpaceDocument, Dimension};
use ordered_float::OrderedFloat;

use crate::error::Error;

// TODO we will need the ability to map coordinates and a test font that does. Then no unwrap.
pub(crate) fn to_ir_location(loc: &[Dimension]) -> ir::DesignSpaceLocation {
pub(crate) fn to_ir_location(loc: &[Dimension]) -> UserSpaceLocation {
loc.iter()
.map(|d| (d.name.clone(), OrderedFloat(d.xvalue.unwrap())))
.map(|d| {
// TODO: what if d has uservalue (new in DS5.0)
let coord = DesignSpaceCoord::new(OrderedFloat(d.xvalue.unwrap()));
let coord = temporary_design_to_user_conversion(coord);
(d.name.clone(), coord)
})
.collect()
}

Expand All @@ -22,19 +32,18 @@ pub fn designspace_to_ir(designspace: DesignSpaceDocument) -> Result<Vec<ir::Axi
Ok(ir_axes)
}

fn design_to_user(value: f32) -> UserSpaceCoord {
let coord = DesignSpaceCoord::new(OrderedFloat(value));
temporary_design_to_user_conversion(coord)
}

fn to_ir_axis(axis: designspace::Axis) -> ir::Axis {
ir::Axis {
name: axis.name,
tag: axis.tag,
min: axis
.minimum
.expect("Discrete axes not supported yet")
.into(),
default: axis.default.into(),
max: axis
.maximum
.expect("Discrete axes not supported yet")
.into(),
min: design_to_user(axis.minimum.expect("Discrete axes not supported yet")),
default: design_to_user(axis.default),
max: design_to_user(axis.maximum.expect("Discrete axes not supported yet")),
hidden: axis.hidden,
}
}
Expand Down Expand Up @@ -86,7 +95,7 @@ fn to_ir_glyph_instance(glyph: &norad::Glyph) -> ir::GlyphInstance {

pub fn to_ir_glyph<S>(
glyph_name: S,
glif_files: &HashMap<PathBuf, Vec<ir::DesignSpaceLocation>>,
glif_files: &HashMap<PathBuf, Vec<UserSpaceLocation>>,
) -> Result<ir::Glyph, Error>
where
S: Into<String>,
Expand All @@ -104,10 +113,15 @@ where
#[cfg(test)]
mod tests {
use norad::designspace::DesignSpaceDocument;
use ordered_float::OrderedFloat;
use std::path::Path;

use crate::toir::designspace_to_ir;
use fontir::ir;
use fontir::{coords::UserSpaceCoord, ir};

fn user_coord(v: f32) -> UserSpaceCoord {
UserSpaceCoord::new(OrderedFloat(v))
}

#[test]
fn simple_wght_variable() {
Expand All @@ -116,9 +130,9 @@ mod tests {
vec![ir::Axis {
name: "Weight".to_string(),
tag: "wght".to_string(),
min: 400_f32.into(),
default: 400_f32.into(),
max: 700_f32.into(),
min: user_coord(400_f32),
default: user_coord(400_f32),
max: user_coord(700_f32),
hidden: false
}],
designspace_to_ir(ds).unwrap()
Expand Down