Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58318

jasonwinterpixel · 2022-02-19T14:00:58Z

Adds scroll wheel sensitivity, pan gesture sensitivity and damping settings to scroll container so that users can tune these values.

The values that are currently hard coded into godot are the default values of these new settings.

Follow Focus buffer is a vector2 that describes extra space that the 'follow focus' setting will use to track the focused control. This makes it so that when the scroll container follows the focus, it tries to scroll a bit past the focused control so it's not 'barely' visibile.

3.x version: #58319

Bugsquad edit: This partially addresses godotengine/godot-proposals#3975 (only for ScrollContainer, not other nodes).

scene/gui/scroll_container.cpp

jasonwinterpixel · 2022-03-20T13:51:50Z

Okay. I added PROPERTY_HINT_RANGE to this branch and the 3.x version for the new properties.

akien-mga · 2022-04-26T10:35:14Z

doc/classes/ScrollContainer.xml

+			If [code]true[/code], the ScrollContainer will automatically scroll to focused children (including indirect children) to make sure they are fully visible. Use in combination with [member follow_focus_buffer] to specify how far past the control to scroll.
+		</member>
+		<member name="follow_focus_buffer" type="Vector2" setter="set_follow_focus_buffer" getter="get_follow_focus_buffer" default="Vector2(0, 0)">
+			When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll, so that the focused control is not barely visibile.


Suggested change

When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll, so that the focused control is not barely visibile.

When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll, so that the focused control is not barely visible.

Aside from the typo, "is not barely visible" sounds a bit awkward to me. Does it mean "is not visible at all", or "is fully visible"?

Original behaviour is to just barely scroll the scroll container enough to have the focused element in view, this allows you to set a buffer to scroll past the focused container. Not sure what a better wording is.

Maybe even just "When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll."

akien-mga · 2022-04-26T10:37:11Z

scene/gui/scroll_container.cpp

+	return sensitivity;
+}
+
+void ScrollContainer::set_scroll_sensitivity(float _sensitivity) {


Parameters should start with p_.

akien-mga · 2022-04-26T10:38:11Z

scene/gui/scroll_container.cpp

+	if (_sensitivity < 0.0) {
+		WARN_PRINT("scroll_wheel_sensitivity cannot be negative");
+		_sensitivity = 0.0;
+	}


This can be an error condition instead:

Suggested change

if (_sensitivity < 0.0) {

WARN_PRINT("scroll_wheel_sensitivity cannot be negative");

_sensitivity = 0.0;

}

ERR_FAIL_COND(p_sensitivity < 0.0);

akien-mga · 2022-04-26T10:38:20Z

scene/gui/scroll_container.cpp

+	sensitivity = _sensitivity;
+}
+
+void ScrollContainer::set_follow_focus_buffer(Vector2 buffer) {


Suggested change

void ScrollContainer::set_follow_focus_buffer(Vector2 buffer) {

void ScrollContainer::set_follow_focus_buffer(Vector2 p_buffer) {

akien-mga · 2022-04-26T10:38:33Z

scene/gui/scroll_container.cpp

+	return damping;
+}
+
+void ScrollContainer::set_scroll_damping(float d) {


Suggested change

void ScrollContainer::set_scroll_damping(float d) {

void ScrollContainer::set_scroll_damping(float p_damping) {

akien-mga · 2022-04-26T10:38:43Z

scene/gui/scroll_container.cpp

+	if (_sensitivity < 0.0) {
+		WARN_PRINT("pan_gesture_sensitivity cannot be negative");
+		_sensitivity = 0.0;
+	}


akien-mga · 2022-04-26T10:39:48Z

scene/gui/scroll_container.cpp

+		// original sensitivity was 0.125
+		float scaled_sensitivity = sensitivity * 0.125;


Maybe sensitivity should default to 0.125 instead of being a scaling factor for a hardcoded 0.125?

I think scaling it this way works well as having the default appear to the user as '1.0' makes the factor easy to understand in your head. But if you want me to remove the 0.125 scaling factor and change the default to 0.125 instead of 1.0 then I will do that.

YuriSizov · 2022-07-12T12:36:52Z

We've discussed this in a PR meeting, and it would be nice to gather more feedback from users. The linked proposal is only about one thing out of everything exposed here, and it doesn't have any discussion.

We can post this PR on Twitter to spark a bit of discussion there. Maybe there are other sources that you can link too?

jasonwinterpixel · 2022-07-21T22:41:10Z

Can you summarise the core of the discussion? What specifically are you interested in feedback on?

Previously, 'damping' and 'sensitivity' were hard coded values chosen years ago. And the sensitivity on the pan gesture on web platforms was set extremely high and was untunable.

This seems like a pure positive to expose these values to the user. There are no fundamental changes here, just more control offered to the user.

YuriSizov · 2022-07-21T23:35:13Z

Can you summarise the core of the discussion? What specifically are you interested in feedback on?

Just the general. Do other users need this, and if they do, is this the solution that works for them.

Calinou · 2022-07-22T00:28:14Z

See also the discussion in #47931, which also aims to improve the touchscreen scrolling experience. I think #47931 is a more robust solution to improve smooth scrolling, but it'll break compatibility so it can only be considered for 4.0. However, it doesn't expose scroll sensitivity and follow focus buffer, so I think both PRs are relevant.

As for user need, I'd say issues like #21137 show that there's a lot of demand for improving how scrolling behaves on touchscreens in Godot. The current implementation is clearly subpar compared to system applications or even Chromium.

jasonwinterpixel · 2022-07-22T15:57:55Z

#21137 is 4 years old and hyperbolic. Scrolling works on touchscreens on Android. That user maybe doesnt have emulate mouse from touch enabled? I think this issue should be closed.

As for #47931 I'd agree that the inertial scrolling function could probably be different as it doesnt feel quite right as is, but it's important to note just how similar this PR is to this one from a user's perspective. Increasing the 'scroll damping' PR is basically 80% of what using the 'quadratic damping' function will achieve. I think this other PR may have more code changes than is needed, making it difficult to review. We could add 'quadratic damping' to scroll_container.cpp without such a large changeset. A smaller change set would lower the risk of regression and be easier to review. This large changeset is probably why this PR hasnt been merged in over a year. Maybe it's better code, maybe not?

This PR also separates 'pan gesture sensivity' from "scroll sensitivity" which allows tuning of laptop trackpads, for instance.

pixelriot · 2022-08-09T14:49:06Z

I'm currently working on a mobile app with a lot of scrollable lists and fully support this PR. The current default scroll speed for Scroll Containers is way too fast on mobile devices.

Please expose those properties to the user.

Thanks.

KoBeWi · 2022-12-22T12:10:36Z

Needs rebase.

Also what's the purpose of the new project settings? They don't seem used.

jasonwinterpixel requested a review from a team as a code owner February 19, 2022 14:00

YeldhamDev added enhancement topic:gui labels Feb 19, 2022

YeldhamDev added this to the 4.0 milestone Feb 19, 2022

jasonwinterpixel changed the title ~~Scroll Container exposes Damping and Sensitivity~~ Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer Feb 19, 2022

jasonwinterpixel mentioned this pull request Feb 19, 2022

[3.x] Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58319

Open

jasonwinterpixel force-pushed the scroll-damping branch 3 times, most recently from 2a74487 to 5ad54b4 Compare February 19, 2022 16:07

jasonwinterpixel requested a review from a team as a code owner February 19, 2022 16:07

jasonwinterpixel force-pushed the scroll-damping branch 2 times, most recently from f6de597 to 717bb9c Compare February 19, 2022 18:45

raulsntos reviewed Feb 20, 2022

View reviewed changes

scene/gui/scroll_container.cpp Show resolved Hide resolved

jasonwinterpixel force-pushed the scroll-damping branch from 717bb9c to 56240ef Compare February 21, 2022 12:02

bruvzg reviewed Feb 25, 2022

View reviewed changes

scene/gui/scroll_container.cpp Outdated Show resolved Hide resolved

jasonwinterpixel force-pushed the scroll-damping branch 2 times, most recently from 54d0279 to 66f3318 Compare March 20, 2022 13:51

jasonwinterpixel force-pushed the scroll-damping branch 3 times, most recently from 4a6ccb9 to 87a1a02 Compare March 20, 2022 19:48

Calinou mentioned this pull request Mar 22, 2022

Scrollcontainer which doesn't show the last element when supposed to spawn outside of Scrollbox #38730

Open

jasonwinterpixel force-pushed the scroll-damping branch 3 times, most recently from de4743b to 6886a11 Compare April 20, 2022 12:43

akien-mga reviewed Apr 26, 2022

View reviewed changes

akien-mga added the for pr meeting label Apr 26, 2022

akien-mga requested a review from YeldhamDev April 26, 2022 12:17

Calinou mentioned this pull request Apr 28, 2022

Erratic scrolling on Android when pressing on an element #60600

Closed

YuriSizov removed the for pr meeting label Jul 12, 2022

jasonwinterpixel force-pushed the scroll-damping branch from 1ce9797 to 73d2776 Compare July 21, 2022 23:13

Scroll container damping and sensitivity.

32d1a99

jasonwinterpixel force-pushed the scroll-damping branch from 73d2776 to 32d1a99 Compare July 21, 2022 23:23

YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58318

Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58318

jasonwinterpixel commented Feb 19, 2022 •

edited by Calinou

Loading

jasonwinterpixel commented Mar 20, 2022 •

edited

Loading

akien-mga Apr 26, 2022 •

edited

Loading

jasonwinterpixel Apr 27, 2022

akien-mga Apr 26, 2022

akien-mga Apr 26, 2022

akien-mga Apr 26, 2022

akien-mga Apr 26, 2022

akien-mga Apr 26, 2022

akien-mga Apr 26, 2022

jasonwinterpixel Jul 21, 2022

YuriSizov commented Jul 12, 2022

jasonwinterpixel commented Jul 21, 2022 •

edited

Loading

YuriSizov commented Jul 21, 2022

Calinou commented Jul 22, 2022 •

edited

Loading

jasonwinterpixel commented Jul 22, 2022 •

edited

Loading

pixelriot commented Aug 9, 2022

KoBeWi commented Dec 22, 2022

	When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll, so that the focused control is not barely visibile.
	When [member follow_focus] is [code]true[/code], specifies how far past the focused control to scroll, so that the focused control is not barely visible.

	void ScrollContainer::set_follow_focus_buffer(Vector2 buffer) {
	void ScrollContainer::set_follow_focus_buffer(Vector2 p_buffer) {

	void ScrollContainer::set_scroll_damping(float d) {
	void ScrollContainer::set_scroll_damping(float p_damping) {

		// original sensitivity was 0.125
		float scaled_sensitivity = sensitivity * 0.125;

Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58318

Are you sure you want to change the base?

Scroll Container exposes Damping, Sensitivity, Follow Focus Buffer #58318

Conversation

jasonwinterpixel commented Feb 19, 2022 • edited by Calinou Loading

jasonwinterpixel commented Mar 20, 2022 • edited Loading

akien-mga Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuriSizov commented Jul 12, 2022

jasonwinterpixel commented Jul 21, 2022 • edited Loading

YuriSizov commented Jul 21, 2022

Calinou commented Jul 22, 2022 • edited Loading

jasonwinterpixel commented Jul 22, 2022 • edited Loading

pixelriot commented Aug 9, 2022

KoBeWi commented Dec 22, 2022

jasonwinterpixel commented Feb 19, 2022 •

edited by Calinou

Loading

jasonwinterpixel commented Mar 20, 2022 •

edited

Loading

akien-mga Apr 26, 2022 •

edited

Loading

jasonwinterpixel commented Jul 21, 2022 •

edited

Loading

Calinou commented Jul 22, 2022 •

edited

Loading

jasonwinterpixel commented Jul 22, 2022 •

edited

Loading