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 proper underline and strikethrough support #1078

Open
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@chrisduerr
Collaborator

chrisduerr commented Feb 1, 2018

Support for strikethrough has been added by inserting and removing a
STRIKE_THROUGH flag on the cell.

Now all strikethrough and underline drawing is also done through the
rectangle renderer. So no glyphs are used to render underlines and
strikethrough.
The position is taken from the font metrics and should be accurate for
linux, however is not yet tested on macos.

It works by checking the underline state for each cell and then drawing
from the start until the last position whenever an underline ended. This
adds a few checks even if no underline is rendered but I was not able to
measure any significant performance impact.

Fixes #806.
Fixes #31.

Demo:

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 1, 2018

Here are the results of the benchmarks I've run:

Master
Benchmark #1: alt-screen-random-write
  Time (mean ± σ):      5.433 s ±  0.032 s    [User: 5.200 s, System: 1.888 s]
  Range (min … max):    5.381 s …  5.476 s
Benchmark #1: scrolling
  Time (mean ± σ):      5.562 s ±  0.087 s    [User: 5.419 s, System: 5.623 s]
  Range (min … max):    5.415 s …  5.714 s
Benchmark #1: scrolling-in-region
  Time (mean ± σ):      5.533 s ±  0.062 s    [User: 5.359 s, System: 5.638 s]
  Range (min … max):    5.399 s …  5.615 s
Benchmark #1: startup
  Time (mean ± σ):      85.7 ms ±   2.2 ms    [User: 73.7 ms, System: 15.0 ms]
  Range (min … max):    83.0 ms …  91.0 ms
Benchmark #1: unicode-random-write
  Time (mean ± σ):      5.494 s ±  0.649 s    [User: 5.469 s, System: 0.155 s]
  Range (min … max):    4.476 s …  6.671 s
Underline + Strikethrough
Benchmark #1: alt-screen-random-write
  Time (mean ± σ):      5.409 s ±  0.015 s    [User: 5.164 s, System: 1.930 s]
  Range (min … max):    5.382 s …  5.431 s
Benchmark #1: scrolling
  Time (mean ± σ):      5.516 s ±  0.064 s    [User: 5.350 s, System: 5.623 s]
  Range (min … max):    5.432 s …  5.615 s
Benchmark #1: scrolling-in-region
  Time (mean ± σ):      5.528 s ±  0.072 s    [User: 5.364 s, System: 5.614 s]
  Range (min … max):    5.432 s …  5.665 s
Benchmark #1: startup
  Time (mean ± σ):      85.3 ms ±   1.9 ms    [User: 72.8 ms, System: 16.0 ms]
  Range (min … max):    82.0 ms …  89.6 ms
Benchmark #1: unicode-random-write
  Time (mean ± σ):      5.836 s ±  0.454 s    [User: 5.804 s, System: 0.175 s]
  Range (min … max):    5.032 s …  6.367 s
Benchmark Script
#!/bin/bash

# Read the command line arguments
if [[ "$#" -lt 2 ]]; then
    echo "Usage: bench.sh <TERM> <OUT_DIR>"
    exit 1
fi
term="$1"
dir="$2"
echo "Benchmarking '$1' and writing results to '$2'"
echo ""

# Install hyperfine benchmarker
echo "Searching for hyperfine"
if hash hyperfine 2>/dev/null; then
    echo "Found existing hyperfine install"
else
    echo "Installing hyperfine"
    cargo install hyperfine
fi
echo ""

# Create directory for the benchmarks
if [ -d "$dir" ] || [ -f "$dir" ]; then
    echo "Removing existing output directory"
    rm -rf "$dir"
fi
echo "Creating new output directory"
mkdir "$dir"
echo ""

# Generate the benchmarks, aiming for ~5 secs
echo "Generating benchmarks"
echo "Generating alt-screen-random-write benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 599999999 alt-screen-random-write > "$dir/alt-screen-random-write.vte"
echo "Generating scrolling benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 50000000 scrolling > "$dir/scrolling.vte"
echo "Generating scrolling-in-region benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 50000000 scrolling-in-region > "$dir/scrolling-in-region.vte"
echo "Generating unicode-random-write benchmark"
cargo run --release -- -w $(tput cols) -h $(tput lines) -sb 800000 unicode-random-write > "$dir/unicode-random-write.vte"
echo ""

# Run the benchmarks
echo "Running benchmarks"
echo "Running startup benchmark"
hyperfine --style 'basic' "$term -e true" > "$dir/startup.bench"
echo "Running alt-screen-random-write benchmark"
hyperfine --style 'basic' "$term -e cat $dir/alt-screen-random-write.vte" > "$dir/alt-screen-random-write.bench"
echo "Running scrolling benchmark"
hyperfine --style 'basic' "$term -e cat $dir/scrolling.vte" > "$dir/scrolling.bench"
echo "Running scrolling-in-region benchmark"
hyperfine --style 'basic' "$term -e cat $dir/scrolling-in-region.vte" > "$dir/scrolling-in-region.bench"
echo "Running unicode-random-write benchmark"
hyperfine --style 'basic' "$term -e cat $dir/unicode-random-write.vte" > "$dir/unicode-random-write.bench"
echo ""

echo "All benchmarks completed successfully"
echo "You can find the results in $dir/*.bench"

All benchmarks with color support used a small patch which randomly generates SGR sequences instead of just changing color so underline and strikethrough is actually tested.
@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Feb 1, 2018

As usual for your PRs, super awesome!

I tested my font (Consolas) as well as another one that I like (Source Code Pro), I have two comments/questions:

  1. Underline seems to "jump vertically" depending on a font, wouldn't it be visually nicer to have it exactly in line with the bottom part of block cursor? Have a look at "Consolas - PR", the underline is so much away from the text that it even feels closer to the letters "ikth" of the word "strikethrough" below.

  2. The strikethrough like feels too high in "Source Code Pro - PR" example, especially comparing to "Consolas - PR" or the image you showed in your demo.

image

IMO LibreOffice renders both fonts more beautifully, underline is closer to the text in Consolas and strikethrough is centered in Source Code Pro.

image

Closer comparison:

image

image

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 1, 2018

For the underline I'm taking the metrics that are provided by the font itself, so those should be okay in theory.

However with the strikethrough I'm just taking the cell height and applying strike-through half way through the cell. It might be a better approach to take the font baseline and the ascent and drawing the strikethrough half way through there.

Thanks for the quick feedback. :)

EDIT: What I haven't thought about is the actual alignment of the underline with a width that is not 1. It looks to me like the Consolas underline is more than one pixel wide. Right now I just assumed that the position of the underline is the top corner of it. However if it would be the middle or even the bottom the underline of Consolas would be higher. I'll look into that again, it's probably somewhere in the freetype documentation.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 2, 2018

According to the docs:
When displaying or rendering underlined text, this value corresponds to the vertical position, relative to the baseline, of the underline bar's center.

So I'm currently still doing it wrong because I'm positioning the underline relative to the baseline, of the underline bar's top. Not relative to the baseline, of the underline bar's center.
This should help moving the underline in the Consolas example from @maximbaz a bit up.

I can't find any specifics on strikethrough though.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 2, 2018

After looking a bit more into it, it seems like there is no "standard" way for rendering the strikethrough. So where exactly the strikethrough line is placed seems to vary significantly from terminal to terminal. From the ones I've looked at none seems perfect.

Another thing I've noticed is that the underline position and thickness are always the same, no matter what font size. I'm not entirely sure if that's correct, because fonts with a huge underline offset (Hack has -4) will lead to the underline leaving the glyph box at small sizes.

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Feb 2, 2018

Huh, interesting discovery about the underline position - to illustrate (Consolas):

image

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Feb 5, 2018

The underline raised a little higher with the latest commit, do you have some ideas about how to fix the underline being so low when the font size is very small? I'm just curious, given the benefits and that it works for usual font sizes I personally think it's fine to merge this even in the current state.

This is font size of 4, Consolas:

image

10 (my regular font size):

image

12 (too large for me, but look how the position of underline perfectly aligns with the cursor):

image

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 5, 2018

I'll be honest here, the state this PR is in right now is pretty much completely unusable with some font/size combinations. And I have no idea how exactly freetype underline and position is supposed to work.

I'll have to look more into drawing underline with freetype, maybe check out how some other applications do it. But as long as underline position/width is always the same with all font sizes, this will not work properly.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 15, 2018

Apparently the undreline position and thickness are specified in font units, not device pixels, so they need to be converted first.

There also is an exact specification for strikethrough position and thickness using the freetype::tt_os2::TrueTypeOS2Table struct. This gives us access to the y_strikeout_size and y_strikeout_position methods.

I'll update this PR with a reworked version of this as soon as possible, so others can test it with their font setup.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 15, 2018

Underline has been reworked, however there's still one small caveat:

To make sure the underline doesn't disappear with small fonts, the
minimum underline thickness has been set to `1`. This decision was made
because the `Hack` font has an underline thickness under `0.5` with
relatively big fonts.

This edge-case can lead to some odd behavior when making the font size
super tiny (smaller than readable) so just not rendering any underline
might be a viable alternative.

I'd love some feedback on the issue if underline should always be rendered.

Strikethrough will hopefully be up tomorrow. :)

@maximbaz

This comment has been minimized.

Contributor

maximbaz commented Feb 16, 2018

Very cool, now the position of the underline stays relatively on the same place regardless of the font size.

Also, a nice accident, using Consolas with my regular font size of 10 the underline perfectly matches the bottom of the box cursor, just like I wanted 🙂 This isn't necessarily true for all fonts, but it's a pleasant surprise.

In fact, the perfect match is for font sizes from 1 to 12, and starting from font size 13 it goes out of sync by one pixel... but it doesn't matter.

underline

And this is Source Code Pro:

underline-source-code-pro

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 16, 2018

I'd classify the first GIF as a bug. It might be up to freetype standards (not sure, I'd have to look into it), but the underline should never be even a single pixel below the cell height.

Since the cell height is the full height of the line, any pixel below that is part of the next line and should never contain content from the previous one. It's probably just a rounding/edge case thing, thanks for always being so quick to test things. :)

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 16, 2018

The current breakage on macos is intentional. I will need some time to test this with a VM.

chrisduerr added some commits Dec 24, 2017

Make visual bell flash color customizable
This addresses the main feedback in /pull/430. I've
decided to go from scratch instead of basing my work on top of what
markandrus has already implemented to keep it as simple as possible.

If there's any stuff that I should take from the other PR, please let me
know. I can also try to send a PR to markandrus.
Improve ability of alacritty to deal with broken config
Until now alacritty completely refuses to start when the config is broken
in any way. This behavior has been changed so the worst-case is always
that alacritty launches with the default configuration.

When part of the config is broken, alacritty shouldn't instantly try to
recover to the default config, but instead try to use defaults only for
the parts of the config which are broken. This has also been implemented
for most of the fields in the configuration. So it should be possible that
parts are broken, but the rest is still used for the configuration.

This fixes #954.
Add clippy check to travis
This commit adds clippy as a required step of the build process. To make
this possible, all existing clippy issues have been resolved.
Draw visual bell as big quad over everything
A method `render_rect` has been added that allows drawing a rectangle
over anything in the terminal. This is used by the visual bell to draw a
big rectangle over everything.

To achieve this a new shader has been added. Shaders are now kept around
until the program is dropped. Then whenever a rectangle draw call is
issued, the shader is swapped from the default shader to the rectangle
drawing shader.

After the shader has been swapped and the rectangle has been drawn,
uniforms (like projection) and blending function are reset to make sure
the rendering can be continued on the default shader.

All current instances of visual_bell in the rendering have been removed,
it is now all done exclusively through the rectangle rendering function.

Currently the shaders are swapped every time `render_rect` is called, so
when rendering multiple rectangles (potentially for underline /
strikethrough) there would be a loads of shader swaps that could be
potentially very expensive.

So before using `render_rect` for a different application, the shader
swap should be removed from the `render_rect` method.
As long as the visual bell's alpha value is 0, this has no effect on
performance because no rectangle is rendered.
Swap programs instead of shaders
Instead of swapping shaders, now there are two programs kept around that
are swapped out when needed.

This has the benefit of skipping the expensive linking process that is
required after swapping the shaders on a program.
Remove `ShaderProgram::(de)activate` abstraction
The abstraction for activating and deactivating shaders has been removed
because the single-line abstraction did not add much with a second
shader program added.

Because RectShaderProgram was added too, it required maintaining four
different methods for activating and deactivating shaders.
Add separate rectangle drawing method
In preparation for potential other users of the rectangle drawing, a new
method has been added which moves all the setup and teardown outside of
the render_rect method.

This change will hopefully make it more efficient to draw loads of
rectangles in one frame and prevent excessive program swaps.
Switch viewport before rendering rectangles
To make sure it is possible to draw over the complete window, the
viewport is removed before rendering rectangles now.
This makes the visual bell behave properly and flash over the padding of
the terminal too, instead of just flashing the grid.

However when rendering a rectangle now, the padding has to be taken into
consideration.
Render underline/strikethrough as rects
Support for strikethrough has been added by inserting and removing a
`STRIKE_THROUGH` flag on the cell.

Now all strikethrough and underline drawing is also done through the
rectangle renderer. So no glyphs are used to render underlines and
strikethrough.
The position is taken from the font metrics and should be accurate for
linux, however is not yet tested on macos.

It works by checking the underline state for each cell and then drawing
from the start until the last position whenever an underline ended. This
adds a few checks even if no underline is rendered but I was not able to
measure any significant performance impact.

Fixes #806.
Fixes #31.
Rework line position
Line position has been reworked to be more accurate to freetype
specification.

However, the strikethrough position is not documented in freetype so a
weird hack has been chosen for now, further improving this depends on
getting an accurate underline first.

Underlines seem messed up because freetype is reporting a fixed
underline position no matter how big the font size. This can lead to the
underline leaving the glyph box with small fonts and a proportionally
tiny underline with big fonts.
Apply scaling to underline
Until now the underline position and thickness has been used without
applying any scaling. However they are both specified in font units
which do not change with the font size.

To make sure the underline looks correct with every size of the font,
the thickness and position are now scaled by the `x_scale` stored in the
font metrics.

This does not fix strikethrough yet, even though it looks significantly
better, the implementation is still a hack.

To make sure the underline doesn't disappear with small fonts, the
minimum underline thickness has been set to `1`. This decision was made
because the `Hack` font has an underline thickness under `0.5` with
relatively big fonts.
This edge-case can lead to some odd behavior when making the font size
super tiny (smaller than readable) so just not rendering any underline
might be a viable alternative.
Add proper strikeout positioning
With this change the strikeout should make proper use of the metrics in
the OS2 table. These metrics are available for all TTF and OTF fonts. If
the metrics are not supplied, a fallback strikeout based on underline
thickness and cell height is used.

To go in line with the metrics supplied by the OS2 table, the term of
`strikethrough` has been replaced with `strikeout` everywhere. I think
it makes sense to go forward with using `strikeout` in general when
talking about this.

There has also been a small bug where the cell would not be
underlined/striked out when an underline is broken over multiple lines.
This has been fixed and the underline/strikeout should always be applied
to each cell with this flag.

@chrisduerr chrisduerr force-pushed the chrisduerr:underline-strikethrough branch from 39fbf2a to a0270d9 Feb 16, 2018

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Feb 16, 2018

@maximbaz To check if the font specifies an incorrect underline position/thickness or if there's some kind of off-by-one error, it would really help if you would give me the metrics for the situation where the underline goes one pixel (or more) beyond the block cursor's bottom line.

Here's a patch you can apply to this PR (a0270d9) for printing the metrics to stdout:
https://hastebin.com/kafalerami

Add strikeout metrics for macos
Macos doesn't provide their own strikeout metrics, so instead of using
the font metrics a fallback is used.

Since this seems to be stored separately on freetype too, there might be
a different way to get strikeout position and thickness (without CoreText)
@petobens

This comment has been minimized.

petobens commented Mar 10, 2018

Hi! Not sure if the strikethrough is already implemented on mac but it doesn't seem to be working:

screen shot 2018-03-10 at 11 12 38

Again, I might've understood wrongly your last commit (I believe it adds strikethrough support on mac) so if that's the case sorry about the noise. Thanks for the beautiful work you are all doing with Alacritty :)

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 10, 2018

I can not test macos because I am currently not able to get any macos VM to work. If the metrics are calculated properly it will also work on macos, however I am not able to test the metrics so I can't implement the macos part of this.

@petobens

This comment has been minimized.

petobens commented Mar 10, 2018

@chrisduerr thanks for the info!

@jwilm

This comment has been minimized.

Owner

jwilm commented Mar 14, 2018

@chrisduerr would you mind committing the script that generates this?

Demo:

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 14, 2018

echo -e "RESET \e[4;32m UNDERyLINE \e[0m RESET \e[9;31m STRIKEyTHROUGH \e[0m RESET \e[9m\e[4;35m BOTH till the endy of the line "
@hlieberman

IMO this is ready to be merged.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 21, 2018

MacOS still needs to get the proper underline and strikethrough metrics. Until then I would recommend against merging this. It shoud be simple to implement, it's just a matter of time to find someone who can test on macos.

@malramsay64

This comment has been minimized.

malramsay64 commented Mar 21, 2018

This is what it looks like on macOS with the font Iosevka, other fonts I have tried have similar behavior.

screen shot 2018-03-22 at 07 18 34

I calculated the initial position metrics as below.

-----------
CELL_HEIGHT: 37
DESCENT: 7
UNDERLINE POS: -1.0527344
UNDERLINE THI: 2.1191406
STRIKETHROUGH POS: 25.5
STRIKETHROUGH THI: 2.1191406
CALC POS: -1.0527344
CALC THI: 2.1191406
-----------

By modifying the position metrics moving the underline down by - descent and changing the strikeout from + descent to - descent the position of the underline and strikethrough appear in reasonable positions over a range of font sizes (patch here )

screen shot 2018-03-22 at 08 00 52

With position metrics

-----------
CELL_HEIGHT: 37
DESCENT: 7
UNDERLINE POS: -8.052734
UNDERLINE THI: 2.1191406
STRIKETHROUGH POS: 11.5
STRIKETHROUGH THI: 2.1191406
CALC POS: -8.052734
CALC THI: 2.1191406
-----------

Additionally with some font sizes there appears to be issues with the x positioning of the underline and strikethough. With the strikethrough going into the reset and starting partway through the B in the below image.

screen shot 2018-03-22 at 07 59 25

Edit: The above image is with the font Monaco not Iosevka

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 21, 2018

Thanks a lot for looking into this! No clue why the X position is wrong, I'll have to look into it again when I have the time. Do you use any glyph offsets?

And your patch seems to work well enough, can you test it with massive font sizes too? I've noticed on Linux that my original approach used unscaled positions, so please make sure that the width of the underline gets bigger with bigger fonts and the position stays reasonable.

Also feel free to send a PR to my branch if you want to, it would be great if someone could add the final tweaks to macos so we can land this. :)

malramsay64 added some commits Mar 21, 2018

Modify y positioning of UL + SO on macOS
This moves the underline down by `- descent` and changes the sign of the
movement of strikeout from `+ descent` to `- descent`.
Round underline position and size to integer
This is to maintain consistency with font/src/ft/mod.rs, rounding the
positions and sizes of the underline and strikethrough to the nearest
integer.

Additionally this sets a minimum bound of the line size to 1, ensuring
the line is drawn for small font sizes.
@malramsay64

This comment has been minimized.

malramsay64 commented Mar 22, 2018

At font size 72 on a retina display

screen shot 2018-03-22 at 11 29 17

I got mixed up with the testing I did above. The issues with the X positioning are both font and font size dependent. Iosevka, and Fira Code work fine across the entire range I tested while Monaco has the positioning issue.

PR Sent!

malramsay64 and others added some commits Mar 22, 2018

Use round instead of floor for UL position
The round is used in the freetype implementation.
@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Mar 25, 2018

It seems like font.offset, font.glyph_offset and window.padding all aren't handled properly and can lead to underline/strikethrough not matching the glyph positions.

Either way this should not get merged until #963 is accepted.

This was referenced Mar 29, 2018

@mpcsh

This comment has been minimized.

mpcsh commented Jul 17, 2018

Hey, what's the status on this? Contemplating switching to alacritty but the lack of underline is bugging me. I'm happy to contribute/remote-pair if you're interested @chrisduerr.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Jul 17, 2018

@mpcsh Since this is a bigger change it will probably have to wait until the scrollback branch is merged before I can rebase and merge this.

@petobens

This comment has been minimized.

petobens commented Oct 2, 2018

Is it possible to add undercurl support? (tmux will eventually support it as per tmux/tmux#1492)

BTW: since the scrollback branch is merged is this PR going to be rebased/prioritize? (it's a really nice feature :))

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Oct 2, 2018

@petobens That would be significantly more work than a simple underline. And I don't think many terminal emulator support that yet. I also don't think it's standardized in any way.

So this is definitely not within the scope of the PR and if you are interested in it you should open a separate issue.

@alexanderjeurissen

This comment has been minimized.

alexanderjeurissen commented Nov 7, 2018

@chrisduerr I was looking into underline support remembering seeing a PR for it a while back (which is this PR 😄) In July you mentioned wanting to await merge-in of the scroll-back branch, is this branch dependent on some changes on that branch ? or what is holding us back from rebasing this and merging it in ?

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Nov 7, 2018

The primary priority right now is #1403, which depends on an update to https://github.com/tomaka/glutin.

After that I can keep working my way through the open PRs. I've finished #1188 just today and #1190 is another one which will definitely be merged before this.

Once there are no newer PRs left open, I'll get to this.

@nakulj

This comment has been minimized.

nakulj commented Nov 12, 2018

Would it be possible to have underlines skip descenders? Chrome started doing this with URL underlines a little while ago and now I don't want to go back.

skip_descenders

Perhaps this could be a configurable, but it would be nice to have the option.

@chrisduerr

This comment has been minimized.

Collaborator

chrisduerr commented Nov 12, 2018

Based on initial estimation, I'd say that's out of scope. It seems way too complicated for what it's worth. If Alacritty ever starts using some kind of font library which helps with this, maybe. But it's not looking like this is going to happen anytime soon (or ever).

@chrisduerr chrisduerr added this to the Version 0.2.5 milestone Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment