[skrifa] public color API #702
Conversation
Expose a color outline API in top level metadata rather than in the scale module.
| pub fn bounding_box( | ||
| &self, | ||
| location: impl Into<LocationRef<'a>>, | ||
| glyph_bounds: impl FnMut(GlyphId, &LocationRef, Transform) -> Option<BoundingBox>, |
There was a problem hiding this comment.
In trying to understand your intention, first some questions: So the difference from a "degenerate" ColorPainter implementation on the client side (to build a bounding box there) would be that the transform matrix is accumulated internally and we would call back less frequently, not for each node in the graph? Since there is no mention in the comment about what to do with compositions where the path through the graph forks, do you mean this to track the accumulated transform internally, and call back for each clip node with a fully spelled-out transform?
A previously configured, client-side transform (on the viewport for example) would be taken into account by the client multiplying that in when it receives the callback?
So the client would perform own path retrieval, combine its own possible viewport transform with the accumulated transform from the callback, and compute the bounding box of the path, with transform applied, and return that?
I do appreciate that this would disconnect the color module from the scale module - which I think make sense, as we discussed in the review of
This works as an interface, and we can shift slightly more code to the Rust side. I am trying to weigh whether this somewhat unusual API pattern is worth defining in addition to the to the paint traversal call, with which the client can compute a bounding box as well: it receives more callback and the matrix tracking would need to happen on the client side, but since we have that on the 2D graphics library side - that's doable there just as well.
There was a problem hiding this comment.
The answer to all of your questions is “yes” with the exception that this wouldn’t do any caching of paths internally although the client could certainly do so if they’d like to trade space for time. Seems to be a point in favor of this approach?
I hadn’t considered a client provided root transform. We can accept that as a parameter here and fold it in if you think that’d be more useful. Otherwise, yes, a client could apply it in the callback.
I have two general concerns:
-
Handling edge cases (composition nodes, detecting unbounded subgraphs) is not exactly trivial so it would be nice to offer a correct implementation of this instead of pushing that burden to all potential clients.
-
While not a top priority atm, I’m trying to keep the embedded case in mind wrt heap allocations. This interface would allow the caller to decide how to handle memory requirements for loading glyphs. Aside: that does leave the recursion blacklist as a potential problem— Rod and I discussed adding a cfg feature that uses a max recursion depth instead but that’s not something we need to address now.
| } | ||
|
|
||
| /// Returns the color outline for the given glyph identifier. | ||
| pub fn get(&self, glyph_id: GlyphId) -> Option<ColorOutline<'a>> { |
There was a problem hiding this comment.
I think the direct access is more important than the iterator - so perhaps we can do without the maxp() access?
As discussed, in googlefonts/oxidize#48 - we need the (cheaply acquirable) information in Skia whether a glyph is COLRv0 and COLRv1. I think that's doable well with the type-based distinction of ColorOutlineKind.
There was a problem hiding this comment.
I’d like to keep the iterator but I can implement it without maxp. Good catch.
There was a problem hiding this comment.
Re detection: I can add a version() method to ColorOutline that returns 0 or 1
| painter: &mut impl ColorPainter, | ||
| ) -> Result<(), ReadError> { | ||
| let instance = instance::ColrInstance::new(self.colr.clone(), location.into().coords()); | ||
| match &self.kind { |
There was a problem hiding this comment.
I am aware this is a draft - but how where does self.kind get initialized here - that would be a ColorOutlineKind retrieved for self.glyph_id, right?
There was a problem hiding this comment.
The kind field is initialized in ColorOutlineCollection::get. Yes, it is the outline for the associated glyph_id. I added that field (and accessor) to support iteration since the table can be sparse. It might make sense to remove that and change the iterator to yield a tuple (GlyphId, ColorOutline)
| @@ -47,6 +48,11 @@ pub trait MetadataProvider<'a>: raw::TableProvider<'a> + Sized { | |||
| fn charmap(&self) -> Charmap<'a> { | |||
| Charmap::new(self) | |||
| } | |||
|
|
|||
| /// Returns the collection of color outlines. | |||
| fn color_outlines(&self) -> ColorOutlineCollection<'a> { | |||
There was a problem hiding this comment.
Yes, I think this works as a good entry point. And is perhaps a bit more idiomatic than just a "do_traverse_paint" function that takes a glyph id somewhere. It also answers the "what glyph format is available" question.
There was a problem hiding this comment.
Thanks. I’m trying to keep the API as consistent as possible. Fonts don’t make it easy 🤷
- add version() method to ColorOutline - remove maxp lookup and change iterator to use max glyph in COLR table - remove glyph_id from ColorOutline and change iterator to return (GlyphId, ColorOutline) tuple
#697 (comment) requests moving the color instance code from the scale module to the top level "metadata section" of the crate. This PR contains a proposal for such an API that meshes well with the current design of
MetadataProvider.This depends on #697 but is pulled out here for separate discussion.
One key point is the method for bounding box computation. It'd be nice to avoid a hard dependency on anything in the
scalemodule so the function here takes a closure for that purpose.@drott thoughts? There's some overlap with your
ColorPainterwork but I think this provides a nice entry point for that.