SpyrexDE / SmoothScroll

Addon for the Godot Game Engine that adds a SmoothScrollContainer.
https://spyrexde.github.io/SmoothScroll/
MIT License
144 stars 13 forks source link

Add toggle for overshoot #36

Closed dannymate closed 11 months ago

dannymate commented 11 months ago

Can disable overshoot. The scrollbar will smoothly snap to the boundary.

I also made some changes to "handle_overdrag" method logic flow. I have tested and not noticed any bugs.

2023-10-22 - Toggle Overshoot.webm

image

HaroldLever commented 11 months ago

Great changes, I'm looking forward of feature like that quite a long time. However, there might be some problem.

elif not allow_overdragging: axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity))) This might not be necessary because this function is to apply a velocity to make it scroll back exactly. I believe the original way could handle the situation which you said "Prevent out of bounds if we disallow overdrag."

The original way if dist1 > 0 and not will_stop_within(vertical, axis_velocity): is reasonable because it needs to judge dist1 > 0 then will_stop_within() should be taken into consideration. I'm not sure whether if (dist1 < 0 or dist2 > 0) and will_stop_within(vertical, axis_velocity): have done extra calculation.

And more importantly, the new function you add would still allow at least a frame to overdrag. Prehaps it would be better to predict the next position then snap to boundary. Still overdrag

(BTW, it would be even useful if this is also effective to dragging)

dannymate commented 11 months ago

elif not allow_overdragging: axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity))) This might not be necessary because this function is to apply a velocity to make it scroll back exactly. I believe the original way could handle the situation which you said "Prevent out of bounds if we disallow overdrag." I did try just setting it but it jumps to the position. If you want it to smoothly scroll I found I needed to calculate a velocity instead that brought it exactly where it needed to be.

I'm not sure what you mean by the original way? Do you mean:

if will_stop_within(vertical, axis_velocity):
    axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity)))

If so I could just but an or not allow_overdragging in the if statement there.

The original way if dist1 > 0 and not will_stop_within(vertical, axis_velocity): is reasonable because it needs to judge dist1 > 0 then will_stop_within() should be taken into consideration. I'm not sure whether if (dist1 < 0 or dist2 > 0) and will_stop_within(vertical, axis_velocity): have done extra calculation.

It's the same thing but flipped. It won't cause any extra calculations. If it's going to stop within the bounds we don't care so we should return as soon as possible. I can rewrite it instead as if not (dist1 > 0 or dist2 < 0) and will_stop_within(vertical, axis_velocity), remember if statements are considered false as soon as possible so if dist1 is greater than 0 and dist is less than 0 then it won't run will_stop_within.

And more importantly, the new function you add would still allow at least a frame to overdrag. Prehaps it would be better to predict the next position then snap to boundary.

Are you using the example scene? I'll check that out and play around there.

(BTW, it would be even useful if this is also effective to dragging)

I will check the dragging out, luckily I have a touchscreen.

HaroldLever commented 11 months ago

I'm just curious of what or not allow_overdragging is for. I tried to delete this condition and it seems there's no glitch. I guess you want axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity))) always work if allow_overdragging is unchecked. But in my design, it shouldn't work always. These few lines are for the situation when we release dragging with a high speed which is not fast enough to scroll back to bounds. I think it will work whether allow_overdragging is checked or not.

I'm not sure what you mean by the original way? Do you mean:

You're right. I must misunderstand something before.

It's the same thing but flipped. It won't cause any extra calculations. If it's going to stop within the bounds we don't care so we should return as soon as possible. I can rewrite it instead as if not (dist1 > 0 or dist2 < 0) and will_stop_within(vertical, axis_velocity), remember if statements are considered false as soon as possible so if dist1 is greater than 0 and dist is less than 0 then it won't run will_stop_within.

Yes, just simply restrict FPS to a low value like 8 and you can see the issue more obviously. Scroll it and the content will still be out of bounds at the first frame.

Are you using the example scene? I'll check that out and play around there.

dannymate commented 11 months ago

I'm just curious of what or not allow_overdragging is for. I tried to delete this condition and it seems there's no glitch. I guess you want axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity))) always work if allow_overdragging is unchecked. But in my design, it shouldn't work always. These few lines are for the situation when we release dragging with a high speed which is not fast enough to scroll back to bounds. I think it will work whether allow_overdragging is checked or not.

The original axis_velocity = -dist*(1-friction)/(1-pow(friction, stop_frame(axis_velocity))) is called after the overshoot to bring it back to the bounds smoothly. The elif not allow_overdragging only occurs if if will_stop_within is false. Therefore if we're about to stop outside of the bounds then we want to prevent that outcome and instead calculate the velocity to stop at the bound instead of going over.

HaroldLever commented 11 months ago

Ahh, I see. It's actually a design for "good" dragging feeling. When we release dragging with a high speed that allows it to go over in bounds, I let it scroll freely rather than restrict it to go back to boundary, the latter situation would make it a little "viscous" I guess.

dannymate commented 11 months ago

Ahh, I see. It's actually a design for "good" dragging feeling. When we release dragging with a high speed that allows it to go back in bounds. So I let it scroll freely rather than restrict it to go back to boundary, the latter situation would makes it a little "viscous" I guess.

Yeah normally it's good "juice" to have that overshoot there with the momentum. However, I'm trying to mimic a webpage so the effect is unwelcome. I'll fix up the bugs you've raised when I get the chance.

dannymate commented 11 months ago

I have updated my code. As you can see I changed approach entirely. Much simpler.

I instead check if the next calculated position in scroll is outside bounds and if it is just cease movement and snap to boundary. This has an effect much closer to a webpage. It fixes the issues with touch scrolling and low FPS.

I would consider this an easing option (i.e. No Easing) for https://github.com/SpyrexDE/SmoothScroll/issues/18.

2023-10-23 - Toggle Overdragging Second Approach.webm

Also quick question. Do horizontal scrollbars not work yet?

HaroldLever commented 11 months ago

Wonderful work!

Still there is some issue like in if not (dist1 > 0 or dist2 < 0) and will_stop_within(vertical, axis_velocity): , and should be or. And scroll bar size haven't taken into consideration yet like calculate_distance() do.

I believe it works, is there any issue?

Also quick question. Do horizontal scrollbars not work yet?

dannymate commented 11 months ago

I believe it works, is there any issue?

Yes, sorry I figured it out. I couldn't get the horizontal bar to show in the example, just had to mess around with the sizes. Edit: Also it says on the asset store that it doesn't support horizontal yet.

New: if not (dist1 > 0 or dist2 < 0) and will_stop_within(vertical, axis_velocity):

Old:

    var result = [axis_velocity, axis_pos]

    # Overdrag on top or left
    if dist1 > 0 and not will_stop_within(vertical, axis_velocity):
        result[0] = calculate.call(dist1)
        # Snap to boundary if close enough
        if dist1 < just_snap_under and abs(axis_velocity) < just_snap_under:
            result[0] = 0.0
            result[1] -= dist1
        return result

    # Overdrag on bottom or right
    if dist2 < 0 and not will_stop_within(vertical, axis_velocity):
        result[0] = calculate.call(dist2)
        # Snap to boundary if close enough
        if dist2 > -just_snap_under and abs(axis_velocity) < just_snap_under:
            result[0] = 0.0
            result[1] -= dist2
        return result

    return result

So if we look at the old version. The two if statements are exclusive. If one of them is true it returns. Another way of thinking about it is if both of those 'if's are false then we want to return result. So we can just flip and combine them to return earlier. So: if (not dist1 > 0 and will_stop_within) or (not dist2 < 0 and will_stop_within) return result simplify further as we have and will_stop_within twice: if (not dist1 > 0 or not dist2 < 0) and will_stop_within Move the not in the first part outside the brackets to apply to both: if not (dist1 > 0 or dist2 < 0) and will_stop_within. This is cleaner code wise in my opinion. Less duplication and easier to manage. I'm happy to remove it but the logic is the same as what was already there so if there's an issue it'll be an issue with the original code too.

HaroldLever commented 11 months ago

Yeah, I understand what you mean. Think of a simple situation, at old version, if will_stop_within() is true, it will return result directly regardless of how dist1 and dist2 are. So I think change and to or is reasonable.

So if we look at the old version. The two if statements are exclusive. If one of them is true it returns. Another way of thinking about it is if both of those 'if's are false then we want to return result. So we can just flip and combine them to return earlier.

In this example below, at old version I can scroll up and down smoothly, while at new version, it stops me from scrolling down manually until it goes back in bounds with speed that formula calculates, which is I mention "good scrolling feeling". And or Or

I notice that too, the description in assets store do needs update, perhaps sample scene as well. What do you think? @SpyrexDE

Yes, sorry I figured it out. I couldn't get the horizontal bar to show in the example, just had to mess around with the sizes. Edit: Also it says on the asset store that it doesn't support horizontal yet.

dannymate commented 11 months ago

I see what you mean. I have made the change.

SpyrexDE commented 11 months ago

Looks great.

I notice that too, the description in assets store do needs update, perhaps sample scene as well. What do you think? @SpyrexDE

I will definitely update that. Thanks for pointing it out!