RazrFalcon / rustybuzz

A complete harfbuzz's shaping algorithm port to Rust
MIT License
498 stars 34 forks source link

Sync to 7.3.0 #114

Closed LaurenzV closed 1 week ago

LaurenzV commented 1 week ago

A couple of points I wasn't sure about / need to discuss, but other than that nothing crazy, luckily:

Status Commit message HB
🟢 [MarkBase] Adjust base-finding logic Link
🟢 [GPOS] Fix indexing in MarkLigPos Link
🟡 [GSUB] Support SingleSubst in get_glyph_alternates Link I guess not relevant? seems more like an API thing, not actually relevant for shaping
🟢 Add Tifinagh to list of both-directions scripts Link
🟢 [indic] Tighten up base-finding Link
🟢 [syllabic] Actually clear syllables Link
🟡 [syllabic] Better fix for previous issue Link unsure about this one, our skippy iter only has syllable but not a bool "per_syllable">
🟢 [match_input] Micro-optimize Link
🟢 [PairPos2] Micro-optimize Link
🟡 [PairPosFormat2] Micro-optimize and don't kern if class2=0 Link not sure I can really port this... I remember we ported this in the past and it was a huge pain because ttf-parser doesn't give that much low-level access
🟡 [Ligature] Speed up Link do we need to port this since it's behind a compilation flag? probably easier to ignore it
🟡 [Ligature] Minor tweak to recent code Link
🟡 [Ligature] Micro-optimize Link
🟡 [Ligature] Micro-optimize more Link same as above
🟡 [Ligature] Use slow path if 2 or fewer ligatures Link
🟡 [GPOS] Optimize iterator reset Link we dont have the whole iter thing with init and reset right?
🟡 [layout] Don't init iters successively multiple times Link same
RazrFalcon commented 1 week ago

[GSUB] Support SingleSubst in get_glyph_alternates Link I guess not relevant? seems more like an API thing, not actually relevant for shaping

Yep.

[syllabic] Better fix for previous issue Link unsure about this one, our skippy iter only has syllable but not a bool "per_syllable">

Looks like it was added in v4: https://github.com/harfbuzz/harfbuzz/commit/044d7a06db552e1564b8575f4d23798f009d9dde We should add it too. Strange that it works at all without it...

[PairPosFormat2] Micro-optimize and don't kern if class2=0 Link not sure I can really port this... I remember we ported this in the past and it was a huge pain because ttf-parser doesn't give that much low-level access

Let's ignore.

[Ligature] Speed up Link do we need to port this since it's behind a compilation flag? probably easier to ignore it

Looks like it was changed quite a bit in the master. Nice to have, but can leave it for later. Let's add a comment/todo in Rust code explaining why it is different.

[GPOS] Optimize iterator reset Link we dont have the whole iter thing with init and reset right?

No idea... Seems like we never reset a skipping iterator.

LaurenzV commented 1 week ago

Hm, yeah there is definitely something fishy... So this per_syllable thing is only used in the set_syllable function, which in turn is only used in the reset function of the iterator... which for some reason, we don't have, along with init. I searched for its usages, and there are indeed differences in how we interact with the iterator throughout the code. Not sure why it is that way. I guess for now I'll just add a TODO.

LaurenzV commented 1 week ago

There are some more changes regarding how skippy iterators are used, so I'll try to align it in the next PR.

RazrFalcon commented 1 week ago

Got it. I remember it was problematic to implement skipping iterator. Probably because of borrow checker. So no surprises it is different. But per_syllable was still added after my initial port.

LaurenzV commented 1 week ago

I can see why now... the context itself contains a skipping iterator iter_input, and the skipping iterator itself contains a reference to the same context, so it's a circular reference... Yeah this seems pretty hard to deal with. :/

RazrFalcon commented 1 week ago

We should start sending patches to harfbuzz 👀