Handle different underlining styles supported by neovim #1469

calops · 2022-08-08T19:11:44Z

What kind of change does this PR introduce?

Feature
Fixes Enable customization of the undercurl #764

Did this PR introduce a breaking change?

No

Content

This overhauls the drawing of underlines and actually implements the
undercurl that was simulated with underdashes before.

Implemented in this PR:

underline
underdouble
underdashed
underdotted
undercurl

Points to note

Previously the underline thickness was proportional to the stroke width. This led to weird aliasing artifacts when drawing lines (especially dashes and dots) and isn't in line with how most other terminal emulators handle this. With this PR, all the lines are pixel-perfect and 1px thick. If desired, it'll be easy to add a configuration option to make it possible to configure the line thickness, but I don't recommend making it proportional to the text itself.
The undercurl looks (in my opinion) nice, but I don't really know what I'm doing here. I'm using quadratic interpolation from skia by generating points under the text every time. I haven't noticed any hangups, but I'm sure there's a better optimized way to do this if it becomes an issue.
I'm not sure if there should be an order of precedence between the different line styles (cf the TODO left in the code for now). I'm inclined to say we leave it like that, as I don't think multiple underline styles can happen at the same time in neovim (don't quote me on this though).
Maybe the gap between the dots for underdotted should be widened? I'm worried it'll look like a thin solid line on high-dpi screens.

This overhauls the drawing of underlines and actually implements the undercurl that was simulated with underdashes before. Implemented in this commit: * underline * underdouble * underdashed * underdotted * undercurl

MultisampledNight

Thank you! This is great.

By the way, how did you even manage to set the "invalid" modifier names? :hi! Normal gui=underdouble results into neovim yelling at me with "invalid value".

src/bridge/events.rs

MultisampledNight · 2022-08-09T14:21:05Z

src/renderer/grid_renderer.rs

+            UnderlineStyle::UnderCurl => {
+                let p1 = (p1.x, p1.y - 2.);
+                let p2 = (p2.x, p2.y - 2.);
+                underline_paint
+                    .set_path_effect(None)
+                    .set_anti_alias(true)
+                    .set_style(skia_safe::paint::Style::Stroke);
+                let mut path = Path::default();
+                path.move_to(p1);
+                let mut i = p1.0;
+                let mut sin = -2.;
+                while i <= p2.0 {
+                    sin *= -1.;
+                    i += 4.;
+                    path.quad_to((i - 2., p1.1 + sin), (i, p1.1));
+                }
+                canvas.draw_path(&path, &underline_paint);
+            }


Your concerns about HiDPI and the curl appearing too thin are rightfully:

I think this could be patched up by making the line slightly thicker, doing less iterations of the loop and doing larger steps each time, though this is indeed tricky. To be honest, I don't think we'll be coming to a really aesthetic solution there.

Yeah I was afraid of this.

To be fair: no terminal I've tested handles it any better than what we're seeing here. So, maybe we could just release this like this and then improve upon it in some way?

A quick win would be to make the line thickness a configuration option, as well as maybe the pattern length (which can be used to scale the dashes and dots as well). Trying to automatically render this the best way seems like a doomed idea, or at least much harder than the rest.

Allowing configuration seems like the most sensible option. There's lots of cases where relying on the scale factor the system gives us is wrong.

Another idea would be to actually scale with the font height, but round the numbers so that they stay pixel perfect. Everything should still be 1px thick at low DPIs, but it should scale nicely after that, without any aliasing shenanigans. I'll see if I can come up with a quick example of this.

Just a note, how currently everything looks like with HiDPI

Alright, so, I worked on this a little and I'm a bit stuck.

On one hand, automatic scaling looks pretty nice:

On the other hand... this can't actually work consistently and this seems to be a limitation of the whole rendering strategy of neovide.

The issue is that everything is drawn asynchronously through a message queue, which works under the premise everything that'll get drawn (within a window) is on its own independent region of the screen, meaning there's no conflict anywhere.

But to draw thick underlines, we need them to be drawn between two lines. They can't be confined to the line the text belongs to, or they will simply strike through it at higher thicknesses (especially the undercurl). But by removing them from the line's canvas so they don't get clipped, they get randomly overwritten by the line below, leaving only two pixels to work with.

What's needed would be to redraw the underlines alone every time the line of text below them gets drawn. It seems pretty hard to do with how the code is currently set up, unless there's an easy solution I'm missing?

I think you're right, draw_foreground only ever gets called with text fragments, and not the whole current text buffer. This is yet another issue which could be handled through keeping state about the whole text visible at all times, and also redrawing the line below when drawing underlines. But as of now, Neovide doesn't have such a facility.

For now, the line below "sometimes" overlapping the curl is as far as you can get without taking quite a few hoops in terms of re-writing code architecture, I fear.

It's a little better when I:

Draw lines from the bottom up instead of the top down when drawing everything

Redraw the line above after any line that's individually drawn

But there are still plenty of glitches here and there. I'm hopeful they can be ironed out, but it'll all just be hacks for now.

Alright. Then feel free to push your changes, I'll create an issue about possible ways to approach the larger needed change after merging this PR.

Yeah that's sensible.

For now, the automatic scaling works well so long as line thickness doesn't get above 1. Above that, underlines get clipped here and there. Perhaps it would make sense to put the automatic scaling behind a configuration option for now? I'll push those changes up for review.

calops · 2022-08-09T14:24:12Z

By the way, how did you even manage to set the "invalid" modifier names? :hi! Normal gui=underdouble results into neovim yelling at me with "invalid value".

The latest neovim versions have aligned with vim's names. Therefore, underlineline became underdouble, for example. Maybe for stability we should handle both versions of the names?

MultisampledNight · 2022-08-09T14:27:36Z

Yeah, this is fair. In this case you should be able to just match both patterns using the | operator.

MultisampledNight

Text rendering has one issue with lines not being redrawn when window size changes, this can be reproduced easily by changing the window size in floating window mode:

Else, one

:let g:neovide_underline_automatic_rescaling=v:true
:hi! Keyword gui=undercurl

later, I can confirm that clipping of the undercurl does happen, but I don't think it's too bad for now:

Nice work regardless.

src/editor/window.rs

src/renderer/grid_renderer.rs

MultisampledNight · 2022-08-12T21:55:16Z

(also, small sidenote since formatting seems to be quite your nemesis: check out :h g:rustfmt_autosave, which will format the file automatically upon writing in neovim)

MultisampledNight

Looks great! One last thing I'd like to have addressed before merging, though I'll just patch it myself quickly.

website/docs/configuration.md

MultisampledNight · 2022-08-13T18:57:34Z

Alright. Truly, thank you! I'll merge this now.

Rémi Labeyrie and others added 3 commits August 8, 2022 20:56

Handle different underlining styles supported by neovim

40601c9

This overhauls the drawing of underlines and actually implements the undercurl that was simulated with underdashes before. Implemented in this commit: * underline * underdouble * underdashed * underdotted * undercurl

run rustfmt

ace0a2b

Run cargo fmt

997c1bc

MultisampledNight reviewed Aug 9, 2022

View reviewed changes

MultisampledNight self-assigned this Aug 9, 2022

rlabeyrie added 3 commits August 9, 2022 16:31

handle both old and new versions of names for underlines

76c85b4

remove TODO

b33cf93

add automatic scaling of underlines, disabled by default

abeec1d

MultisampledNight reviewed Aug 12, 2022

View reviewed changes

src/editor/window.rs Outdated Show resolved Hide resolved

src/renderer/grid_renderer.rs Outdated Show resolved Hide resolved

rlabeyrie added 4 commits August 13, 2022 00:26

run cargo fmt

b709b8d

use correct reverse iterator on range

79d9a63

match undercurl wave length with font width

4c99435

add documentation for new underline scaling option

d82d3f3

MultisampledNight reviewed Aug 13, 2022

View reviewed changes

website/docs/configuration.md Outdated Show resolved Hide resolved

List default value in setting example

6735909

MultisampledNight merged commit 38229f8 into neovide:main Aug 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle different underlining styles supported by neovim #1469

Handle different underlining styles supported by neovim #1469

calops commented Aug 8, 2022 •

edited

MultisampledNight left a comment

MultisampledNight Aug 9, 2022

calops Aug 9, 2022

MultisampledNight Aug 9, 2022

calops Aug 9, 2022

MultisampledNight Aug 9, 2022

calops Aug 11, 2022

MultisampledNight Aug 12, 2022

calops Aug 12, 2022 •

edited

MultisampledNight Aug 12, 2022

calops Aug 12, 2022

calops commented Aug 9, 2022

MultisampledNight commented Aug 9, 2022

MultisampledNight left a comment

MultisampledNight commented Aug 12, 2022

MultisampledNight left a comment

MultisampledNight commented Aug 13, 2022

Handle different underlining styles supported by neovim #1469

Handle different underlining styles supported by neovim #1469

Conversation

calops commented Aug 8, 2022 • edited

What kind of change does this PR introduce?

Did this PR introduce a breaking change?

Content

Points to note

MultisampledNight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calops Aug 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calops commented Aug 9, 2022

MultisampledNight commented Aug 9, 2022

MultisampledNight left a comment

Choose a reason for hiding this comment

MultisampledNight commented Aug 12, 2022

MultisampledNight left a comment

Choose a reason for hiding this comment

MultisampledNight commented Aug 13, 2022

calops commented Aug 8, 2022 •

edited

calops Aug 12, 2022 •

edited