ソースを参照

Remove bool from `Object::notification` virtual function; replace with separate functions to avoid branching.

Lukas Tenbrink 5 ヶ月 前
コミット
8a76e31547
3 ファイル変更91 行追加61 行削除
  1. 26 14
      core/object/object.cpp
  2. 23 8
      core/object/object.h
  3. 42 39
      tests/core/object/test_object.h

+ 26 - 14
core/object/object.cpp

@@ -911,18 +911,14 @@ Variant Object::call_const(const StringName &p_method, const Variant **p_args, i
 	return ret;
 }
 
-void Object::notification(int p_notification, bool p_reversed) {
-	if (p_reversed) {
-		if (script_instance) {
-			script_instance->notification(p_notification, p_reversed);
-		}
-	} else {
-		_notificationv(p_notification, p_reversed);
-	}
+void Object::_notification_forward(int p_notification) {
+	// Notify classes starting with Object and ending with most derived subclass.
+	// e.g. Object -> Node -> Node3D
+	_notification_forwardv(p_notification);
 
 	if (_extension) {
 		if (_extension->notification2) {
-			_extension->notification2(_extension_instance, p_notification, static_cast<GDExtensionBool>(p_reversed));
+			_extension->notification2(_extension_instance, p_notification, static_cast<GDExtensionBool>(false));
 #ifndef DISABLE_DEPRECATED
 		} else if (_extension->notification) {
 			_extension->notification(_extension_instance, p_notification);
@@ -930,13 +926,29 @@ void Object::notification(int p_notification, bool p_reversed) {
 		}
 	}
 
-	if (p_reversed) {
-		_notificationv(p_notification, p_reversed);
-	} else {
-		if (script_instance) {
-			script_instance->notification(p_notification, p_reversed);
+	if (script_instance) {
+		script_instance->notification(p_notification, false);
+	}
+}
+
+void Object::_notification_backward(int p_notification) {
+	if (script_instance) {
+		script_instance->notification(p_notification, true);
+	}
+
+	if (_extension) {
+		if (_extension->notification2) {
+			_extension->notification2(_extension_instance, p_notification, static_cast<GDExtensionBool>(true));
+#ifndef DISABLE_DEPRECATED
+		} else if (_extension->notification) {
+			_extension->notification(_extension_instance, p_notification);
+#endif // DISABLE_DEPRECATED
 		}
 	}
+
+	// Notify classes starting with most derived subclass and ending in Object.
+	// e.g. Node3D -> Node -> Object
+	_notification_backwardv(p_notification);
 }
 
 String Object::to_string() {

+ 23 - 8
core/object/object.h

@@ -544,16 +544,17 @@ protected:
 	_FORCE_INLINE_ void (Object::*_get_notification() const)(int) {                                                                         \
 		return (void(Object::*)(int)) & m_class::_notification;                                                                             \
 	}                                                                                                                                       \
-	virtual void _notificationv(int p_notification, bool p_reversed) override {                                                             \
-		if (!p_reversed) {                                                                                                                  \
-			m_inherits::_notificationv(p_notification, p_reversed);                                                                         \
-		}                                                                                                                                   \
+	virtual void _notification_forwardv(int p_notification) override {                                                                      \
+		m_inherits::_notification_forwardv(p_notification);                                                                                 \
 		if (m_class::_get_notification() != m_inherits::_get_notification()) {                                                              \
 			_notification(p_notification);                                                                                                  \
 		}                                                                                                                                   \
-		if (p_reversed) {                                                                                                                   \
-			m_inherits::_notificationv(p_notification, p_reversed);                                                                         \
+	}                                                                                                                                       \
+	virtual void _notification_backwardv(int p_notification) override {                                                                     \
+		if (m_class::_get_notification() != m_inherits::_get_notification()) {                                                              \
+			_notification(p_notification);                                                                                                  \
 		}                                                                                                                                   \
+		m_inherits::_notification_backwardv(p_notification);                                                                                \
 	}                                                                                                                                       \
                                                                                                                                             \
 private:
@@ -697,7 +698,11 @@ protected:
 	virtual void _validate_propertyv(PropertyInfo &p_property) const {}
 	virtual bool _property_can_revertv(const StringName &p_name) const { return false; }
 	virtual bool _property_get_revertv(const StringName &p_name, Variant &r_property) const { return false; }
-	virtual void _notificationv(int p_notification, bool p_reversed) {}
+
+	void _notification_forward(int p_notification);
+	void _notification_backward(int p_notification);
+	virtual void _notification_forwardv(int p_notification) {}
+	virtual void _notification_backwardv(int p_notification) {}
 
 	static void _bind_methods();
 	static void _bind_compatibility_methods() {}
@@ -895,7 +900,17 @@ public:
 		return (cerr.error == Callable::CallError::CALL_OK) ? ret : Variant();
 	}
 
-	void notification(int p_notification, bool p_reversed = false);
+	// Depending on the boolean, we call either the virtual function _notification_backward or _notification_forward.
+	// - Forward calls subclasses in descending order (e.g. Object -> Node -> Node3D -> extension -> script).
+	//   Backward calls subclasses in descending order (e.g. script -> extension -> Node3D -> Node -> Object).
+	_FORCE_INLINE_ void notification(int p_notification, bool p_reversed = false) {
+		if (p_reversed) {
+			_notification_backward(p_notification);
+		} else {
+			_notification_forward(p_notification);
+		}
+	}
+
 	virtual String to_string();
 
 	// Used mainly by script, get and set all INCLUDING string.

+ 42 - 39
tests/core/object/test_object.h

@@ -465,72 +465,75 @@ TEST_CASE("[Object] Signals") {
 	}
 }
 
-class NotificationObject1 : public Object {
-	GDCLASS(NotificationObject1, Object);
+class NotificationObjectSuperclass : public Object {
+	GDCLASS(NotificationObjectSuperclass, Object);
 
 protected:
 	void _notification(int p_what) {
-		switch (p_what) {
-			case 12345: {
-				order_internal1 = order_global++;
-			} break;
-		}
+		order_superclass = ++order_global;
 	}
 
 public:
-	static int order_global;
-	int order_internal1 = -1;
-
-	void reset_order() {
-		order_internal1 = -1;
-		order_global = 1;
-	}
+	static inline int order_global = 0;
+	int order_superclass = -1;
 };
 
-int NotificationObject1::order_global = 1;
-
-class NotificationObject2 : public NotificationObject1 {
-	GDCLASS(NotificationObject2, NotificationObject1);
+class NotificationObjectSubclass : public NotificationObjectSuperclass {
+	GDCLASS(NotificationObjectSubclass, NotificationObjectSuperclass);
 
 protected:
 	void _notification(int p_what) {
-		switch (p_what) {
-			case 12345: {
-				order_internal2 = order_global++;
-			} break;
-		}
+		order_subclass = ++order_global;
 	}
 
 public:
-	int order_internal2 = -1;
-	void reset_order() {
-		NotificationObject1::reset_order();
-		order_internal2 = -1;
+	int order_subclass = -1;
+};
+
+class NotificationScriptInstance : public _MockScriptInstance {
+	void notification(int p_notification, bool p_reversed) override {
+		order_script = ++NotificationObjectSuperclass::order_global;
 	}
+
+public:
+	int order_script = -1;
 };
 
 TEST_CASE("[Object] Notification order") { // GH-52325
-	NotificationObject2 *test_notification_object = memnew(NotificationObject2);
+	NotificationObjectSubclass *object = memnew(NotificationObjectSubclass);
 
-	SUBCASE("regular order") {
-		test_notification_object->notification(12345, false);
+	NotificationScriptInstance *script = memnew(NotificationScriptInstance);
+	object->set_script_instance(script);
 
-		CHECK_EQ(test_notification_object->order_internal1, 1);
-		CHECK_EQ(test_notification_object->order_internal2, 2);
+	SUBCASE("regular order") {
+		NotificationObjectSubclass::order_global = 0;
+		object->order_superclass = -1;
+		object->order_subclass = -1;
+		script->order_script = -1;
+		object->notification(12345, false);
 
-		test_notification_object->reset_order();
+		CHECK_EQ(object->order_superclass, 1);
+		CHECK_EQ(object->order_subclass, 2);
+		// TODO If an extension is attached, it should come here.
+		CHECK_EQ(script->order_script, 3);
+		CHECK_EQ(NotificationObjectSubclass::order_global, 3);
 	}
 
 	SUBCASE("reverse order") {
-		test_notification_object->notification(12345, true);
-
-		CHECK_EQ(test_notification_object->order_internal1, 2);
-		CHECK_EQ(test_notification_object->order_internal2, 1);
+		NotificationObjectSubclass::order_global = 0;
+		object->order_superclass = -1;
+		object->order_subclass = -1;
+		script->order_script = -1;
+		object->notification(12345, true);
 
-		test_notification_object->reset_order();
+		CHECK_EQ(script->order_script, 1);
+		// TODO If an extension is attached, it should come here.
+		CHECK_EQ(object->order_subclass, 2);
+		CHECK_EQ(object->order_superclass, 3);
+		CHECK_EQ(NotificationObjectSubclass::order_global, 3);
 	}
 
-	memdelete(test_notification_object);
+	memdelete(object);
 }
 
 TEST_CASE("[Object] Destruction at the end of the call chain is safe") {