-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Support for Font Ligatures using harfbuzz #5696
base: master
Are you sure you want to change the base?
Conversation
b317f39
to
6cd8751
Compare
89e7102
to
796cdfe
Compare
@chrisduerr I would appreciate some feedback for the current change. It is likely that not a lot of performance improvements can be made without significantly refactoring the inner |
If a larger refactoring is necessary to match the current performance, then we cannot move forward without these changes. Ligatures are not worth dropping a single frame for. |
I think we could go with an implementation similar to kitty's. They continued using cell-based rendering, except they break ligatures into individual cells that have extra data that may point to the special ligature pixmap. Text runs are not broken by the cursor or any style change, so the division is simple and not dynamic. Plus when ligatures are disabled the only overhead would be the extra bytes that each cell will take. kitty: (each line is shaped) EDIT: there is an option to disable ligatures when cursor is over the characters, but there is no config to make color/style change break ligatures. current change: (different colors are shaped individually, cursor breaks a text run into two) |
Since Alacritty already has dynamic storage in place for cell content that isn't essential, it should be possible to implement this without any extra bits then. |
I just tried an implementation of this idea and it turned out to have worse performance than the current change, probably because it introduces a lot more complexity. I don't think it is worth it to try improving the performance further, because large refactorings induced headaches for me. @chrisduerr, to minimize the regression, I'll make it so that harfbuzz is not called when the ligatures option is disabled for folks that would like a performant terminal, which should make the performance impact minimal with the option disabled (I'll test before it gets pushed). If you are still unhappy, then I can disable ligatures by default, does this sound good? This is the only way forward, at least for me, if anyone else wants to take over, I'd be happy to send a diff. |
Having options to opt-out of performance is not acceptable. Either Alacritty manages to offer all features at high performance, or it doesn't. Just disabling all the useful features by default as an excuse to pretend like Alacritty has reasonable performance is not productive. I strongly recommend anyone who's not willing to spend some time optimizing the ligature implementation to please stay away from sending PRs related to it. That's really just wasting the time of everyone involved. |
Actually, I would argue that ligatures should be off by default, because some programs might have confusing behavior with ligatures on (eg. using <= as an arrow). It would be best if ligatures can be toggled on and off, or configured to turn on automatically when certain programs are running (eg. less, vim, nvim, emacs, etc.). |
Oh just to clarify, regardless of what the final ligature implementation might be, it will be off by default with a configuration option to toggle it. My comment's goal was solely to point out that having options that significantly impact Alacritty's performance is not an option. Performance is gauged based on the worst-case, not the best-case scenario. |
@fee1-dead when running |
@chrisduerr Just disable ligatures by default. Those who want to use them and are OK with sacrificing a bit of FPS can enable. It's just that simple. Why are you so opposed to merging this feature? |
Very "nice" & "supportive" attitude. Good luck getting any more pull requests to your repo. Came here after a couple of years of waiting for ligatures support. Will continue happily using @fee1-dead Huge thanks for your hard work, but I advise you to switch to either |
Merging garbage and slow code is not an option. Either it's implemented properly or not at all. Alacritty will never get any features that opt users out of Alacritty being a reasonably quick terminal emulator, that would completely defeat the purpose of Alacritty. |
Isn't the point of a draft PR to allow others to help? |
In almost all applications or projects, eventually there will be a feature that cannot be added without a regression in performance. If people really want the feature to be added, I have no idea why it must be "fast" such that it displays content from When developers write something in Rust, their primary focus could be performance, binary size, safety, etc. But I do not think that focus should stay forever. If it is not feature complete, it cannot be "the best terminal emulator". This project started with rather paradoxical views: (Announcing Alacritty)
Terminal emulators were not always the problem. See this reddit post where some continues to complain about
Good appearance without the bloat nor bad performance, is, in many ways, impossible to achieve. Alacritty is in a confusing state: do you want good appearance or not? Do you want high performance or not? If you want both, then it will never be the fastest terminal emulator. Being written in Rust does not help. If you just want the high performance, then close #50 and tell everyone that this project is not going to have font ligatures because it causes regressions. Please do not continue to give people hope that it will land if some magic contributor finds a way to implement ligatures without a 2x regression in performance. |
Only because you're incapable of implementing it, does not mean it is not possible. It is most certainly possible to implement ligatures without major performance regressions. |
Sorry, I retract my last sentence on that comment. I did come up with a way without regressions and that is to completely rewrite the grid storage so that it works well with ligatures (each input will only cause reshape of individual lines). It will require the effort of rewriting the entire project and is going to conflict with every other major PR. For people who want ligatures: consider switching to another project that actually supports ligatures, but if you really want to use alacritty, you could maintain a fork. |
I'm willing to give this a shot, and to try making it performant. No promises, but I'm willing to try. @chrisduerr, how do you suggest benchmarking the main branch and the ligatures PRs to measure progress with minimizing regressions? |
Nice, @Nytelife26! For me I used MangoHud to monitor the FPS and ran |
It appears that the preferred method (according to a blog post is to use vtebench and Is there any chance you could add me to your fork so I can work on the PR, or should I create my own? |
The recommended way to test PTY performance is by using the latest version of However I'd expect anyone trying to seriously verify the performance of any ligature PR to create supplementary benchmarks that stress specific worst-case scenarios. If you're struggling to come up with these, chances are you're not gonna get very far when it comes to providing a performant implementation either. |
Yeah, I get you. I'll do an additional formal series of tests, likely consisting of the following scenarios.
If you have any other suggestions, let me know, but those are the ones off of the top of my head. Should cover a lot of fairly common situations. For a more extreme stress test, I'll get it to stream out a random long block of text as fast as possible containing ligatures and then compare it to without. Edit: I also note that there are no benchmarks in the source code. It would probably be beneficial to add some for more crucial internal functions. That way you can track regressions over time. |
We intentionally have no direct Rust benchmarks since they're mostly useless and do not appropriately represent the performance of the actual application. |
zenixls2@3ed0430 just being too lazy to rebase on the latest changes in the repo. |
Yeah because detecting which feature of Alacritty might be causing slow downs just so we can support some slow garbage feature nobody needs is a great investment of time.
I don't care about your side of the discussion. If you disagree, you can either implement it yourself or use a different terminal emulator. I have zero interest in compromising if I have nothing to gain but pain. |
Compile time option could be achieved by complying the patch you want. Adding disabled by default code upstream is not a great idea. |
Dude, it's objectively not a garbage feature when clearly many, many people want it. If you don't want to invest your time on it though, that's fair.
Like this is fair enough, although we're having this discussion on a PR, so someone clearly has implemented it. So my question then becomes what objective measure needs to be met where you would actually consider merging ligature support? Obviously, I've not read the entire thread on this because it's huge, so sorry if it's already been said, but literally no performance loss is pretty much impossible when adding any feature. So what's the benchmark? It's fast enough that we don't drop any frames on modern hardware? 10 year old hardware? What frame-rate are we assuming the display has? I'd be willing to have a crack at this when I get the time if I knew what the goalposts were and that if I could get them met then it would be merged. If the answer is that you never would, then alright, just close this PR and indicate as such on the locked thread that you don't want ligatures in alacritty and never will.
I don't know if that's true, many, many common linux tools have code where you selectively compile what features you want. Fair enough if you don't want to go down that route though, I could fairly easily make an AUR entry with this patch (if someone hasn't already). |
The throughput should stay the same, frame rate could drop only slightly (a few frames) when the shaping is actually performed, and the input lag should not change at all. It also should work just fine on low end CPUs, like arm ones found in pine64 hardware where CPUs are really slow. In general, this issue requires a lot of effort in making it fast. Be aware that in environments like browsers where massive blocks of text are static it's not really an issue, because this content is shaped once, but in terminal where everything can change at any time, it's more complex. Like I know how to make it the same perf (in margin of error) when the user just edits text (and it has a lot of exceptions here as well, because text overlapping). But when your whole screen is redrawn it gets way more complicated. |
Would a solution whereby ligatures are only drawn when the screen has been static for a small amount of time be acceptable (on the order of ms)? Or maybe they get temporarily disabled when throughput is greater than a certain amount. I don't think anyone would mind too much if ligatures aren't drawn when you've got huge amounts of text scrolling and the screen is being constantly redrawn. That might alleviate some of the performance concerns? |
This is just poor UX, at this point just use conceal in vim if you really want them. |
Is it? I would hope you could get it working so that any delay after scrolling was essentially imperceptible. I don't know about you but when I have enough text scrolling through my terminal that performance becomes an issue, I can't really follow what's happening with any level of clarity. Maybe I'm underestimating how much throughput would cause a problem though |
I think it is important to remind people in this discussion that ligatures isn't just some fancy eye candy for programmers. Many languages require ligatures in order to display correctly. See this for an overview of this topic. Many other issues would be resolved with ligatures (#936 #3975 #4025 just to mention a few) but the maintainers simply thinks having full unicode support is "garbage". Of course, why care about internationalization at all when your only goal is to cling on to the performance? Sorry, you guys can keep on talking about "blazingly fast software" and "fastest terminal emulator" but I'm never going to use it if it has literally inferior font rendering. |
#3975 can't be done without breaking clients, needs negotiation, has nothing to do with ligatures, just shaping. #936 breaks width as well, can't be done without negotiation with client. #4025 similar. All you've linked has nothing to do with ligatures though, ligatures preserve the width, the modifiers don't preserve the width, it's a completely different issue and fixing ligatures won't fix any of that, but ofc you know better. |
Why do both maintainers like doing things like calling other developers incompetent and saying things like "ofc you know better"? I used the phrase 'ligatures' with an intention for it to mean text shaping, and that is what is done in this PR. If you're going to come up with a way to implement ligatures without getting the benefits of text shaping, you can feel free to go for it. I have not looked at this code for more than a year, and if the width issue exists, it should still be an easy fix. But really who cares about an insignificant technicality? From my point of view the amount of toxicity displayed is considerably off-putting for any FOSS project, let alone a Rust one. |
You decided speak for us, you've got reply based on what you wrote. If it offended you, then sorry, but I'm not sure what you've expected from commenting that way. If you disagree you can just move on.
The point is that text shaping similar to what ligatures are doing only works for input that doesn't change width. And I'm not familiar with anything other than ligatures which preserve width. Also, none of the PRs I've tested with ligatures made country flags work, though I don't remember testing it on this PR, since I closed it after massive perf loss.
Width between client and terminal, it's not a rendering sort of thing (unless you generally want to center glyphs within an area they're present). There're some term specific escapes which do negotiation on how both terminal and client calculate them, it's not an easy fix as you say.
width is clearly not insignificant, it makes the tui app unusable if you do mismatch, because the cursor can't be positioned reliably anymore and you have whole screen of artifacts.
Yes, not willing to accept every patch in open source is very toxic, I understand, and also getting constant spam on issue telling us how bad we are that we don't accept every patch we don't like. And when it comes to ligatures PRs it's like the 4-th? iteration of nearly the same code, what would you expect where all solution are basically the same as the first suggested one and which had proper reviews, etc? |
And to be clear, I've rechecked the PR and it's 4x slower and none of the linked issues (which are not ligatures) are handled, so flags are broken the same way, etc. Only said eye candy is rendered now. Edit: I'm not expecting linked issues to be fixed with ligatures work, it's like you're speaking from the point is that your patch is going to fix all the unicode. |
I don't think you got what I meant at all. |
Putting the above comments about toxicity aside, as I think everyone involved here could benefit from taking some of the heat out of this discussion. Is there a script anywhere that runs some objective performance tests that we can use to set a goal? Perhaps, we could define a VM that's suitably specced for a reproducible run? Then we could even look to add a performance test as a check to the pipelines and it gives all PRs an objective performance measure they can work to. |
I've got what you mean, but behavior on every ligature relating issue is toxic from both sides from the start, so not sure what you really wanted. And for me your behavior was toxic from the start, it's all subjective in the end, especially when you directly starting to call out. And some necrobumping.
I think it was even possible back then when you wrote it, since you have damage information through out the terminal, and you can shape the individual lines, and even do partial redraws (I have PR around to do that, which could resurected). Which is the reason why I said now that I know how to do it for regular common case to be nearly the same as master, but not for full grid change, when e.g. scrolling. It's not like you wanted to discuss the implementation (though, I wasn't around that time due to issues, so maybe @chrisduerr remembers you comming to irc and asking something). Besides. It's not like we're not interested at adding shaping at all, it's just requires work and probably communicating with us, and be aware that alacritty is just one of the projects we're doing in our free time, besides full time jobs and other issue. If you have challenges during implementation you can ask on irc (I generally reply to anyone, and you can even make me write patch for you if it's damage/partial rendering related, since I'm interested in that). But just claiming that you know how, but it'll require a rewrite and you won't do that is not productive, for both you and us. Maybe an idea to just make a rewrite of something internally besides the ligatures is not bad in the end, given that some parts of our grid code is just garbage. And it's the reason why @chrisduerr said that
|
If you touch the terminal related code (the part behind the mutex in display/mod.rs) vtebench is enough, however for rendering we don't have great infra other than disabling internal frame throttling and using things like |
I'm sorry about the confrontational tone in some of my last comments. I will try my best to be more productive here. I think a lot of the optimizations can be done by reducing the amount of data we feed to harfbuzz. I've read into the documentation again, and the cluster information (provided to us through GlyphInfo when shaping through
|
Maybe this would be a useful starting point for everyone then, not just this issue. Performance is clearly very important to you, but it's also quite a nebulous thing in a terminal. So, perhaps if we work together to come up with an easily reproduceable and rerunnable testbench, you can set it as an AC on all PRs that everyone can understand. You up for that? If so, I can create a separate issue for it and take that discussion over there and I might even have a chance to look at it over the christmas break. |
Resize could actually be fairly complex to optimize (and not sure whether it's worth it)l, given that with reflow involved grid could require attaching rendering metadata to it (but it's separate from the rendering), and in general it could require grid rewrite (it's not reallyl a bad thing, given that the current structure struggles a bit), since
This is all tracked through out alacritty/alacritty_terminal now (was before, but now with a bit better abstractions), so that thing sounds possible and what I was thinking of doing. Like we have information on what roughly changed within each line, so we can try to reshape based on that data, which will reduce the amount we're wasting by a lot. The challenge is to store clusters somehow, but I think it's a sort of thing you get only when you try to plumb it. My main issue is how to deal with full-screen updates and scrolling. Probably scrolling could be handled with damage information we already have if we try to rotate it inside the viewport, surely we still need to re-damage all the frame, but the information on what was changed inside the lines could be more or less accurate here (less accurate means overdamaging). As for full-screen updates I don't have an answer, since I never tried shaping and don't know common patterns. Maybe something like |
@hughesjs we already have https://github.com/alacritty/vtebench and https://github.com/alacritty/termbenchbot (and yes, you can use it to trigger perf report on alaritty PRs). What we don't have is automated rendering performance reporting, which could be nice and I'll be happy to review a patch adding instrumentation to collect fps more reliably out of alacritty (behind a compilation flag, which will be used by said bot). |
Seems like clusters doing something similar if I read the links correctly, at least they can sort of determine the mappings. |
Oh, and besides what was said. I think the good way to approach that issue would be to just start outside of alacritty and try to shape grid (basically That should be more productive or at least educational, since when doing directly inside alacritty it'll likely just prevent from any progress and one could start fighting the codebase. And as for maintainer it'll really help to see why particular approach was used, when you have benchmarks to show it's easier to convince people. Doing it separately could also benefit others, since they can pick up/play with what was discovered. |
Does it run them on virtual hardware approximating the lowest-power device you'd like to be able to run at full speed? I think that's the key here |
The point is delta, it's all on real hardware, since hardware acceleration, but the system is powered on, run this benchmark, and powers back off. It's sole purpose is to run the benchmarks. |
Ahh fair, that'll probably do... I was just thinking less modern hardware might be hit harder than more mordern hardware for certain feature changes, so the delta might not be linear |
That's why it's running an ancient GPU. |
and supporting text shaping where the glyphs' widths change, as you said, would probably require a rewrite as well? But that should probably be after harfbuzz is introduced to the codebase first, right?
I'm not sure if I understand what you meant here. Does |
They just can't be supported, regardless of grid, because you'll break client width as well, thus the entire screen will be unreadable. There're experimental escapes to negotiate width, but that's about it.
It basically gives iterator and we also have a |
Yikes. Maintainers’ uncompromising attitude in the face of overwhelming user demand - yes, the likes count - and the yearlong mistreatment of a would-be contributor, are beyond the pale. I thought I was a user for life, but right after I hit submit on this comment, I’m off to pick another terminal emulator. Be better. |
any updates? |
NOTE: alacritty/crossfont#35 has to be merged first. The current change modifies the dependency to point at my fork. It should point to a released version instead!
Closes #50. We should be able to statically link to harfbuzz on every supported platform, so not gating this behind
cfg(unix)
.Supersedes #2677 and #5615.