undercurls (fixes #136)

[RaafatTurki]

See #136 for more info on what undercurls are. I've patched everything except for one conflict, xst compiles fine, however I still get underlines instead of undercurls.

That one conflict was this line which removes an XftDrawRect function call and replaces it with the actual implementation of the undercurls + a fallback to underlines

Here's a comparison of that function call

patch

XftDrawRect(xw.draw, fg, winx, winy + dc.font.ascent + 1, width, 1);

xst

XftDrawRect(xw.draw, fg, winx, winy + win.cyo + dc.font.ascent + 1, width, 1);

They only differ in the addition of win.cyo, I have no idea what xst patch introduced this parameter, its significance nor how it should be used within the newly patched code.

Any hint would be highly appreciated.

[actionless]

They only differ in the addition of win.cyo, I have no idea what xst patch introduced this parameter, it's significance nor how it should be used within the newly patched code.

Any hint would be highly appreciated.

$ rg cyo
x.c
88:     int cyo; /* char y offset */
1049:   win.cyo = ceilf(dc.font.height * (chscale - 1) / 2);
1319:                   yp = winy + font->ascent + win.cyo;
1565:           XftDrawRect(xw.draw, fg, winx, winy + win.cyo + dc.font.ascent + 1,
1570:           XftDrawRect(xw.draw, fg, winx, winy + win.cyo + 2 * dc.font.ascent / 3,

i leave doing git blame for answering your other question to yourself ;)

[RaafatTurki]

I think the issue is much larger than that, I've tried:

• the patch on vanilla st
• a maintained st fork that claims to have undercurl support.

on 2 of my machines, no undercurls.

This is a major roadblock, could I be doing this all wrong? anyone who does the steps above let me know what you get.

[RaafatTurki]

terminfo is not fun

[actionless]

i could help reviewing the PR when it's ready but as i'm not interested in the feature myself i can't help with developing it, sorry

[RaafatTurki]

oh no worries, I have this in the bag ... sorta

[RaafatTurki]

I've got it working now but it looks a bit different due to how st renders characters

kitty

xst

it seems like st doesn't provide an extra padding around its characters so stuff like a undercurl could fit within its bounding box. should I add the xrdb support and finalize this PR or is there an issue with the above?

[RaafatTurki]

I've selected the most wide spread (and sane) method of rendering undercurls, that has made them look nicer and not going above the characters baseline, it also scales with the font size.

10px

13px

16px

[actionless]

you have to apply that cyo y axis offset to line itself as well, i guess

[actionless]

thanks! i think now it looks good to go

[RaafatTurki]

I went this morning and implemented the rest of the patch, the cyo was the cause of the rendering issue. figured it would make the life of whoever comes after me easier plus more options so why not, xrdb support incoming.

curly

spiky

capped

[actionless]

are the latest screenshots made already with applying cyo?

after zooming them in they still dont look as good vertically adjusted as in kitty (UNLESS that's due to difference of the fonts you've used on the screenshots):

your last screenshot:

your kitty screenshot:

feels like it need to add(remove) 1 extra pixel of vertical coordinates

[actionless]

also, could you make a screenshot of text with with underline? (example_text) using the same font as you used for kitty

[actionless]

and, (here i am being like really picky) - because of using ceil instead of round - you have a rounding mistake in your formula resulting in the "tilt" of the sinewave right in the vertical middle of it:

[RaafatTurki]

the ceil has no effect on the wave tilt, however I've edited (also added cyo to relevant lines)

# x.c:1596
int wh = win.cyo + dc.font.descent - wlw/2 - 1;
to 
int wh = win.cyo + dc.font.descent - wlw/2 - 2;

and now it renders better

kitty

xst

[actionless]

regarding the tilt: it must be somewhere the round() missing, i'll try to look through the code again to guess where (it should be somewhere on computing X coordinate)

[actionless]

regarding vertical alignment - can confirm it looks uniform with kitty now :+1:

[RaafatTurki]

figured a better way to control how thick the wave is would be to let the user decide the best for their setup (kept the patch default), ill look into the tilt issue and try your suggestions now

[RaafatTurki]

The tilt seems to be directly effected by the font size which makes sense, the patch doesn't implement a sophisticated method for mapping pixel patterns to always represent a smooth curve. here are some samples:

9px

14px

15px

16px

17px

This issue is mostly apparent in 15 and 16 but it does show in larger sizes, the gist of it is the pixel plotting works flawlessly on anything less than 15 because it's so restricted the "sin wave" always becomes a square wave, but anything higher and you enter "is this font size going to produce a nice wave or is it going to leave a dangling pixel as a side effect" land.

This whole issue becomes irrelevant if you use any of the other 2 methods as they use a less float-dependant methods of rendering, which makes me think maybe I should make the default one of them?

[actionless]

it seems what just on those 2 font sizes the values getting their 'edge case' to make the rounding problem visible

but since i dont have personal interest in this thing - that won't be a blocker from merging it from my side, so there is no urge in fixing it now

[RaafatTurki]

cool, I have nothing more to add to this PR, I'll wait for the merge.

[RaafatTurki]

@neeasade review pls

[neeasade]

Looks good, thanks again