RazrFalcon / rustybuzz

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

Sync with HarfBuzz 2.9.0 #83

Closed bluebear94 closed 8 months ago

bluebear94 commented 8 months ago

What’s your opinion on harfbuzz/harfbuzz@79e8b30, given that fallback kerning isn’t implemented in Rustybuzz and is deprecated even in HarfBuzz? Should this commit be skipped (along with the corresponding test)?

Edit: the commit does change behavior even if fallback kerning isn’t implemented. I’ll push a corresponding commit with a stub for _hb_ot_shape_fallback_kern.

bluebear94 commented 8 months ago

@RazrFalcon There are some commits in HB to implement higher-order interpolation in variable fonts: harfbuzz/harfbuzz@ba6db26, harfbuzz/harfbuzz@0b2280a, harfbuzz/harfbuzz@8901806, harfbuzz/harfbuzz@233c29b, harfbuzz/harfbuzz@0a44fea, and harfbuzz/harfbuzz@a9a607d, all of which touch code out of scope for RB itself. The test attached with the last commit doesn’t pass on RB yet; I think it will be necessary to add HOI support in ttf-parser.

RazrFalcon commented 8 months ago

What’s your opinion on https://github.com/harfbuzz/harfbuzz/commit/79e8b30, given that fallback kerning isn’t implemented in Rustybuzz and is deprecated even in HarfBuzz? Should this commit be skipped (along with the corresponding test)?

I honestly do not remember this part... We do not have a fallback shaper, but we do support fallback functions. In this case, to my understanding, instead of using the built-in kern parser harfbuzz fallbacks to an external one, like FreeType one. But we do not do this at all. It's ttf-parser or nothing. So in out case calling _hb_ot_shape_fallback_kern is no-op. I would guess if you build harfbuzz without external parsers support (not sure how), you would get the same output as with rustybuzz. And yes, rustybuzz version of harfbuzz is built with HB_DISABLE_DEPRECATED and maybe even with HB_NO_OT_SHAPE_FALLBACK.

UPD: yep, see: https://github.com/RazrFalcon/rustybuzz/blob/c4c792495bedae4ef61839459bf1c7ba0f654881/harfbuzz/src/hb-ot-shape-fallback.cc#L424

This is the old hb fork used by rustybuzz. So you code is correct.

RazrFalcon commented 8 months ago

There are some commits in HB to implement higher-order interpolation in variable fonts:

I'm not sure what is the problem here. ttf-parser provides a low-level enough API to implement this. Or am I missing something?

RazrFalcon commented 8 months ago

Oh, I see now. We have to update this function as well. Instead of using only the first match, we have to check all matches now. It should be a trivial change.

RazrFalcon commented 8 months ago

Updated the ttf-parser. Hopefully that was the right change.

bluebear94 commented 8 months ago

Does HB do anything special for calculating the advance of the space character when it’s not defined in the font file? I suspect that’s why the variations_003 test introduced in harfbuzz/harfbuzz@a9a607d for us.

RazrFalcon commented 8 months ago

No idea. And if so, we're doing the same. So we have to have the same output. Can you check if those tests were producing the same output on older harfbuzz? Because I'm not sure the latests changes could affected it. Maybe an old rustybuzz bug or a missing feature.

bluebear94 commented 8 months ago

Aha, the variations_space tests were moved to variations, so variations_space_001 (which was skipped) is now variations_003.

RazrFalcon commented 8 months ago

Nice catch!

bluebear94 commented 8 months ago

Heads up: HarfBuzz 2.9.0 changes a lot of things in the test harness. I’m going straight to targeting 2.9.0’s interface for gen-shaping-tests.py.

RazrFalcon commented 8 months ago

@bluebear94 The old in_house.rs had 12 KLOC, but the new one is 62 KLOC. Are you sure the generation script is correct?

RazrFalcon commented 8 months ago

Looks like you're generating an insane amount of emoji_clusters_* tests.

bluebear94 commented 8 months ago

Looks like you're generating an insane amount of emoji_clusters_* tests.

That is the case. Since these pass, do you want to skip generating them now?

RazrFalcon commented 8 months ago

Yes, let's skip emoji_clusters_* for now.

RazrFalcon commented 8 months ago

And can you somehow force-drop the Update gen-shaping-tests.py to target 2.9.0 interface commit? I would like to avoid huge commits, especially code-generation related one.

bluebear94 commented 8 months ago

Okay, I tried to squash the Update gen-shaping-tests.py to target 2.9.0 interface commit with the Skip emoji-clusters tests commit.

bluebear94 commented 8 months ago

Also, I switched the ttf-parser dependency to a Git revision for the Face::set_variation fix, but this should be changed back to a crates.io version once ttf-parser gets another release.

RazrFalcon commented 8 months ago

I will bump ttf-parser now.

RazrFalcon commented 8 months ago

Or not... I have some changes I have to finish first. Will take a look on the weekend.

RazrFalcon commented 8 months ago

@bluebear94 New ttf-parser is available.

RazrFalcon commented 8 months ago

Does v2.9.0 sync complete?

bluebear94 commented 8 months ago

Yes, if I didn’t miss anything.

RazrFalcon commented 8 months ago

Thanks!