From 8a76e315473228300922b957375a79e8cb157fcb Mon Sep 17 00:00:00 2001 From: Lukas Tenbrink Date: Tue, 1 Apr 2025 12:50:14 +0200 Subject: [PATCH] Remove bool from `Object::notification` virtual function; replace with separate functions to avoid branching. --- core/object/object.cpp | 40 ++++++++++------ core/object/object.h | 31 +++++++++---- tests/core/object/test_object.h | 81 +++++++++++++++++---------------- 3 files changed, 91 insertions(+), 61 deletions(-) diff --git a/core/object/object.cpp b/core/object/object.cpp index 899e69650dc..e801fc58d6f 100644 --- a/core/object/object.cpp +++ b/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(p_reversed)); + _extension->notification2(_extension_instance, p_notification, static_cast(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(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() { diff --git a/core/object/object.h b/core/object/object.h index 38d0ad7f410..556acd83df6 100644 --- a/core/object/object.h +++ b/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. diff --git a/tests/core/object/test_object.h b/tests/core/object/test_object.h index 92a7edc9a46..0417342b452 100644 --- a/tests/core/object/test_object.h +++ b/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); + + NotificationScriptInstance *script = memnew(NotificationScriptInstance); + object->set_script_instance(script); SUBCASE("regular order") { - test_notification_object->notification(12345, false); + NotificationObjectSubclass::order_global = 0; + object->order_superclass = -1; + object->order_subclass = -1; + script->order_script = -1; + object->notification(12345, false); - CHECK_EQ(test_notification_object->order_internal1, 1); - CHECK_EQ(test_notification_object->order_internal2, 2); - - 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); + NotificationObjectSubclass::order_global = 0; + object->order_superclass = -1; + object->order_subclass = -1; + script->order_script = -1; + object->notification(12345, true); - CHECK_EQ(test_notification_object->order_internal1, 2); - CHECK_EQ(test_notification_object->order_internal2, 1); - - 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") {