godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Re-add the old behaviour of ScrollContainer regarding the minimum size being adjusted #7118

Open L4Vo5 opened 1 year ago

L4Vo5 commented 1 year ago

Describe the project you are working on

A level editor, which naturally uses UI.

Describe the problem or limitation you are having in your project

As seen in https://github.com/godotengine/godot-proposals/issues/2121, ScrollContainer's scrollbars used to not take any space when there was sufficient space to not display them. With the new code/enums, that old behaviour is impossible.

While the old way was generally considered worse for most use cases (where you use ScrollContainers for things that are explicitly meant to be scrollable), there is one case where I personally prefer it: when the child of the ScrollContainer is expected to, most of the time, have more than enough space to fit its contents.

For a specific example, I'm working on UI that looks like this: image It's intended to be used in a maximized window, so currently things fit really comfortably. However, I'd like to support smaller windows and screens (some laptops have a width of ~1366 pixels), and the TabContainer on the right can grow and steal space from the menu. The menu is implemented with an HBoxContainer, so to handle this, I put a ScrollContainer as its parent so it can still be navigated when there's not enough space. But the scroll container set to "Auto" reserves space for the ScrollBar, which looks out of place for a menu as the buttons have some random blank space beneath. image When the space is small, the ScrollBar showing up and taking that space makes more sense. But this isn't something you'd run into in the ideal conditions, so it's out of place most of the time. image The most I can do without custom scripts is setting the HBoxContainer's vertical container sizing to Expand. But all this does is adjust the HBoxContainer into the extended minimum size, making everything taller than it should be. image

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The proposal is to add an option to reintroduce the old behaviour. In my example, the menu would completely "hide" the fact that it was a ScrollContainer, unless it was small enough for it to be necessary.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Add a new enum, SCROLL_MODE_CONDITIONAL, to optionally restore the old behaviour. Document the differences between it and SCROLL_MODE_AUTO, since they might sound similar on a surface level. The result would look something like this: scroll good.webm

If this enhancement will not be used often, can it be worked around with a few lines of script?

I did manage to implement the behaviour I wanted with a Container node that's itself parent of the ScrollContainer. In the one place where I use it, the parent is a MarginContainer. I'm not sure if it depends on that to work properly.

extends Container
@onready var child: ScrollContainer = get_child(0)

func _ready() -> void:
    _sort_children()

func _notification(what: int) -> void:
    if what == NOTIFICATION_SORT_CHILDREN:
        _sort_children()

func _get_minimum_size() -> Vector2:
    if not is_node_ready(): return Vector2.ZERO
    return Vector2(0, child.get_minimum_size().y)

func _sort_children() -> void:
    if child.size == size: return
    if child.get_minimum_size().x >= size.x:
        child.horizontal_scroll_mode = ScrollContainer.SCROLL_MODE_SHOW_ALWAYS
    else:
        child.horizontal_scroll_mode = ScrollContainer.SCROLL_MODE_DISABLED
    child.size = size

(same video as above) scroll good.webm

A worse option is to set both scroll modes to auto and manually adjust the ScrollContainer's vertical minimum size. But then it won't grow when the scrollbar is shown, which looks worse as it overlaps the buttons. Still, if there was a way to fix that, this would likely end up being the simpler workaround

extends ScrollContainer

func _ready() -> void:
    resized.connect(_on_resized)
func _on_resized() -> void:
    custom_minimum_size.y = get_child(0).get_minimum_size().y

scroll bad.webm

Is there a reason why this should be core and not an add-on in the asset library?

I've found no simple way to implement the desired behaviour by extending ScrollContainer. So the solutions are to either reimplement the whole node entirely just for this one thing, or have an upper Container node handle it, like I showed above. Neither is ideal so IMO it's worth adding to the engine.

KoBeWi commented 1 year ago

Unless I'm missing something, the ScrollContainer already behaves like you request?

https://user-images.githubusercontent.com/2223172/246976263-aae9452f-e299-465a-941c-a949eea1dcc2.mp4

This is 4.1 beta2. If you see a different behavior, it's likely a bug.

L4Vo5 commented 1 year ago

Unless I'm missing something, the ScrollContainer already behaves like you request?

godot_459xJKFfdX.mp4 This is 4.1 beta2. If you see a different behavior, it's likely a bug.

I think this is because horizontal scroll is set to auto. When it's disabled, the vertical scrollbar always takes up space.

KoBeWi commented 1 year ago

I tested with horizontal scrollbar disabled. Can you make a small scene that displays a different behavior?

L4Vo5 commented 1 year ago

This is what I get on this scene, in v4.1.beta2.official [a2575cba4] scroll behaviour.webm scroll_test.zip Looking at it again, I think your video has more to do with the Label's autowrap than anything.

L4Vo5 commented 1 year ago

Here's a small mockup of the menu I was having trouble with scroll_test_2.zip My claim is that, without a script, you can't make it so that the ColorRect "touches" the buttons if there's enough space, but the scroll bar gets in between them (shrinking the ColorRect) when the horizontal space runs out.

KoBeWi commented 1 year ago

Right, seems like autowrap thing. get_minimum_size() uses the scrollbar size, but when sorting children this size is ignored, creating an inconsistency. The content may even overlap the scrollbar. image But well, it's unrelated to this proposal.