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

Configuration for undercurl vertical offset #17482

Closed
smprather opened this issue Jun 28, 2024 · 16 comments · Fixed by #17501
Closed

Configuration for undercurl vertical offset #17482

smprather opened this issue Jun 28, 2024 · 16 comments · Fixed by #17501
Labels
Area-AtlasEngine Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Comments

@smprather
Copy link

image

Undercurl is colliding with the bottom of the font. Requesting a configuration to apply a y-offset from the default vertical placement.

Here are some other-term links that may prevent having to think it through from scratch.
kovidgoyal/kitty#853
microsoft/cascadia-code#395
https://wezfurlong.org/wezterm/config/lua/config/underline_position.html

@smprather smprather added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 28, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot 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 Jun 28, 2024
@smprather
Copy link
Author

WezTerm w/ default settings.

image

@smprather
Copy link
Author

Mate-Terminal
image

@lhecker
Copy link
Member

lhecker commented Jun 28, 2024

I'd like to reproduce the issue first, before adding any settings. It could be a bug in our renderer after all. Can you please tell me the following?

  • Your font family
  • Font size
  • Line height, if you set it
  • Display scale (the one you set in the Windows Settings app)

@smprather
Copy link
Author

I'm not sure how to check the font size once I change it. Opening a new session, I know that it's 10pt Hack Nerd Font Mono. Here's a look at that. Line height is default. Display scale is 100%. Grayscale and ClearType antialiasing look identical (literally identical; I tried a restart to make sure).
image

{
    "$help": "https://aka.ms/terminal-documentation",
    "$schema": "https://aka.ms/terminal-profiles-schema-preview",
    "actions": 
    [
        {
            "command": 
            {
                "action": "splitPane",
                "split": "auto",
                "splitMode": "duplicate"
            },
            "id": "User.splitPane.A6751878",
            "keys": "alt+shift+d"
        },
        {
            "command": "toggleFocusMode",
            "id": "User.toggleFocusMode",
            "keys": "ctrl+shift+f"
        },
        {
            "command": "unbound",
            "keys": "ctrl+v"
        }
    ],
    "confirmCloseAllTabs": false,
    "copyFormatting": "none",
    "copyOnSelect": true,
    "defaultProfile": "{e2a4882f-638c-41a2-86c2-2c88bfe51b07}",
    "experimental.detectURLs": false,
    "launchMode": "maximized",
    "newTabMenu": 
    [
        {
            "type": "remainingProfiles"
        }
    ],
    "profiles": 
    {
        "defaults": 
        {
            "adjustIndistinguishableColors": "always",
            "cursorShape": "underscore",
            "font": 
            {
                "face": "Hack Nerd Font Mono",
                "size": 10
            },
            "padding": "0",
            "scrollbarState": "hidden"
        },
        "list": 
        [
            {
                "commandline": "%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe",
                "elevate": false,
                "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
                "hidden": false,
                "name": "Windows PowerShell"
            },
            {
                "adjustIndistinguishableColors": "indexed",
                "backgroundImage": "C:\\Users\\epramyl\\OneDrive - Ericsson\\Pictures\\pink_floyd_dsotm.jpg",
                "backgroundImageOpacity": 0.23,
                "bellStyle": "none",
                "closeOnExit": "graceful",
                "colorScheme": "Tango Dark",
                "commandline": "%SystemRoot%\\System32\\cmd.exe",
                "cursorShape": "filledBox",
                "experimental.rightClickContextMenu": true,
                "font": 
                {
                    "face": "Hack Nerd Font Mono",
                    "size": 10
                },
                "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
                "hidden": false,
                "historySize": 0,
                "name": "Command Prompt",
                "snapOnInput": false,
                "suppressApplicationTitle": true,
                "useAcrylic": false
            },
            {
                "guid": "{2c4de342-38b7-51cf-b940-2309a097f518}",
                "hidden": true,
                "name": "Ubuntu",
                "source": "Windows.Terminal.Wsl"
            },
            {
                "guid": "{b453ae62-4e3d-5e58-b989-0a998ec441b8}",
                "hidden": false,
                "name": "Azure Cloud Shell",
                "source": "Windows.Terminal.Azure"
            },
            {
                "guid": "{51855cb2-8cce-5362-8f54-464b92b32386}",
                "hidden": false,
                "name": "Ubuntu",
                "source": "CanonicalGroupLimited.Ubuntu_79rhkp1fndgsc"
            },
            {
                "adjustIndistinguishableColors": "indexed",
                "altGrAliasing": true,
                "antialiasingMode": "cleartype",
                "bellStyle": "none",
                "closeOnExit": "automatic",
                "colorScheme": "Tango Dark",
                "commandline": "c:\\cygwin64\\bin\\bash.exe --login -i",
                "cursorShape": "filledBox",
                "experimental.retroTerminalEffect": false,
                "font": 
                {
                    "face": "Hack Nerd Font Mono",
                    "size": 11,
                    "weight": "normal"
                },
                "guid": "{95cbe99e-4035-4bc9-90a7-a7675ddb0d16}",
                "hidden": false,
                "historySize": 0,
                "icon": "C:\\cygwin64\\usr\\share\\icons\\hicolor\\256x256\\apps\\mintty.png",
                "name": "Cygwin Bash",
                "opacity": 100,
                "snapOnInput": false,
                "startingDirectory": "%USERPROFILE%",
                "tabTitle": "Cygwin Bash",
                "useAcrylic": false
            },
            {
                "adjustIndistinguishableColors": "indexed",
                "altGrAliasing": true,
                "antialiasingMode": "grayscale",
                "backgroundImage": "C:\\Users\\epramyl\\OneDrive - Ericsson\\Pictures\\pink_floyd_dsotm.jpg",
                "backgroundImageOpacity": 0.23,
                "bellStyle": "none",
                "closeOnExit": "graceful",
                "colorScheme": "Tango Dark",
                "commandline": "C:\\Windows\\System32\\OpenSSH\\ssh.exe -t 100.87.168.59",
                "cursorShape": "filledBox",
                "experimental.rightClickContextMenu": true,
                "font": 
                {
                    "face": "Hack Nerd Font Mono",
                    "size": 10
                },
                "guid": "{e2a4882f-638c-41a2-86c2-2c88bfe51b07}",
                "hidden": false,
                "historySize": 0,
                "icon": "ms-appx:///ProfileIcons/{0caa0dad-35be-5f56-a8ff-afceeeaa6101}.png",
                "name": "VDI",
                "snapOnInput": false,
                "startingDirectory": "%USERPROFILE%",
                "suppressApplicationTitle": true,
                "tabTitle": "VDI",
                "useAcrylic": false
            }
        ]
    },
    "rendering.graphicsAPI": "direct3d11",
    "schemes": [],
    "startOnUserLogin": true,
    "themes": [],
    "useAcrylicInTabRow": false,
    "wordDelimiters": " '\",=[]$*{}<>()^#!`;\t:\u2502"
}

@smprather
Copy link
Author

The collision/legibility is definitely a function of font size. But the top of the curl is definitely abutting the bottom of the font. There needs to be at least some kind of small spacing.
image

@smprather
Copy link
Author

If I super-size, I can a bit of separation.
image

@smprather
Copy link
Author

I think what I would like to see is a minimum integer number of pixels separation, no matter the font size. Even 1 pixel would be great. It would also be nice to be able to specify the pk-2-pk height of the curl, but that's just a nice to have.

@smprather
Copy link
Author

Note that the collision is not as bad if I can set the undercurl color to be different than the font color. And I do have that set in nvim. But there is some kind of problem such that color works when not in tmux, but does not work in tmux (EXCEPT in MateTerminal it does work??? You can see that in the MateTerm grab above.). One day, I'll figure it out. But for now...

@lhecker
Copy link
Member

lhecker commented Jun 28, 2024

I think there are two problems:

  • The curly underline is 1px too high up which causes it to overlap with the text. Mate, WezTerm, and Windows Terminal all draw a 3px tall wave, but we're the only one which positions it this high up.
  • We currently only do vertical anti-aliasing. This looks good on HiDPI displays and large font sizes, but at low DPIs and small font sizes it looks ugly because it's too "pixel-y". I didn't do this because I was too lazy to figure out the math for how to calculate an arbitrary point-sine-wave distance. 🙈

In other words:

  • Fix the vertical position of the curly underline
  • Add horizontal anti-aliasing

@lhecker lhecker added Help Wanted We encourage anyone to jump in on these. Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Area-AtlasEngine and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jun 28, 2024
@lhecker lhecker added this to the Terminal v1.22 milestone Jun 28, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Tag-Fix Doesn't match tag requirements labels Jun 28, 2024
@lhecker
Copy link
Member

lhecker commented Jun 29, 2024

After spending way too much time in desmos's graphing calculator I realized that I can calculate the distance to the tangent of the sin curve quite easily while being a sufficient approximation. The tangent for sin(x) is cos(x), its angle is atan(cos(x)) and the height of a right triangle with such an angle is:

$$f(x,y) = \left|y-\sin\left(x\right)\right| \cdot \cos\left(\arctan\left(\cos\left(x\right)\right)\right)$$

That's such a fantastic coincidence because cos(atan(cos(x))) can be expressed not just using sin(x) which we already need anyway, but also using a reverse square root which is super cheap:

$$f(x,y) = \left|y-\sin\left(x\right)\right| \cdot {1 \over \sqrt{2-sin(x)^2}}$$

This is what we already have:

float s = sin(data.position.x * frequency);
float d = abs(curlyLineHalfHeight - data.texcoord.y - s * amplitude);

and all that was missing was a tiny rsqrt at the end lol:

float s = sin(data.position.x * frequency);
float d = abs(curlyLineHalfHeight - data.texcoord.y - s * amplitude) * rsqrt(2 - s * s);

@o-sdn-o
Copy link

o-sdn-o commented Jun 29, 2024

We currently only do vertical anti-aliasing. This looks good on HiDPI displays and large font sizes, but at low DPIs and small font sizes it looks ugly because it's too "pixel-y".

IMO The sine wave does not need antialiasing even at a few pixels scale since it is quite symmetrical. Antialiasing only blurs it (this is just as ugly as using vertical AA for a single underline).

Here are examples of aliased wavy underlines (first with aliased glyphs and the second with AA glyph rendering)

Aliased glyphs:

screen_record_Sat_06.29.2024_12-44-41.58.aliased.mp4

Antialiased glyphs:

screen_record_Sat_06.29.2024_12-49-16.50.aa.mp4

You can play with it using built artifacts https://github.com/directvt/vtm/actions/runs/9722846266 (or just build https://github.com/o-sdn-o/vtm/tree/gui-bridge) with the following config in ~/.config/vtm/settings.xml:

<config>
    <gui>
        <antialiasing = off />
        <cellheight = 20 />
        <gridsize = 0,0 />
        <wincoor = 0,0 />
        <winstate = normal />
        <blinkrate = 400ms />
        <fontlist> <!-- Font fallback list (LF-delimited (\n), ordered). The rest of the fonts available in the system will be loaded dynamically. -->
            "Courier New\n" <!-- Primary font. Its metrics define the cell geometry. -->
            "Cascadia Mono\n"
            "Fira Code\n"
            "NSimSun\n"
            "Noto Sans Devanagari\n"
        </fontlist>
    </gui>
    <menu>
        <item id="Test" type=dtvt param="$0 -r test"/>
    </menu>
</config>

Use the --gui cli option to force it to run in GUI mode.

@smprather
Copy link
Author

Wow. No wonder WinTerm is looking so good. You guys are incredible!

For comparison, here's some examples from whatever MS Office uses to render. You can see that they've just prerendered a simple squiggle and just roll with it. And what I'm seeing in Opera at the bottom.

Seems like you could come up with a good pre-render (or render on fly + cache) for a given integer # vertical pixels, and then apply the right one given the space available. I would like to see 1-pixel of separation, which is what Opera [or WebKit/whatever] is obviously doing, But even abutment looks decent, especially if the undercurl color is different than the font.

Consolas
6pt
image

8pt
image

9pt
image

20pt
image

10pt, TextZoom=200%
image

Here's the undercurl I see while typing this in Opera.
image

@smprather
Copy link
Author

BTW, you can see that at 6pt, the Office curl does move into the bottom line of pixels of the font. Bug, IMO.

@smprather
Copy link
Author

trying__underscore in Opera.
The 1-pixel separation helps prevent heavy collision with underscores.
image

WinTerm nearly obliterates the underscore.
image

Bump-up font size.
image

@smprather
Copy link
Author

smprather commented Jul 1, 2024

Top is WezTerm, bottom is WinTerm.
I don't have WezTerm underline_position changed from default. I assume I could use it to move the curl down though
WezTerm appears to render the text on top of the curl. This makes it easy to discern _ chars, esp when coloring is working (I can get it to work in test prints, but not nvim; topic for different issue).
WinTerm is rendering curl on top to text.
I believe curl should go under text.
image

@lhecker
Copy link
Member

lhecker commented Jul 1, 2024

Hack 10pt, 100% scale:
image

Hack 10pt, 150% scale:
image

github-merge-queue bot pushed a commit that referenced this issue Jul 2, 2024
We'd previously subtract one underline-height from the curly line
offset, even though we already had subtracted its complete height.

Additionally, the pixel shader received some fine tuning:
* Shrink the stroke width so that the anti-aliasing can be seen
  all the way up to the horizontal edges of the bounding box.
* Add a phase shift to break apart the symmetry of the curve.

Closes #17482

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jul 2, 2024
DHowett pushed a commit that referenced this issue Aug 13, 2024
We'd previously subtract one underline-height from the curly line
offset, even though we already had subtracted its complete height.

Additionally, the pixel shader received some fine tuning:
* Shrink the stroke width so that the anti-aliasing can be seen
  all the way up to the horizontal edges of the bounding box.
* Add a phase shift to break apart the symmetry of the curve.

Closes #17482

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
(cherry picked from commit ad3797a)
Service-Card-Id: 92942049
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants