Container queue_sort not working

[TheDuriel]

Godot version:

3.1

Issue description:

Containers sort_children() function is meant to queue a NOTIFICATION_SORT_CHILDREN. But instead nothing happens.

Steps to reproduce:

tool
extends Container

export(float, 0.25, 1.0) var shrink_factor: float = 0.8 setget set_shrink_factor
export(int, "Vertical", "Horizontal") var list_mode: int = 0 setget set_list_mode
export(StyleBox) var normal_style: StyleBox
const LIST_VERTICAL: int = 0
const LIST_HORIZONTAL: int = 1


func set_shrink_factor(new_value: float) -> void:
	shrink_factor = clamp(new_value, 0.25, 1.0)
	#queue_sort()
	_notification(NOTIFICATION_SORT_CHILDREN)


func set_list_mode(new_value: int) -> void:
	list_mode = new_value if new_value == LIST_VERTICAL or new_value == LIST_HORIZONTAL else 0
	#queue_sort()
	_notification(NOTIFICATION_SORT_CHILDREN)


func _draw() -> void:
	if is_instance_valid(normal_style):
		draw_style_box(normal_style, Rect2(Vector2(), rect_size))


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


func _sort_children() -> void:
	var children: Array = get_children()
	for child in children:
		if not child is Control:
			children.erase(child)
	
	var children_sizes: Array = []
	
	for child in children:
		children_sizes.append(child.rect_size * shrink_factor)
	
	var my_size: Vector2 = Vector2()
	
	for child_size in children_sizes:
		if list_mode == LIST_VERTICAL:
			if child_size.x > my_size.x:
				my_size.x = child_size.x
			
			my_size.y += child_size.y
		
		elif list_mode == LIST_HORIZONTAL:
			if child_size.y > my_size.y:
				my_size.y = child_size.y
			
			my_size.x += child_size.x
	
	rect_min_size = my_size
	rect_size =  my_size
	
	var child_positions: Array = []
	
	for child_idx in children.size():
		var child: Control = children[child_idx]
		var child_pos: Vector2 = Vector2()
		
		for idx in child_idx:
			# Skip this child
			if idx == child_idx:
				break
			
			if list_mode == LIST_VERTICAL:
				child_pos.y += children_sizes[idx].y
			
			elif list_mode == LIST_HORIZONTAL:
				child_pos.x += children_sizes[idx].x
		
		children[child_idx].rect_position = child_pos
		children[child_idx].rect_scale = Vector2(shrink_factor, shrink_factor)


func add_child(child: Node, legible_unique_name: bool = false) -> void:
	.add_child(child, legible_unique_name)
	if child is Control:
		child.connect("tree_exited", self, "_on_child_tree_exited")
		yield(get_tree(), "idle_frame")
		#queue_sort()
		_notification(NOTIFICATION_SORT_CHILDREN)


func _on_child_tree_exited() -> void:
	#queue_sort()
	_notification(NOTIFICATION_SORT_CHILDREN)

The above code works as expected when calling _notification. I would expect that a queue_sort() call would have the same effect. Bug? Documentation error? Looking at source it implies it works as I expect.

https://github.com/godotengine/godot/blob/master/scene/gui/container.cpp#L135

[pouleyKetchoupp]

Can't reproduce on 3.1 stable release 64-bit on windows 10.

I've made a minimal project for test:
sort-children.zip
It should print "NOTIFICATION_SORT_CHILDREN" when you click the Sort property in the inspector.

@TheDuriel Can you test with my project? If it works for you, then please share a project for your repro.

[TheDuriel]

https://github.com/godotengine/godot/files/3509487/buttonlist.zip This is from a different issue of mine. Ignore the broken signals. If you replace the notification calls with the queue calls it should stop responding to updates when you edit the exported properties or add nodes to the container.

Ill take a look at your project.

[pouleyKetchoupp]

Ok, I get it!

You don't get the notification because your script overrides _sort_children which is where NOTIFICATION_SORT_CHILDREN is sent.

It works fine if you add ._sort_children() in your function (better at the end so you don't get and extra sort due to changing the size of the control).

You'll also have to remove sort_children() from the _notification function to avoid a stack overflow crash.

[TheDuriel]

sort_children and _sort_children should be very different functions.

to override a function the name must an identical match. even so, if it was overriding it, then would that not result in the exact same result as calling _notification... considering all _notification is doing is calling _sort_children?

[pouleyKetchoupp]

Sorry for the confusion, I meant to only refer to _sort_children in my previous message.

The main idea is that queue_sort doesn't trigger the notification, it triggers a call to _sort_children which triggers the notification in its default implementation.

Now the reason why you don't get the sorting to happen at all:
When you call queue_sort the first time (on scene load), it sets pending_sort = true; and it's reset to false only in _sort_children. In your case, because _sort_children is overridden, it never does that. Which means any other call to queue_sort is going to exit when testing if (pending_sort) return; (this is probably meant to avoid sorting several times during the same frame).

Hope it's clearer now :)

Code references:

godot/scene/gui/container.cpp

Lines 135 to 145 in afecc0b

	void Container::queue_sort() {
	if (!is_inside_tree())
	return;
	if (pending_sort)
	return;
	MessageQueue::get_singleton()->push_call(this, "_sort_children");
	pending_sort = true;
	}

godot/scene/gui/container.cpp

Lines 86 to 94 in afecc0b

	void Container::_sort_children() {
	if (!is_inside_tree())
	return;
	notification(NOTIFICATION_SORT_CHILDREN);
	emit_signal(SceneStringNames::get_singleton()->sort_children);
	pending_sort = false;
	}

[TheDuriel]

Yeah so this is interesting. _sort_children is bound in the Container.cpp
However. It is not bound to GDScript. Or at least the docs builder can not find it.

https://docs.godotengine.org/en/latest/classes/class_container.html#class-container

I have my doubts whether or not this is the problem at hand, but it could be.

[pouleyKetchoupp]

I see! You're right, it looks like the intended behavior is for the Container class to send NOTIFICATION_SORT_CHILDREN signal to trigger the sorting. That's how all the containers work.

A side effect of the way it has been implemented is that if you have a function called _sort_children in your script, it will be called instead of the actual method from the class. First I thought the function overriding was intended, but it doesn't seem to be the case. I'll see if I can fix it.