Skip to content
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

AtlasEngine rendering wide Nerd Font glyphs incorrectly #14022

Closed
SivanagBalla opened this issue Sep 16, 2022 · 13 comments · Fixed by #14959
Closed

AtlasEngine rendering wide Nerd Font glyphs incorrectly #14022

SivanagBalla opened this issue Sep 16, 2022 · 13 comments · Fixed by #14959
Assignees
Labels
Area-AtlasEngine In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Comments

@SivanagBalla
Copy link

SivanagBalla commented Sep 16, 2022

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

image
image

Actual Behavior

image
image

@SivanagBalla SivanagBalla added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 16, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 16, 2022
@T4P4N
Copy link

T4P4N commented Sep 17, 2022

same here

@lhecker lhecker self-assigned this Sep 20, 2022
@lhecker lhecker changed the title UbuntuMono Nerd Font rendering double wide font incorrectly AtlasEngine: Nerd Font rendering double wide glyphs incorrectly Sep 25, 2022
@lhecker lhecker changed the title AtlasEngine: Nerd Font rendering double wide glyphs incorrectly AtlasEngine rendering wide Nerd Font glyphs incorrectly Sep 25, 2022
@lhecker
Copy link
Member

lhecker commented Sep 25, 2022

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.

@lhecker lhecker added this to the 22H2 milestone Sep 25, 2022
@lhecker lhecker added Product-Terminal The new Windows Terminal. Priority-2 A description (P2) labels Sep 25, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 25, 2022
@lhecker lhecker removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 25, 2022
@stephc-int13
Copy link

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
Copy link
Member

lhecker commented Sep 25, 2022

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.

image

@stephc-int13
Copy link

stephc-int13 commented Sep 25, 2022

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
Copy link
Member

lhecker commented Sep 25, 2022

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
Copy link

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
Copy link
Member

lhecker commented Sep 25, 2022

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
Copy link

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
Copy link

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

@jessey-git
Copy link

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
Copy link
Member

lhecker commented Sep 27, 2022

@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:
image

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

@lhecker lhecker removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 12, 2022
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Oct 12, 2022
@djdv
Copy link

djdv commented Mar 9, 2023

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

This was resolved by a commit in their patcher ryanoasis/nerd-fonts#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants