-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add windows font fallbacks #3127
Add windows font fallbacks #3127
Conversation
@davidhewitt could you show how glyphs looks like without downsampling on windows? |
If I don't downsample, then the glyphs are sharp. But their width is much larger than for my current font (Cascadia Code), so we end up with alignment issues: I've highlighted one smiley to help show the size issue. Alacritty is drawing a highlight rect around what it thinks is two glyphs in width (because the emoji are double-width glyphs), but it doesn't quite capture the smiley face properly. Notice the right edge is cut off. (I think Alacritty is actually rendering the rest of the glyph, but it's black on black background so we can't see it.) |
In Windows Terminal the alignment looks correct, though I don't see any downsampling code. However, its rendering is slightly different to Alacritty's. I think it renders directly to the window surface rather than rasterizing glyphs to bitmaps first as Alacritty does. (Yes, there are a couple of WTFs with some of the glyphs. I have no idea what that is about.) |
Emojis are not two width glyphs in the context of terminal app, their just glyphs with width defined unicode-width-crate(wcwidth in other terminals, etc), so they can be with width equaled to 1.
Ehm, it's a bit strange to be honest, so rather then using downsampling you should understand why they're so much to the right. On Linux/BSD downsampling exists to solve a different issue, since emojis on the these platforms are not scaled at all (you're getting for example 138x128 pixels bitmap for every font size and should scale it yourself). I don't like the windows terminal approach tbh, however I feel like it's just their rendering. |
Oh, could you go into more details why we can't just implement color font support with what we have right now? I'm not forcing you to add it here, but just wondering. |
I query the metrics for each glyph from the font face returned from the fallback API. IIRC, for the highlighted smiley the advance is expected to be about 25 pixels, but Alacritty is only giving the glyph 22 pixels of space (i.e. two monospace columns). All layout is done from the left, which is why the glyph appears to be off to the right.
Starting from this line is where Windows Terminal adds color emojis: There's a number of pieces needed for us to reproduce all that:
All of that stuff is also going to add a whole load of |
I'll have an access to a Windows machine for a next few days, so I'll try to play with this fallbacks and "slightly big" emojis thing. |
You should sort out locale issue, otherwise it's nearly impossible for me to test it. I've changed my locale to much my system locale (hardcode thing), however it still don't work as expected, since I'm getting missing glyphs symbols, followed by emoji in some cases in other just missing glyph symbol. I also had to install wsl, since emojis wasn't working for me in cmd.exe and powershell (Windows was just removing them from paste!!!!). At least I can say that CJK glyphs are seems to work fine now. (A small win?) @davidhewitt Do I need something special to make things work like on your screenshot? |
I've added a commit which uses the correct locale.
Not as far as I'm aware. Are you using ConPTY + Windows 10? WinPTY + emojis doesn't work for me in testing. |
Yeah, ConPty + Windows 10. Windows just decides to strip emojis sometimes from input and paste random things in alacritty. |
Weird, that would be worth investigating as a separate issue perhaps. Might be a clipboard bug our side, or ConPTY bug to report upstream. Paste seems to be working for me. For now I've just manually created a text file in VSCode which has a bunch of emoji in it, and just displaying that with
I occasionally get a missing glyph symbol which disappears on a resize. To me that suggests an upstream bug in ConPTY. (I'll try to debug that.) |
I'll try to create something similar to repro.
Yeah, it seems like it. Knowing how alacritty clipboard and selection works it seems like a ConPty issue. The key thing here is that if you copy a missing glyph symbol and paste to e.g. Firefox, it'll still be a missing glyph symbol, but not the underlying text (there should be some char causing missing glyph thing). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the downsampled image, it seems like the downsampling is absolutely terrible. I don't think there are similar issues with Linux emojis. I'd assume this might be due to the low resolution we're scaling from? It seems very, very strange to me that we would request things at one size and then still scale it down, since at that point we should always be requesting the biggest unscaled size, so downsampling has the highest possible input resolution.
I'd assume the emojis are just to the right because they're too big for our cells. So it seems like the fallback fonts' size might be too big? I don't see you passing through the font size to dwrote
for font fallback, maybe there's something missing?
This is a really interesting point - perhaps I should not be calling |
Some further investigation and I think I've cracked it. I'm doing the right thing with the font size: the font size is not relevant to the DirectWrite font face. The font size is only relevant when actually rasterizing the glyph - as part of the I've also found how Windows Terminal solves this issue. Instead of downsampling, it dynamically rescales the font size so that the glyph fits within the cell width. The algorithm is here: I'm going to adapt this PR to do the same. |
FWIW I don't like the idea of using unicode width crate to make things fit into cell. Just let things overlap, it's not a problem tbh, this 1 width emojis are just bad and you'll be consistent with out other backends. So if you want to follow windows terminal here, just don't use unicode-width to solve problem... |
Yeah, this is true. Since I've just recently updated our unicode-width crate to the latest version of the standard, I can confidently say that non-combined glyphs all have the correct width in Alacritty. So any width mismatch between OS and Alacritty's width function should be an indication of a bug in the OS' library. |
I've tidied up the remaining pieces on this branch. To remove the
Hopefully you agree this is a reasonable compromise between keeping the rendering tidy and avoiding the Attached is a screenshot of the greyscale rendering now: The remaining problems @kchibisov was encountering (paste & missing glyphs) appear to be upstream ConPTY issues; I may look into them but that'll be upstream bugs. In summary, this PR is feature-complete as far as I'm concerned. I plan to follow up with a separate one in the future with color rendering for emojis once I figure out the best places to put all the pieces. |
Yeah, the picture and the idea LGTM.
Could you check that our Windows clipboard is not at fault here? Like making it load and the instantly store to it and paste back to some app? If you need help on how to do it let me know. I mean that you're not pasting text to console at all here, just internal manipulations in alacritty. |
So I did some quick testing and found the following:
So there is possibly an issue with our clipboard code. I wonder if it's a wider issue beyond just emojis (i.e. might be a problem with a number of unicode glyphs). |
It seems like the paste issue is likely upstream with ConPTY, as Windows Terminal has the same problem: microsoft/terminal#1503 |
I fixed a number of the style issues and rebased on master. This PR won't build at the moment. I opened two PRs against |
Just out of curiosity, any chance this can or will be included in the upcoming release? |
The 0.4.2 release will no receive any further updates other than bugfixes for regressions. |
Good catch, fixed. Also squashed all commits. |
@davidhewitt I'll try to get to this as soon as my search effort is done. I have not forgotten about this, just to let you know. If you are in a hurry with this, please let me know and I'll try to make some time to look at that before search is done. |
I don't think there's any plan to fix this if I remember the discussion in this PR correctly. We don't scale fonts on any other platform so it doesn't make sense to start that on Windows. If you want glyphs to fit in with your primary font, you should either pick a compatible font or add them to your font yourself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's already been way too long, I've decided to give this a quick look.
It seems like everything should be good to go mostly. I've only got a bit of feedback on some remaining style issues.
font/src/directwrite/mod.rs
Outdated
device_pixel_ratio: f32, | ||
collection: FontCollection, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the font fallback list, correct? I think collection
might not be ideal naming since a collection might be all kinds of things. On Linux this is called fallback_lists
for example (since every style has a different fallback list).
Or is this a collection of all fonts present on the system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the collection of fonts the use. Usually the system collection - and that's what's loaded in ::new()
.
(The dwrote FontCollection
is a wrapper type around the DirectWrite interface)
This used to be called font_collection
but we took the prefix font_
off most names in the file.
How about available_fonts
? Though I also don't think the name collection
(or something including collection) is bad because it leads readers to the directwrite "collection" concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually when I think collection, I'm directed more torwards std
than directwrite.
I think available_fonts
would be a good choice, it seems to correctly represent what is stored.
Co-Authored-By: Christian Duerr <contact@christianduerr.com>
Co-Authored-By: Christian Duerr <contact@christianduerr.com>
Thanks, have applied suggestions and rebased. Couple of bits of input needed from you to decide how to finish things off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change the name of the two variables this is good to go.
That's done 👍 As there's a lot of commits on this branch, probably worth squashing as part of the merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your work.
I just want to thank you all for making it! Thank you! |
I'd also love for this to be released, i spent hours puzzling why my powerline icons were not rendering thinking it was something wrong with my fonts. I'll see if i can try build it manually from source but that's likely painful on windows. |
I can confirm building from source is not too difficult on Windows and
worth the effort. It seems like fixing font fallback would be a minor
change but I found this to be a significant improvement.
…On Wed, 6 May 2020 at 16:39, Simon Taylor ***@***.***> wrote:
I'd also love for this to be released, i spent hours puzzling why my
powerline icons were not rendering thinking it was something wrong with my
fonts. I'll see if i can try build it manually from source but that's
likely painful on windows.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABX2PQAK6D3DISSLTVVKKMLRQEAZJANCNFSM4J737YHQ>
.
|
For anyone struggling with this, I was able to resolve it with these steps
I feel like the other font installation methods I was using were not working correctly. Suspect there is more to installing fonts in windows than just double clicking on the font files and choosing install. But I did also try other installation methods such as this which did not work either. Installing fira code through choco seems to have done the magic. This is also useful #957 |
This is the first step for #3082
At the moment Alacritty on windows does not use any font fallbacks, so glyphs for emoji cannot be found. I suspect there are many other cases where lack of font fallbacks causes glyphs to be missing.
By adding font fallbacks, we now can rasterize (downsampled) greyscale emoji. They don't look amazing, but hey, something is better than nothing!
The remaining step to add full emoji support is to implement color font rasterizing, which will be a much bigger change and might need upstream support in the
dwrote
crate.Perhaps others may have ideas how to improve the render quality of these greyscale emojis too.
If this PR looks like the right direction to be heading, there's a couple of things for me to finish off:
Possible bugs from testing:
Figure out how to size emoji correctly - downsample?Investigate emoji paste issues?(Upstream bug COOKED_READ (cmd.exe) doesn't properly support emoji input microsoft/terminal#1503)Investigate occasional missing glyph(Sounds a lot like upstream bug Bad characters occasionally displayed when writing lots of identical UTF-8 lines microsoft/terminal#386)Before:
After:
Fixes #3215.