alacritty icon indicating copy to clipboard operation
alacritty copied to clipboard

Support ligature

Open zenixls2 opened this issue 4 years ago • 36 comments

Fork the topic from mainstream.
Any issue related to my fork should go here.

Notice that this fork might take a long time to develop and possibly not (able to) merge back to the mainstream.

zenixls2 avatar Jun 13 '20 06:06 zenixls2

@kchibisov How are the odds this work could inspire upstream? If this could be achieved it would play nicely in a platform independent way: https://github.com/zenixls2/alacritty/commit/b92f094399f08c9bb5ae362d13d317ce3103f849#r39874718

blaggacao avatar Jun 13 '20 18:06 blaggacao

If it won't regress any performance we're all ears, which will likely require a lot of effort from us as well, however upstream is focused on a bit different things right now. So the patch must be complete and don't regress any rendering and throughput performance.

kchibisov avatar Jun 13 '20 18:06 kchibisov

If possible, I want to know more details on how the core developers estimate the performance.

zenixls2 avatar Jun 15 '20 02:06 zenixls2

https://github.com/alacritty/alacritty/blob/master/CONTRIBUTING.md#performance and also render_timer values.

kchibisov avatar Jun 15 '20 02:06 kchibisov

Ok, I've got some data in performance.md.
Seems no big influence on performance.

zenixls2 avatar Jun 16 '20 10:06 zenixls2

Please, look at render_timer while running vtebench the performance of this branch is ~15x slower than master branch. (2ms to render frame compared to 30ms on it). Even your performance.md is showing that due from increased throughput performance, meaning that VTE parser has more time to run and meaning, due to less amount of locks due to very slow rendering.

Unless you'll have performance in a margin of error on render_timer or let's say 50% difference, I won't even try to look into it.

kchibisov avatar Jun 16 '20 10:06 kchibisov

I see you already put some effort in some tweaks in your latest commit https://github.com/zenixls2/alacritty/commit/9427d568d8113f783ebb2359bc5da584b36456a6 Just wanted to highlight that. Maybe with the guidance he gave, this can be pushed further to convince @kchibisov into looking at it?

blaggacao avatar Jun 16 '20 18:06 blaggacao

It seems like the performance hit from ligature is unavoidable. How do you think if performance is unchanged when ligature option is turned off?

In branch ligature I've introduced a text_run hashset to cache the rendered output. This works for most cases, but not for the tests of random writes.

In ligature_test branch I'm trying to replace all RenderableCells with TextRun. There is slight performance improvement but may introduce some issue on Colors in config.

zenixls2 avatar Jun 30 '20 03:06 zenixls2

@blaggacao do you have a windows machine or mac os machine to build? I want to know if my changes break the other systems. The final goal is to support harfbuzz for shaping in different systems, but hard to do so by myself since i currently only have one linux machine...

zenixls2 avatar Jul 17 '20 09:07 zenixls2

I'm afraid, I'm not even having one within reach, let alone using one. @kchibisov would you be able to suggest somebody or a some way?

blaggacao avatar Jul 17 '20 15:07 blaggacao

harfbuzz only works on linux/bsd. On other platforms core text and direct write should be used.

kchibisov avatar Jul 17 '20 15:07 kchibisov

Could country flag emoji issue also be solved? Are those also ligatures?

https://github.com/alacritty/alacritty/issues/4025

dashezup avatar Jul 24 '20 13:07 dashezup

@dashezup not sure, maybe you could give it a try on the latest commit in ligature branch. My libraries in centos are too old to support emojis, also I don't have a mac to test if my modifications really works.

Update: i guess it's already resolved in my branch.

zenixls2 avatar Jul 24 '20 15:07 zenixls2

Your fork has been working pretty well by my side but I noticed recently that ligatures do not work anymore. Which information could be relevant to share in order for this to be solved besides the fact I am using the package from aur/alacritty-ligatures-git 0.7.0.1745.ga15eb3b-1 ?

danielementary avatar Dec 30 '20 22:12 danielementary

Hey, I ran into the same problem after the last commit.

System OS: Linux 5.9.16-1 (Manjaro 20.2 Nibia); Version: alacritty 0.7.0-dev (a15eb3b) Linux: X11, i3 Package: aur/alacritty-ligatures-git Font/Terminal Size: Iosevka SS09 / maximized borderless

Here are examples:

  • before the commit 2020-12-31-022224_42x376_scrot

  • and after 2020-12-31-022617_44x401_scrot

Thanks in advance, appreciate your work.

SiamionRalionak avatar Dec 30 '20 23:12 SiamionRalionak

I tried to confirm from remote but gl rendering is not supported in vnc. might be something wrong in my fork of crossfont. currently I have no way for fixing it and the situation will be lasting until Jan 4th since I could not sit in front of my linux pc.

zenixls2 avatar Dec 31 '20 02:12 zenixls2

@SiamionRalionak @danielementary can u check again? Should be fine now

zenixls2 avatar Jan 04 '21 04:01 zenixls2

Hey, I've tested the new build. unfortunately the issue isn't fixed yet.

Would it be possible to use merging master instead of rebasing one squashed commit onto master. And probably use separate commits for new changes? It would be easier to monitor the changes and more feasible to help. Moreover an issue I have currently, as I'm using fixed commits (for packaging reproducibility), is the garbage collection of github. All older rebased commits disappear after a week or so (that's the frequency github runs garbage collection to remove dangling commits) could be more or less, but after some time it's not possible anymore to download older (in this case working) commits.

Later when a PR to the upstream is opened, it should be no issue to rebase and squash everything into one commit onto master, as the merge commits are resolved while squashing, so no manual changes should be necessary if the current master is merged into this branch.

Thanks for the great work, keeping it up to date with master btw.

Philipp-M avatar Jan 04 '21 12:01 Philipp-M

@Philipp-M did you tested the latest ligature branch, or just the aur package? Aur package is still not updated, and I'm not the maintainer of it either, so it might take time to react. I'm currently using the same version in my branch and it works fine with Iosevka Term Extended.

I've also tested the latest crossfont ligature branch that Fira Code works fine.

I tried to merge the upstream before, but in the end gave up because there were too many redundant conflicts to resolve. Using single rebase commit on top helps me rect faster to the changes in upstream. To address the garbage collection issue you mentioned, probably i should create a branch for backup?

zenixls2 avatar Jan 05 '21 02:01 zenixls2

Hey @zenixls2, thanks for your reaction. I just cloned the repo and built the ligature branch but the issue do no seem to be solved. Is there anything I can help you with ? My current font is Iosevka Term, I also tried Fira Code but none of them seem to work.

danielementary avatar Jan 05 '21 10:01 danielementary

What about explicitly set ligatures = true in config file? This is the only reason I could think of

zenixls2 avatar Jan 05 '21 11:01 zenixls2

Okay, so this was apparently a config issue, it is working now. Thanks for your precious help and contribution, this is priceless.

danielementary avatar Jan 05 '21 14:01 danielementary

Can confirm, setting font.ligatures = true fixes the issue.

Some comments though after skimming the patch: https://github.com/zenixls2/alacritty/blob/974e49f157a78ededb08ccefe0ba6be2421a4387/alacritty.yml#L182 shows ligature instead of ligatures, this should probably be ligatures.

And I don't see anywhere, where font.ligatures gets its default value (true) AFAIK, the default value of a boolean is false isn't it? So this might explain the issue we had?

Philipp-M avatar Jan 06 '21 12:01 Philipp-M

Yes, correct. Recently there was a commit in upstream that changes the serialize/deserialize method of configuration. In order to resolve the conflict and fix the compilation, I remove the use of struct DefaultTrueBool(or DefaultBoolTrue, whatever) and modify it to bool. Probably at that time I forgot to define the Default trait implementation of font config.

Anyway will fix this soon.

Also there was a big change in the upstream recently and introduce lots of conflicts. That's why there's no update in my branch since then. Anyone who is waiting please be patient.

zenixls2 avatar Jan 06 '21 16:01 zenixls2

I just wanted to give my appreciation for your efforts and offer my Mac system if you need to do any testing on said platform. I would love to contribute to this project!

jorgebef avatar Jan 15 '21 17:01 jorgebef

I just tried this fork on Arch Linux and it works perfectly for Arabic script. Thanks for the effort put into the fork. I just want to mention, for what it's worth it, HarfBuzz compiles to WASM, and there are WASM runtimes other than Browsers, just in case.

mehdisadeghi avatar Mar 12 '21 08:03 mehdisadeghi

will this ever be pushed to upstream Alacritty? Was this ever mentioned in https://github.com/alacritty/alacritty/issues/50 ?

luisdavim avatar May 01 '21 09:05 luisdavim

will this ever be pushed to upstream Alacritty? Was this ever mentioned in alacritty#50 ?

Only if all platforms are tested and supported. Also, current implementation still have a lot of bugs. It's not of performance but about stability now that blocks me from sending pr.

zenixls2 avatar May 01 '21 14:05 zenixls2

I compiled the binary from source with cargo as the instructions describe, but when trying any of the formats of ligatures: true that are discussed in this thread do not seem to enable ligatures and simply give me config errors. I am using the JetBrains Mono Nerd patched font for reference. Any help would be greatly appreciated, cloned the repo today to the build for reference as well.

azzarello avatar May 18 '21 19:05 azzarello

I compiled the binary from source with cargo as the instructions describe, but when trying any of the formats of ligatures: true that are discussed in this thread do not seem to enable ligatures and simply give me config errors. I am using the JetBrains Mono Nerd patched font for reference. Any help would be greatly appreciated, cloned the repo today to the build for reference as well.

Did you checkout the ligature branch?

zenixls2 avatar May 20 '21 19:05 zenixls2