AtlasEngine rendering wide Nerd Font glyphs incorrectly

SivanagBalla commented 2 years ago

Windows Terminal version

1.16.2523.0

Windows build number

10.0.19044

Other Software

UbuntuMono Nerd font installed from https://github.com/ryanoasis/nerd-fonts/releases/download/v2.2.2/Ubuntu.zip WSL, running Ubuntu 20.04.1 LTS. SSHing into a RHEL server The linux server is running tmux with custom status bar with "double wide" icon characters

Steps to reproduce

Use UbuntuMono Nerd Font and configure some double wide character in status bar or print them in terminal

Issue is seen with new AtlasEngined enabled(which is default with new version). Disabling AtlasEngine will bring back old behaviour

Expected Behavior

Double wide characters such as clock, calender which are available in NerdFond are to be rendered as below

Actual Behavior

T4P4N commented 2 years ago

same here

lhecker commented 2 years ago

I gave this issue a title that will hopefully make it easier to find in the future. 🙂

FYI I'm currently working on a version of AtlasEngine that solves this issue fundamentally, by going with a more traditional text rendering approach, allowing for freely overlapping glyphs. My prototype is already capable of handling all Nerd Font glyphs correctly. Unfortunately however it comes with its own set of downsides (slightly slower, more complex, doesn't support user-defined text shaders), so the jury is still out on when and in what shape we'll fix this issue. Don't worry though: Until it's been fixed, the old text renderer will not go away.

stephc-int13 commented 2 years ago

I gave this issue a title that will hopefully make it easier to find in the future. 🙂

FYI I'm currently working on a version of AtlasEngine that solves this issue fundamentally, by going with a more traditional text rendering approach, allowing for freely overlapping glyphs. My prototype is already capable of handling all Nerd Font glyphs correctly. Unfortunately however it comes with its own set of downsides (slightly slower, more complex, doesn't support user-defined text shaders), so the jury is still out on when and in what shape we'll fix this issue. Don't worry though: Until it's been fixed, the old text renderer will not go away.

You could probably do that in two passes. First pass with the usual shader rendering standard cells, second pass rendering the overlapping cells if necessary. That is probably optimal and avoid unnecessary overhead.

lhecker commented 2 years ago

You could probably do that in two passes. First pass with the usual shader rendering standard cells, second pass rendering the overlapping cells if necessary. That is probably optimal and avoid unnecessary overhead.

That would mean I'd have to keep both, the complexity of our current approach and add the complexity of the new approach for fallback. That would probably make maintainability more difficult than a larger one time change. An idea I had to keep some of the current benefits was to just composite all text into a viewport-sized offscreen texture and blend it onto the swap chain in a single final shading pass, similar to how we do it now. BTW to give some context on the "slightly slower" / overhead I mentioned: The new approach I've prototyped runs within my test setup at ~200us per frame, whereas the old one needs ~100us per frame. Sure, that's twice as slow, but I'm not even using instancing yet to render glyph quads (or Nvidia's QuadFillMode), nor are we anywhere close to the 500-1000us that the old renderer needs. 😅 I still got quite a number of kinks to iron out, but once that's done I'll discuss with everyone on the team how to best integrate such changes.

stephc-int13 commented 2 years ago

I don't think you need to fall back on per glyph rendering to solve the special wide glyphs needed for proper nerdfont icons rendering.

I understand that the speed difference is not significant, IMHO your solution is more complex than doing two passes with shader on a quad.

First pass as already done. (Ignoring wider glyphs) Second pass only on "special" glyphs with wider cells. ( with almost the same shader, not per glyph quads)

It might even be possible to do everything in a single pass shader, but that should be tested.

lhecker commented 2 years ago

When we render we need to conceptually draw in "layers" from bottom to top and AtlasEngine did it in 1 pass:

1st pass:
- background color
- selection (variant 1)
- cursor (variant 1 and 2)
- glyphs
- text decorations (overline, underline, strikethrough, etc.)
- sixels, etc.
- selection (variant 2)

But in order to draw wide glyphs in between we can't draw in 2 passes. We need 3, since things like sixels or selections are "on top" of the lower contents. From what I can see it basically turns into:

1st pass:
- background color
- selection (variant 1)
- cursor (variant 1 and 2)
- glyphs
- † text decorations (overline, underline, strikethrough, etc.)
- † sixels, etc.
- † selection (variant 2)
2nd pass (optional):
- overly large glyphs (don't adhere to the cell grid)
- sixels, etc.
3rd pass ‡:
- ‡ text decorations (overline, underline, strikethrough, etc.)
- ‡ sixels, etc.
- ‡ selection (variant 2)

† optional if no 2nd pass ‡ required if 2nd pass

This is my new approach:

1st pass:
- background color
- selection (variant 1)
2nd pass:
- any glyphs
- text decorations (overline, underline, strikethrough, etc.)
- sixels, etc.
- selection (variant 2)

Additionally it actually reduces the amount of code required for text rendering by ~12% (it's just that the remaining code requires more intimate knowledge about DirectWrite and Direct3D). And I think I even know how to merge both to draw them in just a single pass all the time. As such right now I personally don't agree that keeping both approaches simultaneously offers a strong benefit. I'm open to any ideas how to not draw individual quads and I'd be happy to be convinced otherwise, but even if you're right that it's simpler and better, given my knowledge I just wouldn't know how that would work.

stephc-int13 commented 2 years ago

The more I think about it the more I think that the whole rendering can be entirely done in one single shader pass.

But that does require a bit of preprocessing and adding metadata for each cell to handle all the cases. That would mostly be a few special case flags to tell the shader to look for more information in adjacent cells (top, left or bottom)

But of course, given that performance gains are not critical (the optimal solution would be a bit more power efficient) you should simply follow your guts and go for the solution that is the simpler to implement for you.

lhecker commented 2 years ago

Thanks for your input! I'll definitely try my hardest to come up with something decent. Nothing is set in stone so far. 🙂

jessey-git commented 2 years ago

I seem to have a similar problem, except that version 2523 is ok, but the most recent 2642 is broken. I'm using MesloLGM NF at size 9.0 (tried 8.5 and 9.5 as well and those were much worse).

2523 on top, 2642 on bottom: fontbrokenness

Is this the same or a different issue?

stephc-int13 commented 2 years ago

You can disable the new atlas renderer in the settings, if that solves your issue then it is related.

jessey-git commented 2 years ago

Yeah, disabling Atlas fixes the problem. Welp, it worked for almost 14 days with the prior version. It had a good run :) Will try Atlas again once this issue here is all fixed.

lhecker commented 2 years ago

@jessey-git I'm sorry I broke your font! 😔

I hope you don't mind me saying this, but your font is... weird. 😄 It looks beautiful of course, but it handles its block characters in a very unusual manner. That character (U+E0B0) works in all other Nerd Fonts I got installed (for instance Fira Code), just not with this Meslo variant, which is already a bit weird. Additionally my understanding is that block characters are expected to have approximately the same size as the font's advance width and height. While most Nerd Fonts variants are slightly incorrect in that regard, Meslo in particular is really bad about this. This particular glyph for instance is almost 40% taller than the advance height it specifies which causes it to look like that. The only difference between build 2523 and 2642 is how those glyphs are vertically centered. In both screenshots you're missing more than half of that glyph - it's actually supposed to be a perfect triangle like this:

Basically, IMO Nerd Fonts should stop making block glyphs taller than the font's advance height. In either case I'm going to simply revert the change that causes your issues: #14085

djdv commented 1 year ago

I was seeing the same rendering issue with the Powerline Extra block of glyphs, which was reported upstream: https://github.com/ryanoasis/nerd-fonts/issues/1106 (my specific configuration https://github.com/ryanoasis/nerd-fonts/issues/1106#issuecomment-1423593788 renders similar to the OP of this issue).

This was resolved by a commit in their patcher https://github.com/ryanoasis/nerd-fonts/pull/1123

After using the latest version of my font, their patcher, and Terminal, at least those glyphs in my chosen fonts seem to render as expected now.

People in this thread may want to try them as well to get their renders back to how they looked before. And it may be worth checking against their latest patcher revisions versus just the latest release when doing the Atlas refactor if it could potentially influence something here.

microsoft / terminal