Feat/scroll per pixel

[idelice]

I was intentionally gonna use this myself but it received some positive feedback in reddit therefore the code may be "hacky" or "ugly" (whatever you decide) and now it's here for review.

This is my attempt to enable smooth scrolling (per pixel) that's configurable with pixel_scroll true

1225.mp4

[kovidgoyal]

Please rebase on master. And the docs need to make clear this is only for scrolling kitty's own scrollback not scrolling in TUIs generally. Otherwise, looks generally OK though I have only glanced over the code. It's going to be some time before I have the bandwidth to review this properly.

[idelice]

Please rebase on master. And the docs need to make clear this is only for scrolling kitty's own scrollback not scrolling in TUIs generally. Otherwise, looks generally OK though I have only glanced over the code. It's going to be some time before I have the bandwidth to review this properly.

It's now rebased and i've updated the docs. Hope it's more clear

[alicealysia]

From what I've tested this is working really well, and I don't see any cause for concern in the code.

[alicealysia]

This is the only bit of code that I really think could be improved. Would this be a good opportunity to find some better answers here than nesting a for loop inside of 3 if statements?

Admittedly, that is outside the scope of this PR, and is frankly a minor nitpick, so I'm moving on to testing now.

[alba4k]

to be fair those variables don't really need to be assigned, as they're only read once before going out of scope. You can just add a comment like // s > 0 means upwards and compress the 3 ifs into one with ands

[DanielShuey]

🎉 nice work!

[cshuaimin]

Tested and it works! I found that the scroll direction is reversed after this change, both mouse wheel and touch pad. I'm using natural scroll from niri.

[idelice]

Tested and it works! I found that the scroll direction is reversed after this change, both mouse wheel and touch pad. I'm using natural scroll from niri.

Can't remember if the direction were reversed when i tested this but I'll take a look once i get more time

[alicealysia]

minor visual bug,

It's possible to end a scroll in a sub-pixel position. resulting in the following:

(how this text should look)

This could be resolved in a few ways, but I think that the best way would be to ensure that if no scroll input is present, then the viewport will attempt to scroll to the closest line over time.

[alba4k]

I can confirm that the scroll direction is reversed (on hyprland with and without natural scroll), as reported by others.

Otherwise, looking great! do you know if it would be possible to also implement inertial scrolling?

Furthermore, can one get rid of that blank padding above and below the text cutoff (and make the text go all the way to the end of the screen)?

unlike @alicealysia , text looks good the whole time on my display (so the scrolling until the next line should probably be optional)

and additionally, the sidebar still scrolls per-line, which could also be left this way tbh

[idelice]

I updated the PR. I can verify the scrolling behaviour is more natural now.

Please retest

[alba4k]

Yup, thanks :)

For readability, I'll repost the list of things I believe worth looking into:

• kinetic/inertial scrolling
• empty padding above/below the text
• artifacts when the text ends up at a weird sub-pixel position (can't replicate)
• sidebar still per-line (could aswell leave it as is)

[idelice]

Yup, thanks :)

For readability, I'll repost the list of things I believe worth looking into:

• kinetic/inertial scrolling
• empty padding above/below the text
• artifacts when the text ends up at a weird sub-pixel position (can't replicate)
• sidebar still per-line (could aswell leave it as is)

Is this the inertial scrolling behaviour you expected?

Note: it's hard to see when i moved my fingers away from the touchpad, but i did and it didn't hard stop scrolling. It kept going on a configured argument: inertial_scroll_decay=1.5 (1.5 in seconds)

11.mp4

empty padding above/below the text

• Regarding empty padding after/below text; what exactly do you mean by that?

artifacts when the text ends up at a weird sub-pixel position (can't replicate)

• I wont work on this unless it's a breaking change that needs to be implemented alongside the scope of this PR

sidebar still per-line (could aswell leave it as is)

• I can give it a try but wont promise anything

[kovidgoyal]

@idelice: Momentum scrolling is orthogonal to this PR leave it out of this. It's anyway on my TODO list so you can leave it to me or make a separate PR if you want to tackle it yourself. Note that momentum scrolling has a lot of parameters, friction, max speed, etc. and you have to only do it on platforms that dont support native momentum scrolling such as macOS. If you want an example implementation you can see one I wrote at: https://github.com/kovidgoyal/calibre/blob/master/src/calibre/gui2/momentum_scroll.py

[idelice]

@idelice: Momentum scrolling is orthogonal to this PR leave it out of this. It's anyway on my TODO list so you can leave it to me or make a separate PR if you want to tackle it yourself. Note that momentum scrolling has a lot of parameters, friction, max speed, etc. and you have to only do it on platforms that dont support native momentum scrolling such as macOS. If you want an example implementation you can see one I wrote at: https://github.com/kovidgoyal/calibre/blob/master/src/calibre/gui2/momentum_scroll.py

I agree that it's not in scope of this PR but i wanted to test to see if i could get it working and it seems to work in my case (macos).

i wont push these changes and will let you do the momentum scrolling.

[alba4k]

@idelice as you can see below, there is some empty space between the end of the window and the point where the text cuts off

edit: after some testing, it seems to happen whenever font_size is set to anything other than 10 or 11 (both > and <). I use 13

[cshuaimin]

Just FYI, for momentum scrolling I find that gnome-console performs pretty well.

[idelice]

If i understand it correctly and you're referring to this "empty space", then i can reproduce it with and without my changes to this PR

---

..and the same empty space is present on another terminal like macos terminal

[alba4k]

No. The space I'm referring to appears next to the bottom and top borders of the window, as if the text wasn't rendering in the whole window. In the picture you can see that the text, when the scrolling stops mid-character, it stops before the actual end of the window, leaving the text to be "cut in mid'air", which is what you see in the Screenshots

for clarification, the purple thing you see in the Screenshots is the border I have around windows

[alba4k]

other than this, tho, lgtm

can anyone replicate it? The following config should do:

pixel_scroll true
font_size 13

[kovidgoyal]

That is just window padding that happens when your window size is not a multiple of the cell size. It is not related to this PR, if you look carefully you will notice it is there in normal kitty as well, it just becomes a bit more noticeable in this case, because of the partially cutoff lines. You can remove it from the top edge by using placement_strategy top in kitty.conf

[alba4k]

yup that's it :)

but shouldn't that get disabled entirely by default whenever pixel_scroll is set to true? It makes no sense to keep it, at least vertically, as no padding is needed if we choose to cut the characters anyway

[kovidgoyal]

You can have more than one kitty window in an OS window, pixel scrolling happens independently
in every kitty window.

[alba4k]

Ok but I'm not sure I understand when the empty space would be needed with pixel scrolling even in that scenario

[kovidgoyal]

Apologies for the delay in reviewing this PR. I was very busy with work for
calibre 9. Here are my comments:

1. The memory allocated for blank_line is being leaked as it is not free'd in the dealloc() function in screen.c

2. You dont actually need blank line. It's only use is to zero a line's worth of GPU cells, which can just be done by returning NULL from render_line_for_virtual_y and when NULL use memset(0) to zero the memory.

3. I am not sure render_line_for_virtual_y is needed at all. Since we only partial scroll when history lines are present we should always have enough lines, but maybe I am overlooking something

4. Selection handling is broken. When a line with a selection is partially cut off at the top of the screen, the selection is not rendered. You can also see this by running

./test.py multicell

with the following patch

diff --git a/kitty_tests/__init__.py b/kitty_tests/__init__.py
index cc1db836b..f69d3728c 100644
--- a/kitty_tests/__init__.py
+++ b/kitty_tests/__init__.py
@@ -267,7 +267,7 @@ def tearDown(self):
         set_options(None)
 
     def set_options(self, options=None):
-        final_options = {'scrollback_pager_history_size': 1024, 'click_interval': 0.5}
+        final_options = {'scrollback_pager_history_size': 1024, 'click_interval': 0.5, 'pixel_scroll': True}
         if options:
             final_options.update(options)
         options = Options(merge_result_dicts(defaults._asdict(), final_options))

It turns on pixel scrolling for the tests and causes the above test to fail

Finally, I added momentum scrolling under Wayland, as part of that there were some changes to mouse.c, please resolve the conflict.

Other than the above it looks fine, good work!

[kovidgoyal]

I went ahead and merged this clearing all the points in my review except for selections. That can be a separate PR, or I will get to it when I have a moment. Additionally, I left render_line_for_virtual_y() in there, pending discussion.

And I made pixel scrolling the default.