RazrFalcon / rustybuzz

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

Fix some glyph flags bug #125

Closed LaurenzV closed 8 hours ago

LaurenzV commented 5 days ago

and use the new test infrastructure to test them!

Unfortunately, there is one bug remaining that I haven't been able to figure out. In rustybuzz, we have these lines of code inside of state_machine_kerning: https://github.com/RazrFalcon/rustybuzz/blob/e2f88315124d8a83b075979731510741c99feb4c/src/hb/kerning.rs#L186-L201

From what I can tell, this doesn't exist in harfbuzz. After debugging it seems like even for kerning, harfbuzz also uses the drive method which we use for morx, and it has a different mechanism for determining the flags:

https://github.com/RazrFalcon/rustybuzz/blob/e2f88315124d8a83b075979731510741c99feb4c/src/hb/aat_layout_morx_table.rs#L223-L296

Unfortunately, I wasn't able to quite port that over because it has some fields which we don't seem to have in state_machine_kern. But I've still added a test and disabled it for now.

But I was able to squash two other bugs!

RazrFalcon commented 5 days ago

From what I can tell, this doesn't exist in harfbuzz.

Well, I ported it from HB, so it was existed at some point in time 😄 Here it is: https://github.com/harfbuzz/harfbuzz/blob/05ef75c55340400d4b318bd24d742653bbf825d9/src/hb-aat-layout-common.hh#L754 I guess it was updated later.

LaurenzV commented 5 days ago

Ah, thanks! I wasn't able to find it in previous commits, hopefully this will help. I'll try to give it another look.

RazrFalcon commented 5 days ago

And this change is currently in morx and not in common, like in HB, for some reason. But it is still generic. Aka it's not morx specific. So we probably should move it to common.

RazrFalcon commented 5 days ago

Here it is in RB: https://github.com/RazrFalcon/rustybuzz/blob/e2f88315124d8a83b075979731510741c99feb4c/src/hb/aat_layout_morx_table.rs#L160

I remember this code well, because I was arguing with behdad about it...

RazrFalcon commented 5 days ago

Found it: https://github.com/harfbuzz/harfbuzz/commit/978918c32a66af41df86386510cf73a4c0e8d393

LaurenzV commented 5 days ago

I'm super confused now. What is the difference between apply_state_machine_kerning in aat_layout_kerx_table.rs and apply_state_machine_kerning in kerning.rs?

Ah, one is for the kern table and one for kerx?...

RazrFalcon commented 5 days ago

Good question. I guess I weren't able to make it generic enough and simply duplicated the implementation. I remember having a lot of issues with generics when implementing AAT. I just wasn't able to replicate HB logic. You can try merging it into one function.

LaurenzV commented 1 day ago

Do you think we could merge it as is for now? I think merging the AAT code will require me to make a deep dive into AAT so I understand what is actually going on, but for now I'd rather focus on finding some other "low-hanging" fruits by setting up the fuzzing. I've added a test case for the case that's not working so it won't be forgotten about. 😄

RazrFalcon commented 8 hours ago

Sure.