Deprecate NOTIFICATION_MOVED_IN_PARENT

* NOTIFICATION_MOVED_IN_PARENT makes node children management very inefficient.
* Replaced by a NOTIFICATION_CHILD_ORDER_CHANGED (and children_changed signal).
* Most of the previous tasks carried out by NOTIFICATION_MOVED_IN_PARENT are now done not more than a single time per frame.

This PR breaks compatibility (although this notification was very rarely used, even within the engine), but provides an alternate way to do the same.

doc/classes/Node.xml

@@ -797,6 +797,11 @@
 				When this signal is received, the child [code]node[/code] is still in the tree and valid. This signal is emitted [i]after[/i] the child node's own [signal tree_exiting] and [constant NOTIFICATION_EXIT_TREE].
 			</description>
 		</signal>
+		<signal name="child_order_changed">
+			<description>
+				Emitted when the list of children is changed. This happens when child nodes are added, moved, or removed.
+			</description>
+		</signal>
 		<signal name="ready">
 			<description>
 				Emitted when the node is ready.
@@ -835,7 +840,7 @@
 			This notification is emitted [i]after[/i] the related [signal tree_exiting].
 		</constant>
 		<constant name="NOTIFICATION_MOVED_IN_PARENT" value="12">
-			Notification received when the node is moved in the parent.
+			[i]Deprecated.[/i] This notification is no longer emitted. Use [constant NOTIFICATION_CHILD_ORDER_CHANGED] instead.
 		</constant>
 		<constant name="NOTIFICATION_READY" value="13">
 			Notification received when the node is ready. See [method _ready].
@@ -874,6 +879,9 @@
 		<constant name="NOTIFICATION_PATH_CHANGED" value="23">
 			Notification received when the node's [NodePath] changed.
 		</constant>
+		<constant name="NOTIFICATION_CHILD_ORDER_CHANGED" value="24">
+			Notification received when the list of children is changed. This happens when child nodes are added, moved, or removed.
+		</constant>
 		<constant name="NOTIFICATION_INTERNAL_PROCESS" value="25">
 			Notification received every frame when the internal process flag is set (see [method set_process_internal]).
 		</constant>

main/main.cpp

@@ -2399,6 +2399,12 @@ bool Main::iteration() {
 		exit = true;
 	}
 	visual_server_callbacks->flush();
+
+	// Ensure that VisualServer is kept up to date at least once with any ordering changes
+	// of canvas items before a render.
+	// This ensures this will be done at least once in apps that create their own MainLoop.
+	Viewport::flush_canvas_parents_dirty_order();
+
 	message_queue->flush();
 
 	VisualServer::get_singleton()->sync(); //sync if still drawing from previous frames.

scene/2d/canvas_item.cpp

@@ -611,20 +611,6 @@ void CanvasItem::_notification(int p_what) {
 				notification(NOTIFICATION_RESET_PHYSICS_INTERPOLATION);
 			}
 		} break;
-		case NOTIFICATION_MOVED_IN_PARENT: {
-			if (!is_inside_tree()) {
-				break;
-			}
-
-			if (canvas_group != "") {
-				get_tree()->call_group_flags(SceneTree::GROUP_CALL_UNIQUE, canvas_group, "_toplevel_raise_self");
-			} else {
-				CanvasItem *p = get_parent_item();
-				ERR_FAIL_COND(!p);
-				VisualServer::get_singleton()->canvas_item_set_draw_index(canvas_item, get_index());
-			}
-
-		} break;
 		case NOTIFICATION_EXIT_TREE: {
 			if (xform_change.in_list()) {
 				get_tree()->xform_change_list.remove(&xform_change);
@@ -662,6 +648,20 @@ void CanvasItem::_name_changed_notify() {
 }
 #endif
 
+void CanvasItem::update_draw_order() {
+	if (!is_inside_tree()) {
+		return;
+	}
+
+	if (canvas_group != "") {
+		get_tree()->call_group_flags(SceneTree::GROUP_CALL_UNIQUE, canvas_group, "_toplevel_raise_self");
+	} else {
+		CanvasItem *p = get_parent_item();
+		ERR_FAIL_NULL(p);
+		VisualServer::get_singleton()->canvas_item_set_draw_index(canvas_item, get_index());
+	}
+}
+
 void CanvasItem::update() {
 	if (!is_inside_tree()) {
 		return;

scene/2d/canvas_item.h

@@ -296,6 +296,8 @@ public:
 	virtual Transform2D _edit_get_transform() const;
 #endif
 
+	void update_draw_order();
+
 	/* VISIBILITY */
 
 	void set_visible(bool p_visible);

scene/2d/skeleton_2d.cpp

@@ -30,9 +30,20 @@
 
 #include "skeleton_2d.h"
 
+void Bone2D::_order_changed_in_parent() {
+	if (skeleton) {
+		skeleton->_make_bone_setup_dirty();
+	}
+}
+
 void Bone2D::_notification(int p_what) {
 	if (p_what == NOTIFICATION_ENTER_TREE) {
 		Node *parent = get_parent();
+
+		if (parent) {
+			parent->connect("child_order_changed", this, "_order_changed_in_parent");
+		}
+
 		parent_bone = Object::cast_to<Bone2D>(parent);
 		skeleton = nullptr;
 		while (parent) {
@@ -59,13 +70,13 @@ void Bone2D::_notification(int p_what) {
 			skeleton->_make_transform_dirty();
 		}
 	}
-	if (p_what == NOTIFICATION_MOVED_IN_PARENT) {
-		if (skeleton) {
-			skeleton->_make_bone_setup_dirty();
-		}
-	}
 
 	if (p_what == NOTIFICATION_EXIT_TREE) {
+		Node *parent = get_parent();
+		if (parent) {
+			parent->disconnect("child_order_changed", this, "_order_changed_in_parent");
+		}
+
 		if (skeleton) {
 			for (int i = 0; i < skeleton->bones.size(); i++) {
 				if (skeleton->bones[i].bone == this) {
@@ -89,6 +100,8 @@ void Bone2D::_bind_methods() {
 	ClassDB::bind_method(D_METHOD("set_default_length", "default_length"), &Bone2D::set_default_length);
 	ClassDB::bind_method(D_METHOD("get_default_length"), &Bone2D::get_default_length);
 
+	ClassDB::bind_method(D_METHOD("_order_changed_in_parent"), &Bone2D::_order_changed_in_parent);
+
 	ADD_PROPERTY(PropertyInfo(Variant::TRANSFORM2D, "rest"), "set_rest", "get_rest");
 	ADD_PROPERTY(PropertyInfo(Variant::REAL, "default_length", PROPERTY_HINT_RANGE, "1,1024,1"), "set_default_length", "get_default_length");
 }

scene/2d/skeleton_2d.h

@@ -53,6 +53,7 @@ class Bone2D : public Node2D {
 protected:
 	void _notification(int p_what);
 	static void _bind_methods();
+	void _order_changed_in_parent();
 
 public:
 	void set_rest(const Transform2D &p_rest);

scene/gui/control.cpp

@@ -592,22 +592,6 @@ void Control::_notification(int p_notification) {
 			}
 			*/
 
-		} break;
-		case NOTIFICATION_MOVED_IN_PARENT: {
-			// some parents need to know the order of the children to draw (like TabContainer)
-			// update if necessary
-			if (data.parent) {
-				data.parent->update();
-			}
-			update();
-
-			if (data.SI) {
-				get_viewport()->_gui_set_subwindow_order_dirty();
-			}
-			if (data.RI) {
-				get_viewport()->_gui_set_root_order_dirty();
-			}
-
 		} break;
 		case NOTIFICATION_RESIZED: {
 			emit_signal(SceneStringNames::get_singleton()->resized);
@@ -2665,6 +2649,15 @@ Control::GrowDirection Control::get_v_grow_direction() const {
 	return data.v_grow;
 }
 
+void Control::_query_order_update(bool &r_subwindow_order_dirty, bool &r_root_order_dirty) const {
+	if (data.SI) {
+		r_subwindow_order_dirty = true;
+	}
+	if (data.RI) {
+		r_root_order_dirty = true;
+	}
+}
+
 void Control::_bind_methods() {
 	//ClassDB::bind_method(D_METHOD("_window_resize_event"),&Control::_window_resize_event);
 	ClassDB::bind_method(D_METHOD("_size_changed"), &Control::_size_changed);

scene/gui/control.h

@@ -508,6 +508,8 @@ public:
 	virtual void get_argument_options(const StringName &p_function, int p_idx, List<String> *r_options) const;
 	virtual String get_configuration_warning() const;
 
+	void _query_order_update(bool &r_subwindow_order_dirty, bool &r_root_order_dirty) const;
+
 	Control();
 	~Control();
 };

scene/main/canvas_layer.cpp

@@ -228,15 +228,16 @@ void CanvasLayer::_notification(int p_what) {
 			viewport = RID();
 			_update_follow_viewport(false);
 		} break;
-		case NOTIFICATION_MOVED_IN_PARENT: {
-			// Note: As this step requires traversing the entire scene tree, it is thus expensive
-			// to move the canvas layer multiple times. Take special care when deleting / moving
-			// multiple nodes to prevent multiple NOTIFICATION_MOVED_IN_PARENT occurring.
-			_update_layer_orders();
-		} break;
 	}
 }
 
+void CanvasLayer::update_draw_order() {
+	// Note: As this step requires traversing the entire scene tree, it is thus expensive
+	// to move the canvas layer multiple times. Take special care when deleting / moving
+	// multiple nodes to prevent this happening multiple times.
+	_update_layer_orders();
+}
+
 Size2 CanvasLayer::get_viewport_size() const {
 	if (!is_inside_tree()) {
 		return Size2(1, 1);

scene/main/canvas_layer.h

@@ -70,6 +70,8 @@ protected:
 	static void _bind_methods();
 
 public:
+	void update_draw_order();
+
 	void set_layer(int p_xform);
 	int get_layer() const;
 

scene/main/node.cpp

@@ -426,9 +426,8 @@ void Node::move_child(Node *p_child, int p_pos) {
 	}
 	// notification second
 	move_child_notify(p_child);
-	for (int i = motion_from; i <= motion_to; i++) {
-		data.children[i]->notification(NOTIFICATION_MOVED_IN_PARENT);
-	}
+	Viewport::notify_canvas_parent_children_moved(*this, motion_from, motion_to + 1);
+
 	p_child->_propagate_groups_dirty();
 
 	data.blocked--;
@@ -1364,9 +1363,11 @@ void Node::remove_child(Node *p_child) {
 
 	for (int i = idx; i < child_count; i++) {
 		children[i]->data.pos = i;
-		children[i]->notification(NOTIFICATION_MOVED_IN_PARENT);
 	}
 
+	Viewport::notify_canvas_parent_children_moved(*this, idx, child_count);
+	Viewport::notify_canvas_parent_child_count_reduced(*this);
+
 	p_child->data.parent = nullptr;
 	p_child->data.pos = -1;
 
@@ -3193,6 +3194,7 @@ void Node::_bind_methods() {
 	BIND_CONSTANT(NOTIFICATION_DRAG_BEGIN);
 	BIND_CONSTANT(NOTIFICATION_DRAG_END);
 	BIND_CONSTANT(NOTIFICATION_PATH_CHANGED);
+	BIND_CONSTANT(NOTIFICATION_CHILD_ORDER_CHANGED);
 	BIND_CONSTANT(NOTIFICATION_INTERNAL_PROCESS);
 	BIND_CONSTANT(NOTIFICATION_INTERNAL_PHYSICS_PROCESS);
 	BIND_CONSTANT(NOTIFICATION_POST_ENTER_TREE);
@@ -3233,6 +3235,7 @@ void Node::_bind_methods() {
 	ADD_SIGNAL(MethodInfo("tree_exited"));
 	ADD_SIGNAL(MethodInfo("child_entered_tree", PropertyInfo(Variant::OBJECT, "node", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT, "Node")));
 	ADD_SIGNAL(MethodInfo("child_exiting_tree", PropertyInfo(Variant::OBJECT, "node", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT, "Node")));
+	ADD_SIGNAL(MethodInfo("child_order_changed"));
 
 	ADD_PROPERTY(PropertyInfo(Variant::INT, "pause_mode", PROPERTY_HINT_ENUM, "Inherit,Stop,Process"), "set_pause_mode", "get_pause_mode");
 

scene/main/node.h

@@ -134,6 +134,11 @@ private:
 
 		int process_priority;
 
+		// If canvas item children of a node change child order,
+		// we store this information in the scenetree in a temporary structure
+		// allocated on demand per node.
+		uint32_t canvas_parent_id = UINT32_MAX;
+
 		// Keep bitpacked values together to get better packing
 		PauseMode pause_mode : 2;
 		PhysicsInterpolationMode physics_interpolation_mode : 2;
@@ -279,7 +284,7 @@ public:
 		NOTIFICATION_DRAG_BEGIN = 21,
 		NOTIFICATION_DRAG_END = 22,
 		NOTIFICATION_PATH_CHANGED = 23,
-		//NOTIFICATION_TRANSLATION_CHANGED = 24, moved below
+		NOTIFICATION_CHILD_ORDER_CHANGED = 24,
 		NOTIFICATION_INTERNAL_PROCESS = 25,
 		NOTIFICATION_INTERNAL_PHYSICS_PROCESS = 26,
 		NOTIFICATION_POST_ENTER_TREE = 27,
@@ -455,6 +460,9 @@ public:
 	_FORCE_INLINE_ bool is_physics_interpolated_and_enabled() const { return is_inside_tree() && get_tree()->is_physics_interpolation_enabled() && is_physics_interpolated(); }
 	void reset_physics_interpolation();
 
+	uint32_t get_canvas_parent_id() const { return data.canvas_parent_id; }
+	void set_canvas_parent_id(uint32_t p_id) { data.canvas_parent_id = p_id; }
+
 	bool is_node_ready() const;
 	void request_ready();
 

scene/main/viewport.cpp

@@ -54,6 +54,8 @@
 #include "scene/scene_string_names.h"
 #include "servers/physics_2d_server.h"
 
+LocalVector<Viewport::GUI::CanvasParent> Viewport::GUI::canvas_parents_dirty_order;
+
 void ViewportTexture::setup_local_to_scene() {
 	Node *local_scene = get_local_scene();
 	if (!local_scene) {
@@ -3214,6 +3216,130 @@ bool Viewport::is_handling_input_locally() const {
 	return handle_input_locally;
 }
 
+void Viewport::notify_canvas_parent_child_count_reduced(const Node &p_parent) {
+	uint32_t id = p_parent.get_canvas_parent_id();
+
+	ERR_FAIL_COND(id == UINT32_MAX);
+	ERR_FAIL_UNSIGNED_INDEX(id, GUI::canvas_parents_dirty_order.size());
+
+	GUI::CanvasParent &d = GUI::canvas_parents_dirty_order[id];
+
+	// No pending children moved.
+	if (!d.last_child_moved_plus_one) {
+		return;
+	}
+
+	uint32_t num_children = p_parent.get_child_count();
+	d.last_child_moved_plus_one = MIN(d.last_child_moved_plus_one, num_children);
+}
+
+void Viewport::notify_canvas_parent_children_moved(Node &r_parent, uint32_t p_first_child, uint32_t p_last_child_plus_one) {
+	// First ensure the parent has a CanvasParent allocated.
+	uint32_t id = r_parent.get_canvas_parent_id();
+
+	if (id == UINT32_MAX) {
+		id = GUI::canvas_parents_dirty_order.size();
+		r_parent.set_canvas_parent_id(id);
+		GUI::canvas_parents_dirty_order.resize(id + 1);
+
+		GUI::CanvasParent &d = GUI::canvas_parents_dirty_order[id];
+		d.id = r_parent.get_instance_id();
+		d.first_child_moved = p_first_child;
+		d.last_child_moved_plus_one = p_last_child_plus_one;
+		return;
+	}
+
+	GUI::CanvasParent &d = GUI::canvas_parents_dirty_order[id];
+	DEV_CHECK_ONCE(d.id == r_parent.get_instance_id());
+	d.first_child_moved = MIN(d.first_child_moved, p_first_child);
+	d.last_child_moved_plus_one = MAX(d.last_child_moved_plus_one, p_last_child_plus_one);
+}
+
+void Viewport::flush_canvas_parents_dirty_order() {
+	bool canvas_layers_moved = false;
+
+	for (uint32_t n = 0; n < GUI::canvas_parents_dirty_order.size(); n++) {
+		GUI::CanvasParent &d = GUI::canvas_parents_dirty_order[n];
+
+		Object *obj = ObjectDB::get_instance(d.id);
+		if (!obj) {
+			// May have been deleted.
+			continue;
+		}
+
+		Node *node = static_cast<Node *>(obj);
+		Control *parent_control = Object::cast_to<Control>(node);
+
+		// Allow anything subscribing to this signal (skeletons etc)
+		// to make any changes necessary.
+		node->emit_signal("child_order_changed");
+
+		// Reset the id stored in the node, as this data is cleared after every flush.
+		node->set_canvas_parent_id(UINT32_MAX);
+
+		// This should be very rare, as the last_child_moved_plus_one is usually kept up
+		// to date when within the scene tree. But it may become out of date outside the scene tree.
+		// This will cause no problems, just a very small possibility of more notifications being sent than
+		// necessary in that very rare situation.
+		if (d.last_child_moved_plus_one > (uint32_t)node->get_child_count()) {
+			d.last_child_moved_plus_one = node->get_child_count();
+		}
+
+		bool subwindow_order_dirty = false;
+		bool root_order_dirty = false;
+		bool control_found = false;
+
+		for (uint32_t c = d.first_child_moved; c < d.last_child_moved_plus_one; c++) {
+			Node *child = node->get_child(c);
+
+			CanvasItem *ci = Object::cast_to<CanvasItem>(child);
+			if (ci) {
+				ci->update_draw_order();
+
+				Control *control = Object::cast_to<Control>(ci);
+				if (control) {
+					control->_query_order_update(subwindow_order_dirty, root_order_dirty);
+
+					if (parent_control && !control_found) {
+						control_found = true;
+
+						// In case of regressions, this could be moved to AFTER
+						// the children have been updated.
+						parent_control->update();
+					}
+					control->update();
+				}
+
+				continue;
+			}
+
+			CanvasLayer *cl = Object::cast_to<CanvasLayer>(child);
+			if (cl) {
+				// If any canvas layers moved, we need to do an expensive update,
+				// so we ensure doing this only once.
+				if (!canvas_layers_moved) {
+					canvas_layers_moved = true;
+					cl->update_draw_order();
+				}
+			}
+		}
+
+		Viewport *viewport = node->get_viewport();
+		if (viewport) {
+			if (subwindow_order_dirty) {
+				viewport->_gui_set_subwindow_order_dirty();
+			}
+			if (root_order_dirty) {
+				viewport->_gui_set_root_order_dirty();
+			}
+		}
+
+		node->notification(Node::NOTIFICATION_CHILD_ORDER_CHANGED);
+	}
+
+	GUI::canvas_parents_dirty_order.clear();
+}
+
 void Viewport::_validate_property(PropertyInfo &property) const {
 	if (VisualServer::get_singleton()->is_low_end() && (property.name == "hdr" || property.name == "use_32_bpc_depth" || property.name == "debanding" || property.name == "sharpen_intensity" || property.name == "debug_draw")) {
 		// Only available in GLES3.

scene/main/viewport.h

@@ -326,6 +326,14 @@ private:
 		bool dragging;
 		bool drag_successful;
 
+		struct CanvasParent {
+			ObjectID id;
+			uint32_t first_child_moved = UINT32_MAX;
+			uint32_t last_child_moved_plus_one = 0;
+		};
+
+		static LocalVector<CanvasParent> canvas_parents_dirty_order;
+
 		GUI();
 	} gui;
 
@@ -588,6 +596,10 @@ public:
 	bool gui_is_dragging() const;
 	bool gui_is_drag_successful() const;
 
+	static void notify_canvas_parent_children_moved(Node &r_parent, uint32_t p_first_child, uint32_t p_last_child_plus_one);
+	static void notify_canvas_parent_child_count_reduced(const Node &p_parent);
+	static void flush_canvas_parents_dirty_order();
+
 	Viewport();
 	~Viewport();
 };