scrollcontainer: Add automatic scrollbar calculation

[SmallJoker]

When specifying 'scroll factor'='auto', it will be calculated automatically based
on the associated scrollbar maximal value. Previously, calculating the exact value was mostly guesswork and try & error.

EDIT: This could also be implemented in reverse by adjusting the maximum value of the scrollbar, which would however be more annoying to read out from mods.

Part fix for #12482

To do

This PR is Ready for Review.

How to test

1. /test_formspec in devtest
2. "ScrollC" tab. Scroll down and compare the padding size with the button scale.

[Desour]

Doesn't fix #12482 entirely, because that issue wants to set other scrollbaroptions as well, like thumb size.

[rubenwardy]

Aw that's what I thought this did...

[grorp]

The scroll factor defines the scrolling "resolution". The default smallstep (10) and largestep (100) value and the default scroll factor value (0.1) are tuned to work well together. Blindly changing the scroll factor without adjusting smallstep and largestep doesn't make sense. This makes scrolling very slow and uncomfortable in your /c 1 and /c 0 examples.

In my opinion, having the scroll factor depend on the content size doesn't make sense. The proper way to use a scroll_container[] is to have a constant scroll factor and adjust the max and thumbsize values in scrollbaroptions[] depending on the content size. This is also how the make_scrollbaroptions_for_scroll_container implementation used by the settings menu works. So 👎 from me for this PR's current approach.

P.S.: Since the scroll factor defines the scrolling resolution, it's also relevant for animations. For my experimental "inertial scrolling on Android" branch, I had to decrease the scroll factor (increase the scrolling resolution) used by the settings menu to make a smooth animation possible: scroll factor diff.txt

[SmallJoker]

@grorp

The apparent inner container size is currently defined by factor * scrollbar_max. As you also mentioned, this does not make any sense. Instead, the bounding box of the children could be used (what this PR aims to do).

The proper way to use a scroll_container[] is to have a constant scroll factor and adjust the max and thumbsize values in scrollbaroptions[] depending on the content size.

These values should not need to be calculated manually. It is tedious work, prone to break in future formspec updates, and might result in cargocult/boilerplate code. Automatic content size with constant max values is what this PR targets. I can try to make the scroll speed independent, if that's acceptable for you.

[grorp]

The apparent inner container size is currently defined by factor * scrollbar_max. As you also mentioned, this does not make any sense.

I didn't say that, there must be a misunderstanding. The way scroll containers work makes sense, it's just complicated to get right manually.

Instead, the bounding box of the children could be used (what this PR aims to do).

In general, this is a good idea. I just disagree with a design choice made in this PR.

The proper way to use a scroll_container[] is to have a constant scroll factor and adjust the max and thumbsize values in scrollbaroptions[] depending on the content size.

These values should not need to be calculated manually. It is tedious work, prone to break in future formspec updates, and might result in cargocult/boilerplate code.

I agree that manually calculating these values is unnecessarily complicated. However, "the proper way" I described (constant scroll factor, max value based on content size) is also the way this should be automated IMO.

Automatic content size with constant max values is what this PR targets.

"Constant max value, scroll factor based on content size" is exactly what I'm opposed to.
Instead, I think the proper way is "constant scroll factor, max value based on content size".

I can try to make the scroll speed independent, if that's acceptable for you.

The scrollbar stores its position as an int. min and max together define a finite amount of possible scroll positions. They define the resolution of the scrollbar, so to speak.

With constant min and max values and arbitrary content sizes, you cannot possibly make the scrolling speed independent. It's possible as long as the content size is not too high, but with high content sizes, ...

• ... the resolution of the scrollbar isn't sufficient for small scrolling steps and you have to do bigger steps (-> higher scrolling speed).

• ... animations like smooth scrolling and inertial scrolling don't work properly due to insufficient scrollbar resolution. Long before they obviously stop working properly, they start to become less smooth.

With the default max value of 1000, you need a lot of content to make this happen, but it happens at some point.

That's why this PR's approach is wrong IMO. "Constant scroll factor, max value based on content size" would be a saner approach and has already been proven in the settings menu.

[grorp]

Here's an implementation of what I imagine, on top of your PR: grorp@737acd1 (or to see the changes compared to master: grorp/luanti@737acd1~3...737acd1)

I also made the settings menu use the new option, works well. This didn't work well with the implementation in this PR.

There's still a conceptual problem my implementation inherits from this PR though: Using the bounding box of the children means you cannot have padding at the end of the scroll_container, which the settings menu needs.

Possible solution: Allow manually specifying a content length as the last scroll_container parameter, instead of the auto parameter. Specifying a content length enables auto mode.

[SmallJoker]

I agree that manually calculating these values is unnecessarily complicated. However, "the proper way" I described (constant scroll factor, max value based on content size) is also the way this should be automated IMO.

As long you don't care about the relative scroll progress - i.e. core.explode_scrollbar_event(fields.myscrollbar) / MAXVALUE - this will work. Possible solution for that case: additional parameter for the field value -> "CHG:<value>:<max>".

With the default max value of 1000, you need a lot of content to make this happen, but it happens at some point.

When you reach the point where 1 increment scrolls more than - let's say - 1/4th the container size, then it's very likely that a new concept is needed for such formspec. We would also be far beyond of what can be reasonably searched by the scrollbar thumb.

Possible solution: Allow manually specifying a content length as the last scroll_container parameter, instead of the auto parameter. Specifying a content length enables auto mode.

I assume that your "enables auto mode" means automatic max calculation. Fine my me. I think that's an acceptable solution.

[grorp]

When you reach the point where 1 increment scrolls more than - let's say - 1/4th the container size, then it's very likely that a new concept is needed for such formspec. We would also be far beyond of what can be reasonably searched by the scrollbar thumb.

That's true, but problems already start earlier. Here's the problem case I had in mind when writing #14623 (comment):

I was working on an experimental "swipe to scroll + inertial/kinetic scrolling on Android" branch. Scrolling in the settings menu (scroll_container) wasn't smooth and felt jerky, while scrolling e.g. in the serverlist (textlist) was smooth.

I found that on my Android device (high display density) in the settings menu, the default scroll factor of 0.1 results in 1 scrollbar position unit = 10.5 pixels, so the scrollbar's resolution was too low for a smooth animation. For a smooth animation, I needed 1 scrollbar position unit <= 1 pixel.

To satisfy this, I had to decrease the scroll factor to 0.0095238095 or lower (I used 0.001) and of course adjust smallstep and largestep accordingly. On the Settings -> Graphics and Audio -> Graphics page, the max value required for that was 4958 (for 0.001: 46647), already more than the default value of 1000.

All this makes me think that we might want to, in addition to what we've already been talking about, ...

1. ... decrease the default scroll factor to 0.001 and increase the smallstep and largestep defaults accordingly in formspec version 8 OR

2. ... make the scrollbar position a float so that we don't have resolution problems OR

3. ... offer a "scroll factor = auto" option that's enabled by default starting with formspec version 8, with a different meaning than the "scroll factor = auto" option in the current version of this PR:

   It would result in 1 scrollbar position unit = 1 pixel and smallstep and largestep would be adjusted automatically. This is what I mean: grorp@825c4b1. (Since the scroll factor is specified in formspec coordinates, it's basically impossible for a mod to get 1 scrollbar position unit = 1 pixel at the moment.)

Obviously this is not necessary for this PR.

I assume that your "enables auto mode" means automatic max calculation. Fine my me. I think that's an acceptable solution.

🎉

[SmallJoker]

@grorp Updated as of c48341a. thumbsize and max are now based on the same calculations.

The absolute scroll container size is also impractical to use (needs a placeholder in the fs table (builtin), and late insertions). Thus, I believe that specifying a content padding is more convenient.

[v-rob]

Tested; works. Very useful feature. Just a few minor comments.

[v-rob]

LGTM

[Desour]

• 🎉
• How to test instructions in top comment look outdated.
• Please add something in /test_formspec.
• Afaik, scollbar min,max can also be negative. Should we maybe support content that is above 0?
  (<content padding> could be <content padding neg>,<content padding pos>, where empty (or ,) means no auto, <num>, means set max to 0, ,<num> means set min to 0, and <num>,<num> means both min and max depend on content.)

[Desour]

Afaik, scollbar min,max can also be negative. Should we maybe support content that is above 0?

@SmallJoker Thoughts on this? Yes/No?

[SmallJoker]

@Desour

Should we maybe support content that is above 0?

Although slightly hacky, modders can achieve the same thing by setting the scrollbar to a very high value, which will scroll to the bottom. Negative values do make sense for other purposes, such as sliders where negative values are wanted.

[Desour]

Looks good. Thanks!
Sorry for late re-review.

Should we maybe support content that is above 0?

Although slightly hacky, modders can achieve the same thing by setting the scrollbar to a very high value, which will scroll to the bottom.

Right.
It's probably too niche of a feature to support anyways, and adding it later is still possible.

(Not adding two approvals label, because there were changes @v-rob's approval.)

[v-rob]

Doesn't require my approval since the PR is by a core dev, but the changes appear to be fine to me.

[grorp]

Settings "Graphics" page, before/after comparison

Settings "Shaders" page, before/after comparison

On the "Shaders" page, the thumb size is clearly different. Is this expected?

[Desour]

On the "Shaders" page, the thumb size is clearly different. Is this expected?

There's also a bit less padding at the bottom.

[SmallJoker]

@grorp I did the calculation.

With PR, settings tab "Minetest Game", visible area

hidden area

600/398×162 = 244 px. Not pixel-perfect but from what I can see it's mathematically correct proportions.