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

Fix code editor scrolling experience on track pads #73502

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

Kurble
Copy link
Contributor

@Kurble Kurble commented Feb 17, 2023

Fixes #28149

This PR fixes two closely related problems:

  • Scrolling slowly in the code editor with a track pad would not do anything;
  • Smooth scrolling would animate track pad scrolls, but they are already animated as the pixel deltas are provided on a frame-by-frame basis.

The first problem is caused by the step size that's set on the VScrollBar, any scroll event going through TextEdit::_scroll_up and TextEdit::_scroll_down got rounded down to an integer number of lines. Since scrolling on a track pad happens every frame, the individual deltas are relatively small, and they would be rounded down to zero unless you're scrolling quite fast. This is addressed differently for the smooth scrolling enabled and disabled case:

  • When disabled the scrolling deltas are accumulated the same way they were already being accumulated for the smooth scrolling case, so when slow scrolling for long enough the step size threshold is reached;
  • When smooth scrolling is enabled the step size is set to 0, allowing all fractional increments.

The second problem becomes quickly apparent when the first problem is fixed: scroll delta's less than 1 line are applied immediately, while scroll delta's larger than 1 line are animated. This feels really snappy as long as the target scroll value is less than 1 line away from the current scroll value, only to suddenly change to a fixed speed when the delta becomes too big. This has been addressed by directly applying any scroll delta coming from InputEventPanGesture events, instead of allowing it to animate.

@Calinou
Copy link
Member

Calinou commented Feb 17, 2023

Reposting what I discussed on the Godot Contributors Chat:

Rounding scrolling to lines is probably not desired behavior nowadays, even with a mouse wheel (and even without smooth scrolling). For example, VS Code doesn't round to lines when scrolling with the wheel, so I think we can get rid of the behavior entirely 🙂

@Kurble Kurble force-pushed the text-edit-scrolling-precision branch from 7cf106d to 07be795 Compare February 17, 2023 20:09
@Kurble
Copy link
Contributor Author

Kurble commented Feb 17, 2023

Updated to get rid of line snapping.

@Kurble Kurble force-pushed the text-edit-scrolling-precision branch from 07be795 to 4dbe82b Compare February 17, 2023 22:14
@Kurble Kurble requested a review from a team as a code owner February 17, 2023 22:14
@Kurble
Copy link
Contributor Author

Kurble commented Feb 17, 2023

Some tests were failing because scrolling to a caret position can now result in fractional scroll values. That actually feels really weird, so I fixed it by rounding the scroll values in set_line_as_last_visible.

The set_line_as_first_visible and set_line_as_center_visible methods are not affected as they deal with integer values.

One test had to be changed since checking for equality of two doubles isn't working there because of a rounding error.

@KeatsPeeks
Copy link

Any update on this one ? Without it the editor really feels clunky with a touchpad.

@Kurble
Copy link
Contributor Author

Kurble commented May 11, 2023

@KeatsPeeks Currently just waiting for review I guess, PR should be good to go.

@Riteo Riteo mentioned this pull request May 24, 2023
@Riteo
Copy link
Contributor

Riteo commented May 25, 2023

I tested this one on my Wayland branch (#57025) and this works fine even there!

This PR really improves the code editing situation considerably on a track pad and the patch looks concise and clean.

@akien-mga akien-mga requested a review from Calinou May 25, 2023 06:28
@akien-mga akien-mga changed the title Editor: fix code editor scrolling experience on track pads Fix code editor scrolling experience on track pads Sep 19, 2023
@akien-mga
Copy link
Member

Looks fine from the input team's perspective, but we'd like some assessment from those more familiar with TextEdit or the expected behavior of trackpads.

@BoydWiglaf
Copy link

BoydWiglaf commented Sep 24, 2023

@Kurble I created an new pull request #82260 with your changes in it with the updates from the godot master branch. It seems this PR has conflicts. If you have the time to resolve the conflicts I think this PR could get pulled in soon. However, If not I have the new PR with your changes in it. I hope Im not stepping on any toes. I am new to doing these things and really would love to see this update pulled into Godot.
Thanks

@AThousandShips
Copy link
Member

AThousandShips commented Sep 24, 2023

@Kurble can you rebase your PR and fix the merge conflicts

@Kurble Kurble force-pushed the text-edit-scrolling-precision branch from 4dbe82b to 6ffa9b0 Compare September 30, 2023 08:56
@Kurble
Copy link
Contributor Author

Kurble commented Sep 30, 2023

Rebased on master and now the tests even pass without any changes!

@BoydWiglaf
Copy link

@Calinou @Paulb23 @bruvzg @godotengine/input @godotengine/gui-nodes @godotengine/tests
What else is needed on this PR?

@Calinou
Copy link
Member

Calinou commented Oct 11, 2023

I suggest testing the pull request locally and make sure it works on your end. (I don't have a macOS device with a trackpad to test this PR.)

scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
scene/gui/text_edit.cpp Outdated Show resolved Hide resolved
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@ikkerens
Copy link

ikkerens commented Oct 13, 2023

Built this from source on my Macbook Pro with M1 Max running MacOS Sonoma 14.0 and found no issues.
Overall this change makes the whole experience a lot more refined and far less sluggish (and actually works when slow-scrolling) than the currently available official release.

I hope this gets merged soon!

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master a574c02) on mouse wheel. It works as expected in the sense that I can't notice any regressions.

Note that this PR doesn't resolve #28385.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again on top of the Wayland branch (#57025), which implements InputEventPanGesture, and it still works great as last time!

Other than that I'll reiterate that the patch looks really nice and concise, great job!

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Oct 16, 2023
@akien-mga akien-mga merged commit 72b14b5 into godotengine:master Oct 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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

Successfully merging this pull request may close these issues.

Script editor doesn't track scrolling via Touchpad smoothly (or at all when scrolling is slow)
9 participants