Consolidate H/V UI nodes into generalized variants

[spindlebink]

Describe the project you are working on

A puzzle game for Game Off 2021 with a typical middlingly-complex UI.

Describe the problem or limitation you are having in your project

Godot's HBoxContainer and VBoxContainer nodes provide the same functionality except for their direction. The same goes for HSplitContainer, VSplitContainer, HSlider, VSlider, HSeparator, VSeparator, HScrollBar, and VScrollBar. All of these classes are subclassed as well from a general, non-directional parent.

But subclassing here rather than simply increasing the functionality of the base class seems unnecessary. There are separate node classes for what's really just a configuration value. This leads to not only doc and API redundancy but also several minor issues which, while not insurmountable on their own, are consistently present and pop up every time I do layout:

1. Changing the direction of the container--again, something that should be purely configuration--means invalidating a script extending from it and hence requires the small but annoying additional step of changing extends HBoxContainer or a variant as many times as the direction changes.
2. This may just be me, per this comment, but there's potential friction between naming, visual appearance, and node purpose.
3. Changing the direction of a box container requires switching the node type (as mentioned), which means all separators in the container must also be individually changed to their H or V variants.

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

The respective functionality that makes them horizontal or vertical (or theoretical further directions, like Flutter's depth-based stacks) could be accessed by a simple enum property instead, slimming down redundancy in the class list, theming, and documentation.

I propose that the H- and V-specified variants of the UI controls be subsumed into their parent classes and switching between the layout methods be done through enum properties. In keeping with the move toward simplicity, clarity, and showing only necessary properties in the UI system, this would turn subclasses' layout direction into a single parameter which could be set on a SplitContainer or BoxContainer (which, in the process, could likely be more clearly named StackContainer). Similarly, the H- and V-separator classes would be simplified into a single Separator or StackSeparator which would adapt as necessary to the direction of its parent StackContainer.

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

HSplitContainer and VSplitContainer would become SplitContainer. HBoxContainer and VBoxContainer would become StackContainer (although BoxContainer works as well--I think GTK uses "box," but most other frameworks I know of use "stack"). HSlider and VSlider would become Slider. HSeparator and VSeparator would become Separator, which would fit the direction of their parent StackContainer, and HScrollBar and VScrollBar would become ScrollBar.

Each of these classes would then expose a direction property (or a similar name), one of an enum of layout directions which could then be theoretically expanded in the future should the need arise. This way, we don't lose the flexibility that subclassing currently enables (there'd be less flexibility with a boolean horizontal flag) while also not using subclassing further than necessary, a la Swing.

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

As mentioned, it's not a critical issue, but since 4.0 will be making many breaking changes in the interest of more intentionally-designed, clear, and accessible APIs, it seems like a good time for the switch.

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

It pertains to melting down classes which are part of the core UI system.

[Calinou]

Merging the horizontal and vertical variants together would make it possible to implement godotengine/godot#36040 without hacks, but reduz was opposed to it last time I asked.

[spindlebink]

Did he give a rationale? I wonder if the switch might be more reasonable in this form, as part of a wider improvement/anti-redundancy/composition-over-inheritance measure rather than a specialized use case.

[akien-mga]

I like the idea, but it's worth noting that it will add an extra step every time one needs to use whichever orientation is not the default.

For example in the editor code, we have:

$ rg "memnew\(VBoxContainer\)" | wc -l
296
$ rg "memnew\(HBoxContainer\)" | wc -l
314

So whichever is default, the other one will require an extra method call to change the orientation:

BoxContainer *hbc = memnew(BoxContainer);
hbc->set_orientation(Orientation::HORIZONTAL);

instead of:

HBoxContainer *hbc = memnew(HBoxContainer);

(Not saying that it makes this proposal problematic, but it's worth being aware of for UI heavy workflows like editor code.)

[YuriSizov]

For example in the editor code, we have:

This is a perfect illustration that in a given project of a considerable size that has a lot of UI either node is heavily used. As such, unification would make it a worse experience to build such a project. And it also highlights a wrong statement in the OP:

All of these classes are subclassed as well from a general, non-directional parent.

This is not true, because most if not all of them rely on a boolean flag, so one direction is assumed as default. E.g. BoxContainer is horizontal by default. So all of them would have to assume one direction to be the default, which is a very big assumption.

[akien-mga]

Some additional context for engine and editor code:

$ rg "memnew\(HSplitContainer\)" | wc -l
35
$ rg "memnew\(VSplitContainer\)" | wc -l
14
$ rg "memnew\(HSlider\)" | wc -l
11
$ rg "memnew\(VSlider\)" | wc -l
2
$ rg "memnew\(HSeparator\)" | wc -l
26
$ rg "memnew\(VSeparator\)" | wc -l
92
$ rg "memnew\(HScrollBar\)" | wc -l
12
$ rg "memnew\(VScrollBar\)" | wc -l
11

So it's mostly BoxContainer which would be negatively affected, the rest is mostly anecdotal (aside from HSeparator/VSeparator whose misnaming is the original motivation for this proposal).

[YuriSizov]

So it's mostly BoxContainer which would be negatively affected, the rest is mostly anecdotal

Well, yes, the other nodes are simply more contextual and less used 🙃 So I think it's fair to judge this based on the box.

[spindlebink]

in a given project of a considerable size that has a lot of UI either node is heavily used

That's a good point. That said, I don't think it's C++ instantiation of these nodes (and hence an extra set_orientation call) that's heavily used--it's just a box container with a configurable direction, however it might be instantiated. When Godot users rather than engine developers implement a UI, they'll be doing it through the editor. I'd argue the priority ought to lie in in-editor and runtime usage rather than in implementation of the editor itself. Also, I'm not an engine developer, so I wouldn't be the one using it C++ side, but I will also admit that I don't see the addition of a call to set_orientation as that big of a deal--it could likely be done across the engine with a find+replace followed by a testing pass to make sure nothing's been screwed up.

Is there a reason to stay away from specifying direction in a constructor? I know very little about the codebase's internals, so I don't know if the (custom macro?) memnew method allows for arguments. If the class could provide an extra constructor that configures the direction, you'd be able to use it conveniently from C++ while also not worrying about it from the editor.

[Calinou]

Is there a reason to stay away from specifying direction in a constructor? I know very little about the codebase's internals, so I don't know if the (custom macro?) memnew method allows for arguments.

It does, as sometimes used with Label (memnew(Label("Example"))). Only a handful of nodes use this ability, and it's not exposed to GDScript.

In fact, this should probably be extended to Button/CheckBox/CheckButton nodes as well (and possibly more) 🙂

[YuriSizov]

That said, I don't think it's C++ instantiation of these nodes (and hence an extra set_orientation call) that's heavily used--it's just a box container with a configurable direction, however it might be instantiated. When Godot users rather than engine developers implement a UI, they'll be doing it through the editor. I'd argue the priority ought to lie in in-editor and runtime usage rather than in implementation of the editor itself. Also, I'm not an engine developer, so I wouldn't be the one using it C++ side, but I will also admit that I don't see the addition of a call to set_orientation as that big of a deal--it could likely be done across the engine with a find+replace followed by a testing pass to make sure nothing's been screwed up.

My point doesn't actually change whether we are talking about the editor itself, or creating nodes in GDScript, or creating them in the editor UI. In fact, I'd say that the editor users would have it the worst if we make this change.

We would still have to assume one direction is more prevalent than the other (which is provably false), and as such we would make users do extra configurations if they need non-default orientation. While we could use the constructor to make this problem less severe in code, there is no such thing for creating UI in the editor. So this will be a downgrade for the majority of users, because the majority of users create scenes using the editor UI.

[spindlebink]

Oh, I see your point now, that makes sense.

It seems to me to be a question of which direction to weight convenience. Consolidation makes creating a ton of non-default boxes more tedious but benefits management and modification of existing structures, where keeping them as is makes it easier to instantiate but more tedious to modify and code around afterward.

I personally prefer editing rather than instantiation convenience, but that's probably something to get a wider sample size for.

[Geometror]

We have to keep in mind that by consolidating the H/V UI nodes we would lose the ability to theme each variant independently (at least in a conveniant way).
For example, if you want different separation values for each orientation, we will have to add two theme properties: hseparation and vseparation, possibly cluttering the theme editor. (or you use theme overrides for every control)

I made a small experiment/mockup to demonstrate a different solution which can be seen here: https://github.com/Geometror/godot/tree/h-v-container-solution. (just H/VSplitContainer for now, although this could be applied to other controls)

My approach to allow changing the orientation dynamically (e.g. needed for godotengine/godot#36040) is the following:

• Register the base container class (SplitContainer) as a normal class (non-virtual) to allow using it as a "general" SplitContainer.
• Add an orientation property which is only visible in the inspector when the general SplitContainer class is used.
• When you try to set the orientation on a H/VSplitContainer (with "locked" orientation) a warning is printed and nothing happens

Visual demos:

This allowes us to have dynamically oriented UI controls in some specific cases (like godotengine/godot#36040), while keeping the traditional way of defining orientation.
Please let me know whether this is a good or rather terrible idea :)
(Since this is mockup which was quickly thrown together the code could need some improvement)

[YuriSizov]

We have to keep in mind that by consolidating the H/V UI nodes we would lose the ability to theme each variant independently (at least in a conveniant way).
For example, if you want different separation values for each orientation, we will have to add two theme properties: hseparation and vseparation, possibly cluttering the theme editor. (or you use theme overrides for every control)

Not necessarily, we can use theme type variations. We can have some built into the default theme. We'd have to hack a bit to make it so when the custom type variation is empty (i.e. not set by user) we would smartly take one of the other of the default ones.

---

This allowes us to have dynamically oriented UI controls in some specific cases (like #36040), while keeping the traditional way of defining orientation.
Please let me know whether this is a good or rather terrible idea :)

I think that for godotengine/godot#36040 specifically we don't even need to expose anything to scripting. A method to change orientation can be public, but engine internal.

But if there is user need for such flippable containers as well, then I think your proposal is better, as it adds an option without breaking the good thing that we already have. And the codebase already has those base classes, so it's not an increase in maintenance.

[marstaik]

I have to disagree with some of the supportive points on distinct nodes. While there is a slight upfront cost to change alignment direction, because it is not the default, it is also ingrained in most human languages to read beginning from a certain direction, and as the project and code is written in English primarily, assuming a left-to-right orientation by default is fine.

As for the points of users expecting to needing to put in an extra click to change orientation, the opposite side is that during the creative portion of the UI process things aren't set in stone and you typically want to experiment with variations. In cases of left versus right, that would be creating a brand new box of the opposite orientation, having to copy over all of the parameters you have set, and re-parenting the scene nodes, which is a lot more work than using an enum dropdown or a checkbox to set the direction.

Additionally, people coming from web developer backgrounds are also familiar with default left-to-right orientation, as well as specifying right alignment manually when needed via e.g. float: right; or flexbox alignment rules.

Also, to make UI controls a bit more flexible from a code point of view, the property setters could be improved to be reference-returning. ie: auto box = HBox().align(right).padding(10); as an example. This removes needed unnecessary extra constructors for every property.

[YuriSizov]

@marstaik This has nothing to do with LTR vs RTL. This is about horizontal and vertical direction of some control nodes.

[marstaik]

@marstaik This has nothing to do with LTR vs RTL. This is about horizontal and vertical direction of some control nodes.

That was a very small part of my reply, the rest of it does have to do with the direction of control nodes.

[nathanfranke]

In the meantime this can be "worked around" by using a GridContainer and changing the columns to either 1 (horizontal) or get_child_count() (vertical, through code)

[me2beats]

I think this is a good idea
There were some cases where I need to switch vertical and horizontal orientation of a BoxContainer in a running game.

I saw this approach in some UI frameworks I worked with, for example Kivy has orientation property.

I think the can be named direction (as proposed) or orientation.

[kitbdev]

To keep the UI workflow the same in the editor we could have a system to show presets in the Add Node menu after consolidating the nodes.
related: #6057 but able to replace the original class seamlessly in the menu.

Implementation ideas:
Presets should only affect the default values, they don't add functionality on their own.
Presets should be automatically added to documentation.
Icons and theming should be customizable as well.
This could even be expanded and exposed to users to make their own presets for custom nodes or resources.

// idea for how this could work
// split_container.cpp
void SplitContainer::_preset_vbox() { }
void SplitContainer::_preset_hbox() {
    vertical = false;
}
void SplitContainer::_bind_methods() {
    ClassDB::Add_Preset("SplitContainer (Vertical)", "VSplitContainer", _preset_vbox, default = true);
    ClassDB::Add_Preset("SplitContainer (Horizontal)", "HSplitContainer", _preset_hbox, theme);
    ...
}

[Zireael07]

This issue is way more complex, it's not about showing 'presets'