search

yeslogic / allsorts

Font parser, shaping engine, and subsetter implemented in Rust
https://yeslogic.com/blog/allsorts-rust-font-shaping-engine/
Apache License 2.0
706 stars 23 forks source link

Fix #39 #40

Closed fschutt closed 3 years ago

fschutt commented 3 years ago

NOTE: The code for .map_glyph is duplicated, which isn't great. I can't create a ReadArray<T> from a Vec<T>, so I can only either duplicate the code or use a macro.

wezm commented 3 years ago

I'm not sure I really want is_mark to be public again. Ideally it would remain an implementation detail. What was your use case for making it public?

Did you have a particular need for making the various types Copy? Typically I reserve Copy for smaller types (that fit in a register or two) to avoid surprise memcpys.

Here are the sizes (in bytes) of some of the types: HeadTable = 56, TableRecord = 16 HheaTable = 22, MaxpTable = 30, MaxpVersion1SubTable = 26

fschutt commented 3 years ago

I'm okay with not implementing Copy, but I need them to be at least Clone, because my implementation doesn't store the entire font, it only stores a few tables. So I can either parse the tables again or I could simply clone them from the font provider.

My usecase for is_mark is that I want to switch the positioning between MarkPlacement and Placement. I can make it a function that returns true on MarkPlacement::None, it's just that I wanted my code to build yesterday evening

wezm commented 3 years ago

My usecase for is_mark is that I want to switch the positioning between MarkPlacement and Placement.

So after going through the process of adding positioning info to the allsorts-tools shape command I don't think it's quite this simple: if a mark is attached to a base glyph with Placement::Distance then the mark also needs to be adjusted by that distance:

https://github.com/yeslogic/allsorts-tools/pull/14/files#diff-f686acba59f5e44ba9e3177784a8c42d45806e266474c5540305fd33dad01f40R64-R70

wezm commented 3 years ago

I merged a number of these changes manually. I had to drop the derived Debug and PartialEq impls on the cmap types as these don't work in Rust 1.38.0 (due to the Box<[u8; 256]>), which is the minimum version we support. Are these required for your use case?

fschutt commented 3 years ago

No, not required, I just added it because it does make things easier for testing. Clone is the most important trait.

Thanks for mergin.