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.

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

To what extent, if any, should norad collaborate with MFEK/glifparser.rlib? #107

Open
ctrlcctrlv opened this issue Apr 18, 2021 · 20 comments

Comments

@ctrlcctrlv
Copy link
Contributor

Hello,

I just got done writing glifparser v1.0.

glifparser was written primarily for MFEKglif but it's also used in MFEKstroke, and in MFEKufo (unreleased) for previewing glyphs.

It's currently in mfek branch, awaiting @MatthewBlanchard and I to patch MFEKglif and MFEKstroke to support the changes. The biggest change is that there are no more panic's, originally, I just panicked via .expect(...) in a lot of cases because I knew MFEKglif was the only consumer and had a custom panic unwinder that would pretty print the message in red and make it clear what went wrong.

glifparser can do quite a few things norad cannot:

  • "flatten" a glyph with components to its constituent contours, detect loops and bail out (uses kurbo::Affine!)
  • allow arbitrary data storage along with points via generic type attached to points
  • most importantly for MFEKglif project, deal with unattached .glif files that aren't part of a parent UFO
  • convert glif to SkPath (Skia path), piecewise contour (needs MFEK/math.rlib for this one)
  • load image data if glif has a parent UFO and image exists

I was thinking, maybe norad can support using glifparser at user option, via compile-time feature. Right now what I'm doing is, using norad with the UfoDataRequest I added and just not reading glyphs with it, using it for metadata only.

I think closer collaboration is possible. Is it desirable?

@cmyr
Copy link
Member

cmyr commented Apr 18, 2021

I definitely think it's possible, and some of these items seem like pretty obvious fits. Let me think in point form:

  • glyph flattening: @madig has been playing around with this. I think that there's a case to put it in norad, since it's useful in a lot of cases.
  • arbitrary data storage along with points: glyphs have a lib type which norad exposes as an arbitrary plist attached to the lib. I need to improve serde support upstream, but I've been thinking about API that lets you assign any type that impls Serialize & Deserialize to the lib (or a key on the lib) which would come pretty close to supporting your use-case? There is the downside that modifying that data will require a serialization roundtrip, but I'm not sure if this is a major concern in practice.
  • unattached glifs: I might need more info here, but you can already use Glyph::load to load a single glif file? Are there other additional functionality you need?
  • convert to SkPath: this is the one I'm least sure of. There are lots of possible path representations, and I don't think we will want to support all of them, although I could imagine a skia feature that adds From impls for paths, affines, etc?
  • I'm not really sure about this because I haven't had to do it yet).
  • Load image data: I'm going back and forth on this. The user may have particular crates or types that they want to use to store/represent images? The most general thing we could do is load the data but just hand it off as a bag of bytes + a filename and let the user deal with it, but there may be compelling alternatives, and I'm happy to consider them.

So yes, I agree that collaboration on most of these points sounds very reasonable, with only a few items I'm not sure about.

@madig
Copy link
Collaborator

madig commented Apr 19, 2021

(I wrote some code to decompose a composite: https://github.com/linebender/norad/blob/add-example-letterspacer/examples/letterspacer.rs#L474-L521)

I haven't looked at MFEK but maybe capabilities can be merged over?

@ctrlcctrlv
Copy link
Contributor Author

It doesn't look like that code checks if there's circular references, making my version safer.

@ctrlcctrlv
Copy link
Contributor Author

About the image data, my glifparser.rlib basically does just that: gives you a Vec<u8>. I also figure out for you if it's a PNG (because if it's not, that violates the UFO glif spec; if the consumer cares about such things, they can bail right here). But I don't try to do much more than that. However, because the UFO glif spec has language around images being able to be colored, I will eventually be handling that, but only for PNG files. I'll add a PNG crate to glifparser and then, if it's an image with color set, handle the coloring for you and give you a function like Image::bitmap() you can call. Right now, Image::data() just gives you a Vec<u8> though that you need to figure out what to do with.

@madig
Copy link
Collaborator

madig commented Apr 19, 2021

It doesn't look like that code checks if there's circular references, making my version safer.

Cool. Yes, neither norad nor my code currently deal with malformed component trees.

@ctrlcctrlv ctrlcctrlv mentioned this issue Apr 19, 2021
@cmyr
Copy link
Member

cmyr commented Apr 19, 2021

Cool, doing raw image data works for me, and then if we wanted to use something like the Image crate we could put that behind a feature flag.

@madig
Copy link
Collaborator

madig commented Apr 19, 2021

I think ufoLib also passes back bytes and no Image object or anything 🤔

@ctrlcctrlv
Copy link
Contributor Author

ctrlcctrlv commented Apr 19, 2021

Well, obviously the degree people implement the spec varies, but the spec says:

Coloring Images

There are two places that the color for an image can be defined: the color attribute of the image element and the color defined in layerinfo.plist. The color defined in the image element’s color attribute always takes precedence. If that is not defined and the color is defined for the layer, the layer’s color should be used. If both of these are undefined, the image’s colors, without modification, should be used unless an authoring tool has a default color for images. If a color is to be applied, the authoring tool should convert the image to grayscale and then apply the color. This modified version of the image must not be saved into the images directory.

So, I have no clue how you can sanely do this without parsing the PNG, reading a bitmap, and clobbering the pixels.

I suppose, technically, you could implement it insanely another way:

  1. if IHDR is Color Type 3 (indexed colors), skip to step 3
  2. read all the unique colors, create a palette, assign each pixel one of the unique colors and overwrite the data block
  3. convert the palette to the new color
  4. rewrite IHDR

Then you can just return back the modified PNG data which you've mangled in memory. But that seems crazy to me, to do that and not just return a bitmap.

@cmyr
Copy link
Member

cmyr commented Apr 19, 2021

I guess this depends on goals? I assume the whole rationale for this is that some authoring tool (robofont?) wanted to be able to use grayscale images as masks, and that ended up being baked into the spec. I would be personally happy to just return the image data and the color information, and then let the user do whatever they want with that.

Put another way: per the spec, I would not consider norad to be "the authoring tool".

@ctrlcctrlv
Copy link
Contributor Author

ctrlcctrlv commented Apr 19, 2021

@cmyr It is as you say, a matter of goals.

For example, for me, glifparser.rlib is tightly coupled to MFEK project, mostly MFEKglif, but everything else too. Converting back and forth between Skia paths is therefore absolutely a necessity—I put it behind a skia feature tag and a trait, but obviously all of my software is going to want that tag and trait. And certainly, for me, it makes no sense to do this coloring in MFEKglif. MFEKufo is going to need it to display properly glyph previews of the whole font. MFEKmetrics is going to need it when it's ready. MFEKinterpolate will probably need it, but might not display images at all. For me to just copy therefore the MFEKGlif type isn't feasible. Maybe some day it'll go into its own library, but then again, a feature tag seems fine to me. It's MFEK/glifparser.rlib, not ctrlcctrlv/glifparser.rlib :-) While I'm working on making it so people can ignore our features which don't apply to all use cases, like skia and mfek, obviously, I'm not going to cut them out of the source tree for no appreciable gain, and added confusion around naming. (MFEK/glifparser-mfek.rlib? Yech.)

glifparser.rlib also has a type I didn't talk about since ① it's so new and ② not related to norad, but which illustrates the point: MFEKGlif. That's behind mfek feature tag, and its job is to read the MFEK private library, as well as our items in <lib> (we have some in both. Typically our items go in our private library, but when we implement a third pseudo-standard key, like contour names, which robofont came up with, we put that in the general <lib>), which will never have a use outside MFEK probably. Things like open contour stroke (variable-width stroking, pattern along path, etc. definitions), our internal glif layers, (we consider a glif file as itself having layers, but these are private layers that won't appear in output, used for things like tracing a scan from a background image or keeping the source contours for a boolean operation) etc. MFEKGlif (note "G") type therefore would never find its way into Norad core, parsing our private library is totally useless to general .glif file API's.

@davelab6
Copy link

I would be personally happy to just return the image data and the color information, and then let the user do whatever they want with that.

However, since the spec mandates what should be done, it makes sense to me a ufo library will contain a standard compliant implementation that every app will need and benefit from being the same. Of course apps can do more, if they wish

@ctrlcctrlv
Copy link
Contributor Author

ctrlcctrlv commented May 12, 2021

@davelab6 This is now implemented in glifparser.

image

There's a glifparser::Image type. It has two functions, load and decode, and a field called data which is a DataOrBitmap, a self-explanatory enum.

If you call load, data will be just the bytes.

If you call decode after load, data will be a struct Bitmap{pixels: Vec<u8>, width, height}. pixels are guaranteed to be RGBA8888.

If the glifparser::Image has a color, decode will apply the color.

If the application changes the color, it can call load and decode again to get a new bitmap.

Changing the color is up to the application, but applying the color is obviously the job of the UFO library, since the spec describes how it should be applied, and applications aren't free to just apply colors however they wish.

@ctrlcctrlv
Copy link
Contributor Author

I updated the README file of glifparser.rlib recently to reflect its status and the space it takes up in the ecosystem: https://github.com/MFEK/glifparser.rlib

@madig
Copy link
Collaborator

madig commented Dec 19, 2021

I still haven't found the time to sift through glifparser etc. to see if there's anything to steal... one day!

@ctrlcctrlv
Copy link
Contributor Author

In my opinion, most obviously stealable parts are:

  • Components handling code which is safe because it uses trees
  • Image handling code which supports coloring images, part of UFO standard
  • General philosophy surrounding Norad Glyph (glifparser Glif) being a toplevel type and not a subordinate type

@madig
Copy link
Collaborator

madig commented Dec 22, 2021

Hm yes, points 1 sounds interesting, point 2 I need to review (what does norad not support?), but point 3 I'm not sure about. What specifically do you need to have free-standing Glyphs?

@cmyr
Copy link
Member

cmyr commented Dec 24, 2021

Point three is interesting; there are various projects (such as RoboCJK) that are starting to use the .glif format but outside the context of a normal UFO. That said, the current Glyph in norad works fine as a top-level type, or at least should... I don't use it like this anywhere so I'm not totally sure, but it has (for instance) a public load/save API.

Maybe one possible limitation is that it might not serialize the lib, since normally that's serialized at the layer-level?

@ctrlcctrlv
Copy link
Contributor Author

Free standing glyphs are normal in font development which is centered around UFO format. There are so many cases you might want them I couldn't even begin to list them all.

Regarding point two, your Image type is not like ours, ours is a created type that can happen any time after parse time. You get from us at parse time a GlifImage very similar to your Image. Our Image is guaranteed valid, contains the data of the image, and also applies the color to the image data…if there's any reason why the GlifImage can't be converted you get back an error and not an Image. As long as you can get a bitmap though, the coloring is an infallible operation since it just involves math on the pixel color values.

lib not being serialized would be quite a big issue for me yeah.

@ctrlcctrlv
Copy link
Contributor Author

Also, my Image type supports a lot of codecs, but it warns you if you're doing something out-of-spec:

https://github.com/MFEK/glifparser.rlib/blob/bc8376f1a575dfdeb49a7b8fef6d1053c2a29807/src/image.rs#L71-L73

        if codec != ImageCodec::PNG {
            warn!("Image not PNG! `fontmake` and other UFO spec conformant progams will refuse to embed the bitmap. MFEKglif may still be able to display it, however, so only use it for proofing, not output.");
        }

I support BMP, GIF, and (most importantly) JPEG and WebP besides the UFO spec demand of PNG.

The reason I support all these codecs is that people want to trace from books usually and storing many hicolor 600dpi scans losslessly in a font repo is not logical.

@ctrlcctrlv
Copy link
Contributor Author

Btw my codec matcher might not be the best but I haven't found any valid files it fails on:

        let codec = match self.data.unwrap_data().chunks(12).next() {
            Some(&[0x42, 0x4D, _, _, _, _, _, _, _, _, _, _]) => ImageCodec::BMP,
            Some(&[0xFF, 0xD8, _, _, _, _, _, _, _, _, _, _]) => ImageCodec::JPEG,
            Some(&[0x89, 0x50, 0x4E, 0x47, _, _, _, _, _, _, _, _]) => ImageCodec::PNG,
            Some(&[0x47, 0x49, 0x46, 0x38, _, _, _, _, _, _, _, _]) => ImageCodec::GIF,
            Some(&[0x52, 0x49, 0x46, 0x46, _, _, _, _, 0x57, 0x45, 0x42, 0x50]) => ImageCodec::WebP,
            _ => ImageCodec::Unknown
        };

If it gets it wrong, then API will just return back an Err(e) with the reason when it tries to use .with_guessed_format() from image crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants