editor: Smooth scrolling#44827
Conversation
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
|
Thanks for this PR, feels smooth. Are there any plans to integrate this into vim-mode as well? (e.g., or is this not part of this PR? |
|
@niekdomi yep, I still need to make the changes. I'm currently perfecting the scroll animation |
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
the set_scroll_position of the editor saves to the db the position and creates event which is wasteful for an animation Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
Signed-off-by: Marco Mihai Condrache <52580954+marcocondrache@users.noreply.github.com>
ttytm
left a comment
There was a problem hiding this comment.
Amazing effort!
Hope giving input already doesn't bother you.
Taking it through manual testing, I found that it doesn’t work with the optional preceding [count].
For example, my personal default for C+e / Ctrl+y is to scroll 5 lines. Currently it is reverted to 1 line when smooth scrolling is enabled.
No matter which scrolling motion is pressed [count] is ignored.
I wouldn’t want this to be a blocker, though. So a fix could also be part of a separate PR.
|
@ttytm, thank you for the feedback. I'm primarily focusing on improving the animation, especially for trackpads. I'm waiting for some input from the team before I make significant changes and dive deep into the vim crate |
62fbef4 to
c929736
Compare
|
there seems to be a bug/edge case: If I enter a negative value for the duration, zed crashes. I think there should be some kind of clamp |
MrSubidubi
left a comment
There was a problem hiding this comment.
Sorry for the delay, as you know, was out sick and still a bit slow to pick up on some things.
Next round here, some nits although I'd argue we better are safe than sorry here, given that this reaches really deep. There is also the following issue, which we obviously need to figure out, where trackpad scrolling on my Mac is not acceptable with the native acceleration from the OS:
Bildschirmaufnahme.2026-03-09.um.22.24.54.mov
Which I think might very well be related to one of my comments.
Nonetheless, we are getting closer though and I'd say after this round we are getting to the topic of tests, so feel free to start thinking about them (not necessarily add them just yet, but you are obviously free to already start working on these). Arguably, I think we want to test stuff amongst the lines of
- expected vs. calculated scroll positions on a per-frame basis.
- change in direction during animation
- instant scroll request during scroll animation
- animation scroll request during animation scroll request
- animated scroll after animated scroll to ensure state is in sync (can perhaps be part of the first test
- settings update test during animation - perhaps we might not need this, but what I'd like to test here is that we do not end up in a state we cannot escape
and perhaps some other nice cases you can think about. As said, food for thought, I think it might be easier to tacke these after this batch here.
| pub enum ScrollAnimationPhase { | ||
| Intermediate, | ||
| Final, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug)] | ||
| pub struct ScrollAnimation { |
There was a problem hiding this comment.
I think the animation phase should be pub(crate) at best, the state itself should perhaps even only be visible within this file even?
There was a problem hiding this comment.
The animation must be at least pub(crate) since it's used in the EditorSnapshot. Should be fixed
| } | ||
| } | ||
| }; | ||
| let (anchor, top_row) = self.calculate_scroll_anchor(scroll_position, map, cx); |
There was a problem hiding this comment.
Let's revert the change here since calculate_scroll_anchor is only called once now.
There was a problem hiding this comment.
Super right, should be done
| // Whether to animate scrolling with a smooth easing effect. | ||
| "enabled": false, | ||
| // Duration of the animation | ||
| "duration": 0.125, |
There was a problem hiding this comment.
I am not entirely sure we should make the duration configurable at all - I am sold on having the behavior change based on the input type used, and feel like making the duration configurable might be better to do at a later step if at all. This feels like a case where good defaults might just be more worth than to make this configurable in the first place. The fact that VSCode does not have a setting for this makes me feel like we might not need it as well or at least not in the first place.
There was a problem hiding this comment.
That makes sense. The main reason I added it initially was that scrolling felt much slower when smooth scrolling was enabled. Later I discovered that increasing the sensitivity fixes that, so it definitely makes sense to remove it.
Should be done 😊
| self.scroll_anchor.scroll_position(&self.display_snapshot) | ||
| } | ||
|
|
||
| pub fn scroll_target_or_position(&self) -> gpui::Point<ScrollOffset> { |
There was a problem hiding this comment.
Can we not just have scroll_position, do we really need to distinguish these here?
There was a problem hiding this comment.
We don't need to distinguish them anymore, should be done
| pub placeholder_display_snapshot: Option<DisplaySnapshot>, | ||
| is_focused: bool, | ||
| scroll_anchor: SharedScrollAnchor, | ||
| pub scroll_animation: Option<ScrollAnimation>, |
There was a problem hiding this comment.
Is there a good reason for this to be pub?
There was a problem hiding this comment.
Should be done
| if self.is_finished() { | ||
| self.phase = ScrollAnimationPhase::Final; | ||
| self.position = self.target_position; | ||
| } else { | ||
| let progress = self.progress(); |
There was a problem hiding this comment.
We calculate the progress twice here in rapid succession - Perhaps we can have progress return a newtype ScrollAnimationProgress(f32) that then has the method is_finished on that so we avoid doing this twice?
There was a problem hiding this comment.
Nice idea, I have created ScrollAnimationProgress
| impl ScrollAnimation { | ||
| pub fn instant(target: gpui::Point<ScrollOffset>) -> Self { | ||
| Self { | ||
| phase: ScrollAnimationPhase::Final, | ||
| duration: Duration::ZERO, | ||
| position: target, | ||
| start_position: target, | ||
| target_position: target, | ||
| start_time: Instant::now(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at this and the surrounding methods, I can't help but wonder whether we should manage the state here using an enum as opposed to the struct - that way, we can get rid of plenty of the duplication here and can avoid some invalid states a bit better.
This might be more verbose in parts, but might also help more with what the current state is? Since ScrollAnimationPhase::Final basically implies position == target_position for example. Would like your view on this obviously
There was a problem hiding this comment.
That’s a really nice idea, not sure why it didn’t come to my mind earlier. Looks a lot better now
| let snapshot = editor.snapshot(window, cx); | ||
| let scroll_position = snapshot.scroll_target_or_position(); | ||
| let old_top_row = DisplayRow(scroll_position.y as u32); | ||
| let old_top_column = scroll_position.x as u32; |
There was a problem hiding this comment.
This is much better already, thank you.
We should make scroll_manager private as part of this then though - if these methods can no longer be used freely due to these changes, we should ensure that no one can actually use them (and again (sorry), ensure that visibility of methods matches what we enforce here). I know our policies on comments, but for this case, might even be worth to have a comment within editor to ensure peole do not easily violate that.
There was a problem hiding this comment.
That's right. I checked where scroll_manager is accessed, and it looks like the agent_ui crate is still accessing it directly. Should we address that in this PR?
There was a problem hiding this comment.
I think to save you on conflicts, I'd be best if we could move this out into another PR. I'll get that one in then ASAP
| } else { | ||
| ScrollAnimationPhase::Intermediate | ||
| } | ||
| } |
There was a problem hiding this comment.
I think one issue we might face here is when we invoke this method, we reset the current acceleration, which I am not sure is what we want - you lose the current scroll momentum as we start the easing method from the very beginning again, which means we might be in the mid of easing - and then are at the very start again. Instead, I think we should consider the current acceleration/speed at which we are animating and try to find a suitable progress at which we can continue the animation/perhaps change the duration slightly. So this should not be a hard restart I believe but instead more calculate a new animation based on the current state.
There was a problem hiding this comment.
That’s also why increasing the sensitivity makes such a big difference. I’ve added an initial implementation that computes the velocity of the current animation using the derivative of the easing function, and then adjusts the duration of the next animation accordingly. The approach is inspired by Chromium’s smooth scrolling implementation (ref 1, ref 2).
The results are already much better than the previous approach, but I think with some additional tuning and research we can improve it even further.
|
|
||
| let base_scroll_position = if is_precise { | ||
| editor.scroll_manager.cancel_animation(); | ||
| current_scroll_position | ||
| } else { | ||
| position_map | ||
| .snapshot | ||
| .scroll_animation | ||
| .map(|animation| animation.target_position) | ||
| .unwrap_or(current_scroll_position) | ||
| }; |
There was a problem hiding this comment.
Could you briefly elaborate on the logic here?
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
@MrSubidubi I've removed this logic it's not necessary anymore. It was an attempt to understand the scrolling slowness difference between mouse and trackpad.
00be4d6 to
49f54fc
Compare
49f54fc to
0d8fd32
Compare
1a8fa0f to
97119a7
Compare
This comment was marked as spam.
This comment was marked as spam.
b9928b1 to
b2d2f6a
Compare
|
@marcocondrache do you have some time to rebase this branch and resolve the conflicts? |
MrSubidubi
left a comment
There was a problem hiding this comment.
I agree this is at a good point to resolve conflicts - the PR is in very good shape now and I'd love to see this progress further. Currently, at a high level, see only three things left:
- the comment below
- tests as mentioned in the prior round
- how we want to handle trackpad scrolls - if we can, I am thinking about whether it would make sense to distinguish between already smoothed scrolls and non-smoothed scrolls on the platform level, or do you have a better idea for that? Would love your opinion on this
| let snapshot = editor.snapshot(window, cx); | ||
| let scroll_position = snapshot.scroll_target_or_position(); | ||
| let old_top_row = DisplayRow(scroll_position.y as u32); | ||
| let old_top_column = scroll_position.x as u32; |
There was a problem hiding this comment.
I think to save you on conflicts, I'd be best if we could move this out into another PR. I'll get that one in then ASAP
3126841 to
e9bd4e5
Compare
|
@MrSubidubi @seanstrom I've resolved the conflicts and merged For everyone following the status of this PR: I’ve recently been pulled into leading a project at my current job, so I have very little, if any, time to dedicate to open source right now. Since this PR is nearly ready and it’s something the community really values, I’m aiming to get this closed out soon. Apologies for this becoming a bit stale and for the lack of progress recently. I appreciate everyone’s patience |
e9bd4e5 to
3addcc6
Compare
Closes #4355
Before:
zed-no-smooth.mp4
After:
zed.-.smooth.scroll.mp4
Ref: https://pavelfatin.com/scrolling-with-pleasure
Release Notes: