Allow SplitContainer to have more than two children

[kitbdev]

• related Make Editor dock resizing consistent #83359

SplitContainers can now work with more than two children. Set the main editor HSplit and the script editor debugger to use this, so they are resized consistently.
This should not break compatibility.

Split offsets are just offsets from their default positions (aka wished_middle_offset).
The default positions are calculated similar to how BoxContainers sort their children. However, if the dragger is before or after all expand flags, then it will be at the start or end of the SplitContainer to keep the same behavior as before.

The number of split offsets increase whenever there is a new valid child, but it won't decrease since that can lose data and won't undo.

• Edit: Separate PR for the editor: Use multiple children in dock SplitContainers to make resizing consistent #90439

• Bugsquad edit, closes: Allow SplitContainer to have more than 2 children godot-proposals#9666

[groud]

The number of split offsets increase whenever there is a new valid child, but it won't decrease since that can lose data and won't undo.

For that, you could use an _undo_redo_inspector_callback, but well, it does not really matter, an array is fine too.

Split offsets are just offsets from their default positions (aka wished_middle_offset).
The default positions are calculated similar to how BoxContainers sort their children. However, if the dragger is before or after all expand flags, then it will be at the start or end of the SplitContainer to keep the same behavior as before.

Hmm, maybe I misunderstand how things are implemented, but I believe we should avoid that. I don't remember exactly why, but this was how it used to be implemented before and it created problems when resizing the container, or when children components change their minimum size.

Edit: here is one of the issues that used to happen: #43749

[kitbdev]

For that, you could use an _undo_redo_inspector_callback

I can't use it in split_container.cpp, right? Undoing adding a child should undo adding the split_offset, but it happens in the sort children notification, so I don't know how to add it to undo redo.

Hmm, maybe I misunderstand how things are implemented, but I believe we should avoid that. I don't remember exactly why, but this was how it used to be implemented before and it created problems when resizing the container, or when children components change their minimum size.

If I didn't account for the minimum size, then the children with expand flags would not be the same proportions with their stretch ratio.
For example (B and D set to expand and a stretch ratio of 1, all split offsets at 0, A and C have minimum sizes):

If the minimum size of A increases, then B and D wouldn't be the same size any more. So C should be moved to keep them the at same ratio.

I don't think this is a problem since it only affects draggers that are in between two expanding children. So its default position will only be moved to keep them at the same ratio, and its split offset will be the offset from that position.

[groud]

I can't use it in split_container.cpp, right? Undoing adding a child should undo adding the split_offset, but it happens in the sort children notification, so I don't know how to add it to undo redo.

Ah right. This has been a problem forever... A ton of properties (size flags, stretch ratio, etc...) are in the Control node, in case they end up used as a child of a container. But often they end up unused.
Here, the "right" solution would be to add properties to Control, but that still sucks... I guess your current implementation is the most acceptable for now then :/

Removed the SplitContainerDragger because it is easier to just manage an vector of rects than creating a new one each time a child is added and removing when a child is deleted.

This does not sound right, it was added by #65355 to allow the dragger to have a bigger area than the underlying SplitContainer node.

TBH, I feel this is a lot of changes. I can't really wrap my head around all changes done there. So I believe this should be split into a core PR (SplitContainer) and an editor PR (editor changes). SplitContainer has been the source of tons of small issues in the past, so we need to test the new behavior thoroughly.

[kitbdev]

Changed back to use SplitContainerDragger

• Split editor part into separate PR Use multiple children in dock SplitContainers to make resizing consistent #90439

For reference:
wished_middle_sep is now default_dragger_positions and is now cached, middle_sep is dragger_positions.
_compute_middle_sep() is split into _update_default_dragger_positions() and _update_dragger_positions(). The default dragger positions only need to be updated in resort.
_update_default_dragger_positions() logic is referenced from BoxContainer::_resort(), including _StretchData (_MinSizeCache).
The split_offset property still exists and can be used like normal, but is hidden in the inspector to avoid duplicate data.

[Calinou]

Would this be useful to implement godotengine/godot-proposals#9676?

[kitbdev]

Would this be useful to implement godotengine/godot-proposals#9676?

Yes, this (along with #90439) will make it easier to implement.
move_child could be used instead of removing and adding children to specific nested SplitContainers.

[groud]

sc->get_containable_child(1)

This seems like it is still hardcoded for two children.

[kitbdev]

This actually checks for at least two children. When there is 0 or 1 children, get_containable_child(1) will be false and when there is 2 or more it will be true.
I changed this in a few places, but get_containable_child(1) implies get_containable_child(0).

[groud]

Ah ok that makes sense. In such case, I think it would be good to write a comment. Or maybe be explicit about it by defining a variable instead:

bool has_at_least_two_containable_child = sc->get_containable_child(1);
if (sc->collapsed || has_at_least_two_containable_child || sc->dragger_visibility != SplitContainer::DRAGGER_VISIBLE) {
 ...

Something like that I'd say.

[kitbdev]

I found a crash when duplicating SplitContainers with multiple children, I think it's related to how I dynamically add SplitContainerDragger as internal children. I add and remove them in the sort children notification.
Is there anything I can do so draggers aren't duplicated?
In Node::_duplicate() it fails to duplicate a dragger and it returns null.

Edit: I fixed it with force_parent_owned()

[kitbdev]

Rebased and fixed conflicts.

When duplicating a SplitContainer with 3+ children I'm getting:
scene\main\node.cpp:1682 - Index p_index = 4 is out of bounds ((int)data.children_cache.size() = 4).
Child node disappeared while duplicating.
so I may need to find a different way to handle the draggers that get added and removed dynamically.

[KoBeWi]

Do draggers need to be nodes? 🤔

[kitbdev]

Yes, since the grab area needs to be able to be bigger than the Split Container.
See #90411 (comment)
I changed them originally, but I had to change it back to Nodes.

Maybe there could be just one dragger handler node? It would complicate the dragging logic but it might work.

[Mickeon]

What's the consensus on this? It's marked for 4.3 and even though it's a nice feature, we are in feature freeze right now.

[anonymalek]

What's the consensus on this? It's marked for 4.3 and even though it's a nice feature, we are in feature freeze right now.

In my opinion this shouldn't count as a feature and should be included with 4.3 since SplitContainers should've always been like this. For me it counts as a "fix" since it also fixes sizing inconsistencies within the editor #90439

[kitbdev]

Unfortunately there are some issues with this and I'm not sure how to fix it. (#90411 (comment))
It has to do with the dynamically added draggers getting added/removed while the SplitContainer is being duplicated.
There can't just be one since it would take input from the controls in between. I also don't want to change the order that child nodes are duplicated in, and they need to be at the end so they are on top, so they are duplicated first and causing issues.

This has the potential to break things a lot with the Editor so I don't expect #90439 to be merged in 4.3 even if this was fixed now.
I'm also spending more time on bugfixes, so this probably won't be ready for 4.3 in time.

[KoBeWi]

This is set in every loop iteration, no? So while looks redundant.

[kitbdev]

Looks like I messed up the logic, it should work like it does in BoxContainer::_resort.

[KoBeWi]

I did some (not super-extensive) testing and it works correctly. The code looks fine overall, but it's not like I'm very familiar with SplitContainer. Would be nice if @groud can take a look too.

I spotted one minor issue that adding a child will add new entry to split_offsets, but deleting it won't delete the entry. It doesn't really have adverse effect though.

[kitbdev]

Fixed the stretching algorithm, it should now match BoxContainer in the case that the not-first-stretching child has a minimum size greater than its desired size.

I spotted one minor issue that adding a child will add new entry to split_offsets, but deleting it won't delete the entry. It doesn't really have adverse effect though.

Yeah, this is because removing a child and undoing it won't restore the split offset, so I don't resize split_offsets to prevent loss of data. I think the undo system needs more functionality for this, so it can be fixed in the future.

[WhalesState]

Note:
This will break compatibility with existing projects when loading them, because set_split_offset() and get_split_offset() have been removed, they should be deprecated instead, setting and getting the first element of the split_offsets array if it's size is greater than 0, or returning 0.0 instead.

[AThousandShips]

They have been deprecated, please check the code

[WhalesState]

Sorry i just noticed the variable was removed, and thought they have missed this, Amazing work and thanks for making this ^^.

[WhalesState]

I was testing it since i have had a hard time making a similar Container, and i have found some issues that we can fix before merging.

• Hiding a child makes the other children uses a wrong offset index, so hidden sortable children should be respected when getting the dragger's offset index.

• We should not deal with all internal children as draggers, adding an internal control node will not be sorted. We better ignore non Control nodes, top level controls and SplitContainerDragger nodes only.

• Size flags should only be taken into account when calculating offsets for the first time or when the node is inside editor, any other sorting after that should be calculated using drag_offsets if the offsets size is greater than or equal the sortable children size - 1.

Test2.mp4

have a look on this godotengine/godot-proposals#10610 and check the splitter_container.cpp for some ideas (I have dealt with offsets as a ratio array instead of position array to easily resize the SplitContainer without rewriting the offsets).

In addition, Label and MarginContainer are not used inside the cpp file, and should be removed.

#include "scene/gui/label.h"
#include "scene/gui/margin_container.h"

[kitbdev]

Rebased and removed unused includes.

Hiding a child makes the other children uses a wrong offset index, so hidden sortable children should be respected when getting the dragger's offset index.

Other Containers ignore hidden children intentionally, so only having enough split offsets for the valid visible children is expected.
This would also cause compatibility issues, since if a user has hidden controls at the start of their SplitContainer it would affect the split offset.

We should not deal with all internal children as draggers, adding an internal control node will not be sorted. We better ignore non Control nodes, top level controls and SplitContainerDragger nodes only.

Ignoring internal children was the behavior beforehand, but other Containers work like that so it makes sense to not ignore them all. Since it is not directly related and may need more review this can be done in another PR.

Size flags should only be taken into account when calculating offsets for the first time or when the node is inside editor, any other sorting after that should be calculated using drag_offsets if the offsets size is greater than or equal the sortable children size - 1.

This would mean changing size flags after starting does nothing, which doesn't seem very useful. Also what if the user adds a child and then sets a size flag for it after? It won't be recognized. Other containers like BoxContainer also don't do it like this.

Having it saved as a ratio seems nice, but it would break compatibility.

• There is a related proposal: SplitContainer - add percentage option for better scaling and UI responsiveness. godot-proposals#4033

[WhalesState]

Let's say we use offsets as a ratio which seems the best solution for this, it won't break compatibility since we can control the old behaviour in the deprecated method by converting old offset offset / (vertical ? size.y : size.x). this will make it easier to deal with offsets and will make it possible to control the offsets in the editor.

Size flags are made for static containers because we can't control their size manually, SplitContainer Should be an exception, since it's the only container that allows you to manually control it's children size.

Containers are numbers and characters are the dragger.

// [ 1 |a| 2 |b| 3 |c| 4 |d| 5 ]
// [ 2       |b| 3 |c| 4       ]

// Hiding 1 and 5 should make 2 use b offset and 4 expands till the end. dragger_offsets should not be changed at all when hiding/showing children.

// [ 1 |a| 2 |b| 3 |c| 4 |d| 5 ]
// [ 2       |b| 4             ]

// Hiding 3 should make 2 use b offset and 4 expands to the end.

// [ 1 |a| 2 |b| 3 |c| 4 |d| 5 ]
// [ 2       |b| 4       |d| 5 ]

// Showing 5 should make 4 use the d offset and 5 expands to the end,

// To achieve this, we should get the sortable children first and we save them and then we loop them to get the visible children.

// split_container.h
Vector<Control *> children;

// split_container.cpp
void SplitterContainer::sort_children() {
	children.clear();
	Vector<Control *> visible_children;

	for (int i = 0; i < get_child_count(); i++) {
		Control *c = Object::cast_to<Control>(get_child(i));
		if (!c || get_child(i)->is_class("SplitContainerDragger")) {
			continue;
		}
		if (!c->is_top_level_control()) {
			// get all sortable children even if they are hidden.
			children.append(c);
			if (c->is_visible_in_tree()) {
			        // get all visible children that will be sorted.
				visible_children.append(c);
			}
		}
	}
}

// To get the correct offset index, let `c` be the visible child.
int index = children.find(c);

Ignoring internal children was the behavior beforehand, but other Containers work like that so it makes sense to not ignore them all. Since it is not directly related and may need more review this can be done in another PR.

SplitContainerDragger can be excluded when looping over the children, if (get_child(i)->is_class("SplitContainerDragger")).

• There is a related proposal: SplitContainer - add percentage option for better scaling and UI responsiveness. godot-proposals#4033

I will have a look on it.

Edit:

Size flags are useful for initializing the dragger_offsets when it's invalid or empty. if the node is inside the edited scene, the offsets should be tottaly ignored.

[WhalesState]

A workaround for controlling offsets in the editor is by using the Control.size_flags to override the offsets in the canvas item editor, check Node.is_part_of_edited_scene() since this shouldn't be applied for editor plugins or running scenes unless the dragger_offsets is invalid, like using set_dragger_offsets([]) to reset the offsets.

[kitbdev]

Let's say we use offsets as a ratio which seems the best solution for this, it won't break compatibility since we can control the old behaviour in the deprecated method by converting old offset offset / (vertical ? size.y : size.x).

I don't think converting it is enough, since if the size changes, the value changes as well.

We better force all the children to only use the FILL flag and we expand them by default when they are created for the first time

We can't make SplitContainer not use its children's size flags, that breaks compatibility. Even the editor uses size flags in SplitContainers. It would make the implementation simpler, but it wouldn't have much benefit to the user.
It is also useful, since it is used when resizing the SplitContainer to know which side to expand.

SplitContainerDragger can be excluded when looping over the children, if (get_child(i)->is_class("SplitContainerDragger")).

Yes, it should be simple to implement, but it is not really related to this PR. It's better to have a separate PR for it.

[WhalesState]

I have found an issue, by hiding the left or the right dock completly, their draggers are not removed.

31.08.2024_02.01.46_REC.mp4

[kitbdev]

Draggers are now hidden when there is 0 or 1 child.
They aren't removed since the first one is added in the constructor, and it could cause issues with duplicate.

[WhalesState]

Great work.
Thanks for making this and good luck with your software games ^^.
Sorry i thought you are the one who was making the blender like UI 😄.

[kitbdev]

• Rebased Use multiple children in dock SplitContainers to make resizing consistent #90439 on this so it can be tested on the editor ui.

There are some issues when docks are moved (when a child visibility changes), docks can jump around when they get another's split offset. There is also a case where the minimum size isn't respected, so this still needs some work.
Maybe invisible children should be considered somehow? or automatically remove / insert a split_offset when children hide/show? We may need to attach some metadata to the children for this, like TabContainer does.

[WhalesState]

Maybe invisible children should be considered somehow? or automatically remove / insert a split_offset when children hide/show? We may need to attach some metadata to the children for this, like TabContainer does.

In cases like hiding a child, it's dragger offset should be kept but ignored and it's dragger should hide, the next child should use an offset based on it's child index. this will preserve the layout when showing/hiding a child control.

In cases like adding/removing children in game or editor plugin, i see that the layout is reset based on the size flags, which will ignore the current offsets that was set by user, it means that if one child is set to expand, and others are not, all the children that aren't expanding will be shrinked to their minimum size, which will force the user to set the offsets again from start, this seems wrong and offsets set by user should also be respected in game and editor plugins. to fix this [in game and editor plugins only] children should be arranged the first time based on their size flags, then once the offsets are set, they should always be respected and not be overrided by size flags except when expanding, the offsets should be tweaked and not reset.

In cases like resizing, it seems to work as expected, children with size flag expand expands equally and others respect their offsets.

Just make some tweaks to the EditorInterface, and sort all the docks alongside the center v split container inside one HSplitContainer and you will notice those issues that i have mentioned.

Edit:

The editor for example is using 5 vsplits currently [left dock, left dock 2, center/botom screen, right dock 2, right dock] all of them can become hidden/visible on demand except the center/bottom screen which is set to expand horizontally, and all other docks are set to fill, and when setting their offsets manually then resizing the editor window, they can shrink to their minimum size, and expand back to their offset that was set by the user. this behavior should be the same when you make all the vsplits inside one hsplit container instead of four, and if you do that, hiding any dock completly will reset the layout to make all the visible docks shrink and the main screen expands.

[kitbdev]

• Rebased to fix merge conflicts after Improvements to SplitContainer including a drag bar background StyleBox #72680

Still working on the visibility issue,

• Due to the complexity and chance of regressions, I'd like to add some tests first Add tests for SplitContainer #97371