-
Notifications
You must be signed in to change notification settings - Fork 37
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
Backport to 7.0.0 #112
Backport to 7.0.0 #112
Conversation
LaurenzV
commented
Jun 18, 2024
•
edited
Loading
edited
Status | Commit message | HB | |
---|---|---|---|
🟡 | [hb-view/hb-shape] Add --glyphs | Link | We don't need this API, right? We don't have anything similar to hb-view, so it probably doesn't make much sense |
🟡 | [util] Improve --glyphs | Link | |
🟡 | [util] Fix vertical positioning with --glyphs | Link | |
🟢 | [paint-extents] Start out | Link | |
🟢 | [paint-extents] Flesh out some more | Link | |
🟢 | [paint-extents] Flesh out more | Link | |
🟢 | [paint-extents] More | Link | |
🟢 | [paint-extend] More | Link | |
🟢 | [paint-extents] Finish off | Link | |
🟢 | Get outline extents manually | Link | |
🟢 | Skip empty outlines | Link | |
🟢 | [paint-extents] Better handle empty glyphs | Link | |
🟢 | [colr] Return true extents | Link | |
🟢 | [paint-extents] Clean up | Link | |
🟢 | [paint-extents] Streamline extents_t | Link | |
🟢 | [paint-extents] Streamline extents_t more | Link | |
🟢 | [paint-extents] Refactor code | Link | |
🟢 | [paint-extents] Minor refactor | Link | |
🟢 | [paint-extents] Comments | Link | |
🟢 | [paint-extents] Rename variable | Link | |
🟡 | [gsubgpos] Minor use ->clear() directly | Link | relevant? wasnt able to fint he corresponding code |
🟡 | [gsubgpos] Use swap instead of move | Link | |
🟢 | [paint-extents] Simplify transform_extents | Link | |
🟢 | Revert "Revert "[aat] Support feature ranges"" | Link | |
🟢 | [aat] Always generate a feature range | Link | |
🟢 | [aat] Support ranges in NonContextual subtable as well | Link | |
🟢 | [aat] Run subtable across ranges if flags match | Link | |
🟢 | [aat] Adjust last range | Link | |
🟢 | [aat] Always unsafe-to-concat in state machine | Link | |
🟢 | [aat] Add test for feature range | Link | |
🟢 | [aat] Optimize feature application | Link | |
🟢 | [aat] Comment | Link | |
🟡 | [aat] Optimize feature-range application | Link | No idea what this does. |
🟢 | [aat] Initialize values | Link | |
🟢 | [test] Remove non-free font and its test | Link | |
🟢 | [COLRv1] Don't return extents if glyph has no paint | Link | |
🟢 | [COLRv1] Handle void extents | Link | |
🟢 | [layout] Limit how far we skip when looking back | Link | |
🟢 | [gsubgpos] Refactor skippy_iter.match() | Link | |
🟢 | Revert "[layout] Limit how far we skip when looking back" | Link | |
🟢 | [GPOS] Avoid O(n^2) behavior in mark-attachment | Link | |
🟢 | [buffer] Optimize _infos_find_min_cluster for monotone clusters | Link | |
🟢 | [buffer] Optimize _infos_set_glyph_flags to avoid O(n^2) behavior | Link | |
🟢 | [buffer] Speed up merge_clusters_impl | Link | |
🟢 | [GPOS] Fix assert fail introduced recently | Link |
@RazrFalcon The commits contain pretty big changes to AAT... I'm afraid we need to figure out a solution to the AAT testing problem before we continue with that. :( I don't think it makes sense to make AAT more broken than it (potentially) already is, and at least to me it seems that there are a few differences between |
Ugh... AAT is pain. I know it works because I ran tests locally before, at least some of them. But any changes would require a proper CI support. And yes, HB and RB implementations are pretty different, because HB uses insane C++ templates magic, which we cannot port to Rust or any other language. One thing to note is that the ttf-parser side of the code should be fine. I don't think we would need to change it. But rustybuzz one... Otherwise, it's the same drill as always: I wrote this code 4 years ago and I do not remember anything. We're on the same level here. Can you point me to HB commits related to AAT? As for testing, last time I've checked, i.e. 4 year ago, HB was running different macOS versions on CI. Then it would check hash-sum of system fonts and if any of them match - runs tests. We have to do the same. |
Those should encompass the main changes: https://github.com/harfbuzz/harfbuzz/compare/b2087132..8b17c6ca3 There are some smaller ones afterwards, but they don't seem really hard.
I wanted to look into that, too, but at least to me it seems that CI always runs on the latest MacOS: https://github.com/harfbuzz/harfbuzz/blob/main/.github/workflows/macos-ci.yml
I have a Mac, so I'll give it a try! |
Doesn't looks that bad. Some stuff moved around, but it's not a rewrite of the logic I was worried about. And it seems And you have to pass Hopefully this clarifies things a bit. Maybe I will take a look into it myself later this week.
Interesting. Maybe something changed or I misremember stuff. But tests still check for hashes. |
Okay, thank you, I will see when I find the time and motivation to look into it in more detail. |
@RazrFalcon So, by the way, I feel kind of bad for asking about this considering that a lot of time and effort was put into As mentioned, The only (potential) disadvantages I could see are binary size (I presume that it's a heavier dependency than As I mentioned, I'm more curious to hear what you think, I'm not saying that we should definitely do this. |
I'm aware of Honestly, I don't have much attachment to my projects. I don't have time working on them anymore. Yes, there is swash, which is sort of abandoned and completely untested. And seems like it uses Also note that we use While
While In the end it all boils down to: what we would gain from switching to Honestly, when it comes to throwing stuff out of the window - rustybuzz is the first thing that comes to mind. I hate it so much. I spend so much time working it. I still have no clue how it actually works. It's an endless rat race to keep it in sync with harfbuzz. PS: looks like people at |
As I mentioned, I didn't mean to imply it's not, it obviously works really well! I don't think there is a pressing need to switch, I just was curious on your thoughts.
I'm actually working on this right now, I'm having some difficulties with integrating it with variations, but hopefully I can manage to get it to work. I'll try to create a PR soon-ish.
Yeah, that's true...
I guess right now, apart from the hinting thing, there might not be any immediate benefit. I was just thinking that in the long term, it could be nice to look into it since it seems to have a lot of backing. Probably the best thing is just to wait and see how things develop... |
I've said this before: We'd be happy to move rustybuzz to the harfbuzz org and try to keep it up to date after the forward-porting by @LaurenzV is complete.
Correct.
While I don't work on |
I'm skeptical how well it will work. Maintaining two codebases instead of one is not fun. Simply moving the repo around would not help much either. The only way to keep rustybuzz alive is to make backporting as simple and as streamlined as possible. Which is impossible, because harfbuzz relies on C++ templates magic. A curse of every C++ project. We cannot simply port/translate harfbuzz to Rust 1-to-1. We have to make changes to avoid unsafety, like having The only real solution is to have harfbuzz or a brand new shaper to be written in Rust.
Yeah, that's kinda my problem with it. I need a user friendly, high-level TrueType parser, which is the purpose of
The problem is that harfbuzz does a lot of things. A lot. And rustybuzz does only shaping. So swapping Once again, I will archive |
To my knowledge, hinting has no effect on shaping and glyphs rendering in |
That's exactly what But yeah, I just noticed that |
@LaurenzV how did you able to build |
What commit are you going from? |
6.0.0 tag |
It seems to work fine for me... I always run this script when updating the tests:
|
Yes, but a very low-level one, compared to But yeah, will wait for now. |
Can you try removing |
I did that and it still works fine. What exactly is the error you are getting? |
I do not get an error per se, but I see:
And |
Check Here is my log output in case it helps. Maybe you could post yours.
|
Nevermind, you need to check |
I had to install |
Do you know if |
Also, what does this do: if (event->start)
{
active_features.push (event->feature);
} else {
feature_info_t *feature = active_features.lsearch (event->feature);
if (feature)
active_features.remove_ordered (feature - active_features.arrayZ);
} I don't really get the |
No idea. Doesn't seem like it. Looks like all it does is feature-gates
if let Some(index) = active_features.iter().position(|f| f == feature) {
active_features.remove(index);
} Classic C shenanigans. |
The Rust code sample above is just for the if let Ok(index) = active_features.binary_search(&feature) {
active_features.remove(index);
} |
Got it, thanks. |
I do not know why are you torturing yourself this much, but it's highly appreciated. Once again, if you have any ideas about how to make porting easier - feel free to share them. We might even try "improving" harfbuzz itself 👀 |
Not sure I have many suggestions, apart from the fact that some of the things could be aligned more to harfbuzz, especially in AAT. But overall it's pretty okay for now, I think! |
Trust me, I've tried. I couldn't make Rust generics happy with C++ templates duck typing. |
In |
Just a couple minor questions (see the top comment), but other than that this should be done now. |
Well, well, well. You're not the first one. harfbuzz/harfbuzz#1967 In a nutshell, in harfbuzz
Yep, we do not have
Yes, looks like we do not have it.
Checks if any |
Okay, thanks! Then this should be ready from my side. |
Congrats! We're a year behind harfbuzz now. Should we make a release? |
I think we can still wait a bit. I want to see how far I can get in the next few days, but as you want! |