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

Improve undercurl rendering for HiDPI displays #3637

Merged
merged 1 commit into from May 21, 2021

Conversation

disrupted
Copy link
Contributor

@disrupted disrupted commented May 17, 2021

Fixed/Improved rendering of curly underlines on HiDPI (i.e. Retina) displays.

I took the existing algorithm for the anti-aliased line (Xiaolin Wu) to calculate an upper & lower bound and applied a solid fill in between. This way the amplitude & weight of the undercurl is correctly scaled for HiDPI displays.

Feedback welcome :)

Comparison

@1x Before

kitty_undercurl_before@1x

@1x After (unchanged)

kitty_undercurl_after@1x

@2x Before

kitty_undercurl_before@2x

  • Issue: Curly underline is thinner than normal and appears flatter (less curly)

@2x After

kitty_undercurl_after@2x

  • Change: Undercurl has correct thickness and curly-ness :)

@disrupted disrupted marked this pull request as ready for review May 17, 2021 23:16
@kovidgoyal
Copy link
Owner

If you are changing the height of the curl by removing the // 2, that would make the curl too thick at normal DPI. You would need to make it conditional on the dpi. Furthermore in this case the height of the normal underline should also be scaled. The best place to do that would be in core_text.m line 58. Multiply underline thickness by fg->logical_dpi_y / 72.0

That will cause all underlines to be thickened not just the curl. Although I can find no documentation suggesting that CTFOntGetUnerlineThickness returns a value that is DPI independent and needs to be scaled.

And then there is the question of underline position, if we are scaling thickness, should we scale positiona s well? Can you check regular underline thickness and position in some cocoa native apps on DPI to try to answer this question.

@disrupted
Copy link
Contributor Author

disrupted commented May 18, 2021

Hi Kovid! Thank you for your feedback.

If you are changing the height of the curl by removing the // 2, that would make the curl too thick at normal DPI. You would need to make it conditional on the dpi. Furthermore in this case the height of the normal underline should also be scaled. The best place to do that would be in core_text.m line 58. Multiply underline thickness by fg->logical_dpi_y / 72.0

I am testing this with 2 monitors, one of which is 4K (HiDPI @2x) and the other one 1080p (standard @1x scaling factor).
I think there was an issue before, that the height of the curly underline wasn't scaled correctly to @2x. It appeared smaller/less curly compared to standard @1x scaling which can be seen in the before screenshot above. I found that removing the // 2 solved the issue for me, but perhaps this is not the best solution yet. (should test this for possible side effects and different resolutions, @3x?)

The thickness parameter which gets passed to add_curl() already seems to have the correct value. (1 for 1x dpi, 2 for 2x)

When I changed the underline_thickness to self->underline_thickness = CTFontGetUnderlineThickness(self->ct_font) * fg->logical_dpi_y / 72.0;

the thickness parameter was calculated as 3 for @2x display scaling.

That will cause all underlines to be thickened not just the curl. Although I can find no documentation suggesting that CTFOntGetUnerlineThickness returns a value that is DPI independent and needs to be scaled.

good point! But straight underlines already seem to be scaled to the correct thickness when I view them on @2x. I assume the CTFOntGetUnderlineThickness already handles the scaling then.

And then there is the question of underline position, if we are scaling thickness, should we scale positiona s well? Can you check regular underline thickness and position in some cocoa native apps on DPI to try to answer this question.

I will look into this. I think the current logic of positioning it at the the very bottom of the cell is already good in all cases, but I'll try and compare it to some other apps.

@kovidgoyal
Copy link
Owner

Cool once you have checked out the positioning and settled on a fix,
ping me again and I will take another look. Also please squash your
commits into one, easier for me to review that way.

@disrupted
Copy link
Contributor Author

disrupted commented May 18, 2021

@kovidgoyal this is ready for review now.

After testing I found that placing the undercurl at the very bottom of the cell isn't always desired, especially when setting larger cells through adjust_line_height, because then there could be a big blank space between text and undercurl.

I changed the positioning logic to be more consistent across those cases.

@kovidgoyal kovidgoyal merged commit c5afe4e into kovidgoyal:master May 21, 2021
@disrupted disrupted deleted the feat/undercurl-retina branch June 10, 2021 11:40
@akinsho
Copy link

akinsho commented Jul 11, 2022

I'm sorry for posting on a closed PR, I was looking around this repo to see if there was anything related to an issue I was having which seems to be the same as the OP (@disrupted) except I'm on a 16-inch MacBook and don't seem to get the benefit of this PR. My current undercurls in kitty look a lot more like the before photo than the after. They are very thin and not particularly curly, making them less visible/impactful

My kitty screenshot:

image

Result of the PR 👇🏿
image

If this is better opened as a new issue please let me know and sorry if I should have done that to start. It just seemed like this was the fix for me, but doesn't seem to work. It doesn't seem to be gated behind any specific flag or anything, so presumably using a retina macbook should be enough to get this behaviour?

@kovidgoyal
Copy link
Owner

You are running kitty 0.25.2? Press ctrl+shift+f6 and post the output. Also try running kitty with kitty --config NONE and see if you can reproduce.

@akinsho
Copy link

akinsho commented Jul 11, 2022

Yep I'm running 0.25.2. The output is

kitty 0.25.2 created by Kovid Goyal
Darwin Akins-MBP 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:37 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T6000 arm64
ProductName:	macOS ProductVersion:	12.4 BuildVersion:	21F79
Frozen: True
Paths:
  kitty: /Applications/kitty.app/Contents/MacOS/kitty
  base dir: /Applications/kitty.app/Contents/Resources/kitty
  extensions dir: /Applications/kitty.app/Contents/Resources/Python/lib/kitty-extensions
  system shell: /bin/zsh
Loaded config files:
  /Users/akin/.config/kitty/kitty.conf

Config options different from defaults:
active_tab_title_template          {sup.index} ❐ {fmt.fg.red}{bell_symbol}{activity_symbol}{fmt.fg.tab}{title} {fmt.fg._5B6268}{fmt.nobold}({num_windows})
adjust_line_height                 3
allow_remote_control               y
bold_font                          Cartograph CF Bold
bold_italic_font                   Cartograph CF Extra Bold Italic
confirm_os_window_close            2
copy_on_select                     clipboard
cursor_stop_blinking_after         10.0
disable_ligatures                  1
enable_audio_bell                  False
enabled_layouts                    ['tall:bias=55;full_size=1', 'stack', 'fat', 'grid', 'horizontal']
env:
{'PATH': '/opt/homebrew/bin/:/usr/local/bin/:/Applications/kitty.app/Contents/MacOS:/usr/bin:/bin:/usr/sbin:/sbin'}
focus_follows_mouse                True
font_family                        Cartograph CF Light
font_size                          14.2
hide_window_decorations            2
inactive_text_alpha                0.8
italic_font                        Cartograph CF Light Italic
listen_on                          unix:/tmp/kitty
macos_option_as_alt                2
macos_quit_when_last_window_closed True
macos_thicken_font                 0.75
mouse_hide_wait                    15.0
placement_strategy                 top-left
pointer_shape_when_dragging        hand
resize_draw_strategy               1
scrollback_lines                   10000
startup_session                    /Users/akin/Dropbox/kitty/startup.conf
tab_bar_align                      center
tab_bar_edge                       1
tab_bar_style                      separator
tab_separator                          
tab_title_template                 {sup.index} {fmt.fg.red}{bell_symbol}{activity_symbol}{fmt.fg.tab}{title}
window_border_width                (1.0, 'pt')
Added mouse actions:
	cmd+left release ungrabbed → mouse_handle_click link
	cmd+left release grabbed → mouse_handle_click link
Added shortcuts:
	f1 → show_kitty_env_vars
	ctrl+h → kitten pass_keys.py neighboring_window left   ctrl+h
	ctrl+j → kitten pass_keys.py neighboring_window bottom ctrl+j
	ctrl+k → kitten pass_keys.py neighboring_window top    ctrl+k
	ctrl+l → kitten pass_keys.py neighboring_window right  ctrl+l
	ctrl+shift+space → toggle_layout stack
	ctrl+shift+; → detach_window ask
	ctrl+shift+\ → toggle_fullscreen
	ctrl+shift+a → kitty_shell window
	ctrl+shift+d → scroll_page_up
	ctrl+shift+e > c → edit_config_file
	ctrl+shift+y → scroll_page_down
	cmd+. → move_tab_forward
	cmd+d → launch --type=overlay --cwd=${HOME}/.dotfiles zsh -c 'nvim'
	cmd+p → previous_tab
Removed shortcuts:
	ctrl+shift+a > 1 → set_background_opacity 1
	ctrl+shift+a > d → set_background_opacity default
	ctrl+shift+a > l → set_background_opacity -0.1
	ctrl+shift+a > m → set_background_opacity +0.1
	ctrl+shift+e → open_url_with_hints
Changed shortcuts:
	ctrl+shift+- → decrease_font_size
	ctrl+shift+. → next_layout
	ctrl+shift+= → increase_font_size
	ctrl+shift+c → new_tab
	ctrl+shift+g → kitten hints --type=linenum --linenum-action=tab nvim +{line} +{path}
	ctrl+shift+q → close_window
	ctrl+shift+s → focus_visible_window
	ctrl+shift+u → input_unicode_character
	ctrl+shift+v → launch zsh -c 'nvim'
	ctrl+shift+w → quit
	ctrl+shift+x → close_tab
	ctrl+shift+backspace → restore_font_size
	cmd+, → move_tab_backward
	cmd+- → decrease_font_size
	cmd+1 → combine : launch --cwd=~/projects/work/tumelo/mono/ --type=os-window zsh -c 'nvim; $SHELL' : launch
	cmd+= → increase_font_size
	cmd+n → next_tab
Colors:
	active_border_color                #46d9ff   
	active_tab_background              #16181c   
	active_tab_foreground              #bbc2cf   
	background                         #242730   
	color0                             #2a2e38   
	color1                             #ff665c   
	color10                            #99bb66   
	color11                            #ecbe7b   
	color12                            #51afef   
	color13                            #c678dd   
	color14                            #46d9ff   
	color15                            #bbc2cf   
	color2                             #7bc275   
	color3                             #fcce7b   
	color4                             #51afef   
	color5                             #c57bdb   
	color6                             #5cefff   
	color7                             #dfdfdf   
	color8                             #484854   
	color9                             #ff665c   
	cursor                             #bbc2cf   
	cursor_text_color                  #242730   
	foreground                         #bbc2cf   
	inactive_border_color              #484854   
	inactive_tab_background            #16181c   
	inactive_tab_foreground            #5b6268   
	selection_background               #6a8fbf   
	selection_foreground               #bbc2cf   
	tab_bar_background                 #16181c   

Important environment variables seen by the kitty process:
	PATH                                /Applications/kitty.app/Contents/MacOS:/usr/bin:/bin:/usr/sbin:/sbin
	LANG                                en_GB.UTF-8
	SHELL                               /bin/zsh
	USER                                akin

@kovidgoyal that seems much better with --config NONE
image

I tried changing the font before posting, so it wasn't that, although I guess some other setting I have enabled might be the root cause

@kovidgoyal
Copy link
Owner

Should be easy to bisect kitty.conf to find the culprit

@akinsho
Copy link

akinsho commented Jul 11, 2022

@kovidgoyal so I managed to boil it down to the main font family, it seems to not be specific to just my particular font, Cartograph CF since I tried with a patched Fira Code, I also happened to have Jet brains mono installed and that worked fine, that and the default font which I believe is Menlo on MacOS.

It's evidently not, but I had assumed this feature i.e. undercurl thickness would be agnostic of the font (speaking completely out of my depth here)

@akinsho
Copy link

akinsho commented Jul 11, 2022

The default behaviour here via --config NONE is definitely the solution I was looking for though

@akinsho
Copy link

akinsho commented Jul 11, 2022

As a quick note, I thought it might relate to the font weight but moving to a thicker font weight i.e. LightDemi Bold didn't change the undercurl behaviour

@kovidgoyal
Copy link
Owner

It's not font related, it will be font metrics related. You can probably
workaround it by using the adjust_cell_width/height settings.

@akinsho
Copy link

akinsho commented Jul 11, 2022

Hmm there are no explicit settings for adjusting cell height and width, at least named as such, e.g. adjust_cell_height I tried adjust_line_height and adjust_column_width neither of which made a difference.

@kovidgoyal
Copy link
Owner

yeah i got the name wrong, I think in terms of cells. The curls are
rendered programmatically without any reference to font (other than cell
size and baseline). See the add_curl() function. I am guessing the
problematic font has a very low baseline or underline thickness so there
isnt enough space to draw it thick. You can check that my running from
source and adding prints to add_curl

@akinsho
Copy link

akinsho commented Jul 11, 2022

@kovidgoyal did a bit of logging, these are the arguments to add_curl and got:

Cartograph CF - Position: 36, cell width: 18, cell height: 48, thickness: 1
JetBrains Mono Medium - Position: 34, cell width: 18, cell height: 41, thickenss: 2

Seems the cell height is lower for Jetbrains, but the thickness is greater. Tried following it through a bit but still not sure how thickness is determined

@kovidgoyal
Copy link
Owner

thickness is a property of the font. fonts specify the underline
thickness they want for underlines.

@akinsho
Copy link

akinsho commented Jul 11, 2022

Hmm, that makes sense, but is somewhat unfortunate. I guess I can ask the font author if they'd be open to changing it, but I wonder if this is something that can be overriden in kitty, since I don't know all the reasons and use cases they might have for choosing that thickness.

Also, this seems like it could happen at any point if I change my font, and they choose some thickness that produces this behaviour, for example if I switched to fira code which also behaves like my current font here. If it was a user option, could this then be something I set on my machine that would work for all fonts, granted I'm using the same screen resolution etc.

@kovidgoyal
Copy link
Owner

It's not something I care to make configurable, patches welcome. But
note that if you are going to do this, I will want to make the baseline,
underline position, strikethrough position, underline thickness and
strikethrough thickness configurable.

@akinsho
Copy link

akinsho commented Jul 11, 2022

That seems like it overlaps quite a lot with #4723 if I'm not mistaken?

@kovidgoyal
Copy link
Owner

yes, that PR is similar

@akinsho
Copy link

akinsho commented Jul 11, 2022

@kovidgoyal not sure if I will fare better than other people that have attempted this, but I'm pretty keen on this functionality. Just to clarify some comments, you made on that PR, i.e.

Change the size by a percentage of existing size or by pts
Change the underline, strikethrough positions, by font units
Change the thickness of underline/strikethrough

  1. There you refer to changing the size (is this of the font) separate to the font_size? Here, you mention the baseline instead? Is this different to adjust_baseline which already exists
  2. You mention a modify_font setting similar to font_features there are not many other options similar to this I think. Did you mean for this to be specified as a series of space separated strings i.e. how did you imagine an example of this setting looking modify_font underline_thickness:3 there seems to be a lot of precedent for just numeric options which seems like this would easily fall under with a common prefix for grouping them like modify_font_underline_thickness or something?
  3. Is this feature essentially all or nothing or is there a more minimal subset here that would be acceptable, in the interest of reducing scope as much as possible to make this easier to implement (smaller PR etc.)

@kovidgoyal
Copy link
Owner

  1. Yes. The idea is that font_size sets the cell size, but since actual text might be rendered by many different fallback fonts, you can adjust the size of these to work well with the size of the main font.
  2. modify_font * underline_thickness 3pt (* because this applies to all fonts)
    modify_font FontPostScriptName adjust_size -5%
  3. Yes, it's all or nothing, if we are modifying font rendering, then I want a general framework for it otherwise in the future I would just have to change things which is much more effort thatn doing it right from the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants