Rough in CPAL #1321
Rough in CPAL #1321
Conversation
| skrifa = "0.28.0" | ||
|
|
| let colors = context.ir.colors.try_get(); | ||
| // Guard clause: no colors | ||
| if !colors | ||
| .as_ref() | ||
| .map(|c| c.palettes.iter().any(|p| !p.is_empty())) | ||
| .unwrap_or_default() | ||
| { | ||
| debug!("Skip cpal; no colors"); | ||
| return Ok(()); | ||
| } | ||
| let colors = colors.unwrap(); |
There was a problem hiding this comment.
I was staring at this for a while trying to figure out how I'd write it and I think I prefer,
| let colors = context.ir.colors.try_get(); | |
| // Guard clause: no colors | |
| if !colors | |
| .as_ref() | |
| .map(|c| c.palettes.iter().any(|p| !p.is_empty())) | |
| .unwrap_or_default() | |
| { | |
| debug!("Skip cpal; no colors"); | |
| return Ok(()); | |
| } | |
| let colors = colors.unwrap(); | |
| let colors = match context.ir.colors.try_get() { | |
| Some(c) if !c.palettes.iter().all(Vec::is_empty) => c, | |
| _ => { | |
| debug!("Skip cpal; no colors"); | |
| return Ok(()); | |
| } | |
| }; |
But this makes me wonder why we are even getting here in the case where the palettes are all empty? Why don't we validate that at construction time, and just not bother setting colors at all in that case?
| let sz0 = colors.palettes[0].len(); | ||
| if colors.palettes.iter().any(|p| p.len() != sz0) { | ||
| return Err(Error::InconsistentPaletteLength( | ||
| colors.palettes.iter().map(Vec::len).collect(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
ditto here.. I would prefer to enforce our invariants earlier on, when we're constructing the palettes, instead of needing to handle the various edge cases here at the end.
| red: c.r, | ||
| green: c.g, | ||
| blue: c.b, | ||
| alpha: c.a, |
There was a problem hiding this comment.
not necessary but I tend to prefer a From impl for these kinds of conversions, it leaves the business logic a cleaner and moves the boilerplate over to somewhere out of sight.
| if color_records.len() > u16::MAX.into() { | ||
| return Err(Error::OutOfBounds { | ||
| what: "Too many CPAL colorRecords".to_string(), | ||
| value: format!("{}", color_records.len()), | ||
| }); | ||
| } |
There was a problem hiding this comment.
There should be an equivalent validation check for this generated by write-fonts, so we could omit this and let it be caught when we try to serialize the table? It's possible that this error message is more helpful though, I don't know how that write-fonts error gets propagated here.
| num_palette_entries: entries_per_palette as u16, | ||
| num_palettes: colors.palettes.len() as u16, | ||
| num_color_records: color_records.len() as u16, | ||
| color_records_array: NullableOffsetMarker::new(Some(color_records)), |
There was a problem hiding this comment.
also odd that this is nullable, it isn't described as nullable in the spec (https://learn.microsoft.com/en-us/typography/opentype/spec/cpal#cpal-version-0)
| @@ -95,6 +95,8 @@ pub enum Error { | |||
| CmapConflict(#[from] CmapConflict), | |||
| #[error("Progress stalled computing composite bbox: {0:?}")] | |||
| CompositesStalled(Vec<GlyphName>), | |||
| #[error("Inconsistent palette lengths observed: {0:?}")] | |||
There was a problem hiding this comment.
I think it's useful to note that this is a CPAL thing, so that the debugger knows where to start looking
| } | ||
|
|
||
| impl ColorPalettes { | ||
| pub fn new(mut palettes: Vec<Vec<Color>>) -> Self { |
There was a problem hiding this comment.
mentioned above, but it feels to me like this is an excellent place for us to go and do our validation:
- palettes isn't empty
- palettes all have the same length
| if palettes.len() == 1 { | ||
| palettes = vec![palettes | ||
| .into_iter() | ||
| .next() | ||
| .unwrap() | ||
| .into_iter() | ||
| .collect::<HashSet<_>>() | ||
| .into_iter() | ||
| .collect::<Vec<_>>()]; | ||
| } |
There was a problem hiding this comment.
code golf but I would prefer to write this as,
| if palettes.len() == 1 { | |
| palettes = vec![palettes | |
| .into_iter() | |
| .next() | |
| .unwrap() | |
| .into_iter() | |
| .collect::<HashSet<_>>() | |
| .into_iter() | |
| .collect::<Vec<_>>()]; | |
| } | |
| if let [one_palette] = palettes.as_mut_slice() { | |
| one_palette.sort(); | |
| one_palette.dedup(); | |
| } |
|
|
||
| /// We may in time need more than RGBA, but likely not for some years so start simple | ||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct Color { |
There was a problem hiding this comment.
we could just use the write-fonts ColorRecord type here instead of a newtype, if we wanted?
Not by any stretch a full implementation enough I'd like to checkpoint.
We build CPAL with the same set of colors as FontTools for FoldIt, though we order them differently.
Requires googlefonts/fontations#1373 so update write-fonts dep.