From e379cc76e5e4cdba8393ed5988f12a6c46a77493 Mon Sep 17 00:00:00 2001 From: Danil Alexeev Date: Fri, 1 Nov 2024 02:28:21 +0300 Subject: [PATCH] Core: Fix `Callable.get_bound_arguments{,_count}()` return incorrect data --- core/variant/callable.cpp | 25 +++++++---- core/variant/callable.h | 6 ++- core/variant/callable_bind.cpp | 67 +++++++++++++----------------- core/variant/callable_bind.h | 6 ++- core/variant/variant.cpp | 16 +++---- core/variant/variant_call.cpp | 1 + doc/classes/Callable.xml | 19 ++++++++- scene/main/node.cpp | 9 ++-- tests/core/variant/test_callable.h | 64 ++++++++++++++++++++++++++++ 9 files changed, 150 insertions(+), 63 deletions(-) diff --git a/core/variant/callable.cpp b/core/variant/callable.cpp index 5ce90cd8ff4..ddeea271184 100644 --- a/core/variant/callable.cpp +++ b/core/variant/callable.cpp @@ -206,19 +206,17 @@ int Callable::get_bound_arguments_count() const { } } -void Callable::get_bound_arguments_ref(Vector &r_arguments, int &r_argcount) const { +void Callable::get_bound_arguments_ref(Vector &r_arguments) const { if (!is_null() && is_custom()) { - custom->get_bound_arguments(r_arguments, r_argcount); + custom->get_bound_arguments(r_arguments); } else { r_arguments.clear(); - r_argcount = 0; } } Array Callable::get_bound_arguments() const { Vector arr; - int ac; - get_bound_arguments_ref(arr, ac); + get_bound_arguments_ref(arr); Array ret; ret.resize(arr.size()); for (int i = 0; i < arr.size(); i++) { @@ -227,6 +225,14 @@ Array Callable::get_bound_arguments() const { return ret; } +int Callable::get_unbound_arguments_count() const { + if (!is_null() && is_custom()) { + return custom->get_unbound_arguments_count(); + } else { + return 0; + } +} + CallableCustom *Callable::get_custom() const { ERR_FAIL_COND_V_MSG(!is_custom(), nullptr, vformat("Can't get custom on non-CallableCustom \"%s\".", operator String())); @@ -464,9 +470,12 @@ int CallableCustom::get_bound_arguments_count() const { return 0; } -void CallableCustom::get_bound_arguments(Vector &r_arguments, int &r_argcount) const { - r_arguments = Vector(); - r_argcount = 0; +void CallableCustom::get_bound_arguments(Vector &r_arguments) const { + r_arguments.clear(); +} + +int CallableCustom::get_unbound_arguments_count() const { + return 0; } CallableCustom::CallableCustom() { diff --git a/core/variant/callable.h b/core/variant/callable.h index e3c940a0e59..e76b888ac28 100644 --- a/core/variant/callable.h +++ b/core/variant/callable.h @@ -111,8 +111,9 @@ public: CallableCustom *get_custom() const; int get_argument_count(bool *r_is_valid = nullptr) const; int get_bound_arguments_count() const; - void get_bound_arguments_ref(Vector &r_arguments, int &r_argcount) const; // Internal engine use, the exposed one is below. + void get_bound_arguments_ref(Vector &r_arguments) const; // Internal engine use, the exposed one is below. Array get_bound_arguments() const; + int get_unbound_arguments_count() const; uint32_t hash() const; @@ -158,7 +159,8 @@ public: virtual const Callable *get_base_comparator() const; virtual int get_argument_count(bool &r_is_valid) const; virtual int get_bound_arguments_count() const; - virtual void get_bound_arguments(Vector &r_arguments, int &r_argcount) const; + virtual void get_bound_arguments(Vector &r_arguments) const; + virtual int get_unbound_arguments_count() const; CallableCustom(); virtual ~CallableCustom() {} diff --git a/core/variant/callable_bind.cpp b/core/variant/callable_bind.cpp index d82aa3583d0..0bba329d7c5 100644 --- a/core/variant/callable_bind.cpp +++ b/core/variant/callable_bind.cpp @@ -100,44 +100,42 @@ int CallableCustomBind::get_argument_count(bool &r_is_valid) const { } int CallableCustomBind::get_bound_arguments_count() const { - return callable.get_bound_arguments_count() + binds.size(); + return callable.get_bound_arguments_count() + MAX(0, binds.size() - callable.get_unbound_arguments_count()); } -void CallableCustomBind::get_bound_arguments(Vector &r_arguments, int &r_argcount) const { - Vector sub_args; - int sub_count; - callable.get_bound_arguments_ref(sub_args, sub_count); +void CallableCustomBind::get_bound_arguments(Vector &r_arguments) const { + Vector sub_bound_args; + callable.get_bound_arguments_ref(sub_bound_args); + int sub_bound_count = sub_bound_args.size(); - if (sub_count == 0) { + int sub_unbound_count = callable.get_unbound_arguments_count(); + + if (sub_bound_count == 0 && sub_unbound_count == 0) { r_arguments = binds; - r_argcount = binds.size(); return; } - int new_count = sub_count + binds.size(); - r_argcount = new_count; + int added_count = MAX(0, binds.size() - sub_unbound_count); + int new_count = sub_bound_count + added_count; - if (new_count <= 0) { - // Removed more arguments than it adds. - r_arguments = Vector(); + if (added_count <= 0) { + // All added arguments are consumed by `sub_unbound_count`. + r_arguments = sub_bound_args; return; } r_arguments.resize(new_count); - - if (sub_count > 0) { - for (int i = 0; i < sub_count; i++) { - r_arguments.write[i] = sub_args[i]; - } - for (int i = 0; i < binds.size(); i++) { - r_arguments.write[i + sub_count] = binds[i]; - } - r_argcount = new_count; - } else { - for (int i = 0; i < binds.size() + sub_count; i++) { - r_arguments.write[i] = binds[i - sub_count]; - } + Variant *args = r_arguments.ptrw(); + for (int i = 0; i < added_count; i++) { + args[i] = binds[i]; } + for (int i = 0; i < sub_bound_count; i++) { + args[i + added_count] = sub_bound_args[i]; + } +} + +int CallableCustomBind::get_unbound_arguments_count() const { + return MAX(0, callable.get_unbound_arguments_count() - binds.size()); } void CallableCustomBind::call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, Callable::CallError &r_call_error) const { @@ -242,22 +240,15 @@ int CallableCustomUnbind::get_argument_count(bool &r_is_valid) const { } int CallableCustomUnbind::get_bound_arguments_count() const { - return callable.get_bound_arguments_count() - argcount; + return callable.get_bound_arguments_count(); } -void CallableCustomUnbind::get_bound_arguments(Vector &r_arguments, int &r_argcount) const { - Vector sub_args; - int sub_count; - callable.get_bound_arguments_ref(sub_args, sub_count); +void CallableCustomUnbind::get_bound_arguments(Vector &r_arguments) const { + callable.get_bound_arguments_ref(r_arguments); +} - r_argcount = sub_args.size() - argcount; - - if (argcount >= sub_args.size()) { - r_arguments = Vector(); - } else { - sub_args.resize(sub_args.size() - argcount); - r_arguments = sub_args; - } +int CallableCustomUnbind::get_unbound_arguments_count() const { + return callable.get_unbound_arguments_count() + argcount; } void CallableCustomUnbind::call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, Callable::CallError &r_call_error) const { diff --git a/core/variant/callable_bind.h b/core/variant/callable_bind.h index 43cebb45f01..1346277197b 100644 --- a/core/variant/callable_bind.h +++ b/core/variant/callable_bind.h @@ -55,7 +55,8 @@ public: virtual const Callable *get_base_comparator() const override; virtual int get_argument_count(bool &r_is_valid) const override; virtual int get_bound_arguments_count() const override; - virtual void get_bound_arguments(Vector &r_arguments, int &r_argcount) const override; + virtual void get_bound_arguments(Vector &r_arguments) const override; + virtual int get_unbound_arguments_count() const override; Callable get_callable() { return callable; } Vector get_binds() { return binds; } @@ -84,7 +85,8 @@ public: virtual const Callable *get_base_comparator() const override; virtual int get_argument_count(bool &r_is_valid) const override; virtual int get_bound_arguments_count() const override; - virtual void get_bound_arguments(Vector &r_arguments, int &r_argcount) const override; + virtual void get_bound_arguments(Vector &r_arguments) const override; + virtual int get_unbound_arguments_count() const override; Callable get_callable() { return callable; } int get_unbinds() { return argcount; } diff --git a/core/variant/variant.cpp b/core/variant/variant.cpp index 7550477d70e..9b317d449e4 100644 --- a/core/variant/variant.cpp +++ b/core/variant/variant.cpp @@ -3668,18 +3668,20 @@ String Variant::get_call_error_text(Object *p_base, const StringName &p_method, String Variant::get_callable_error_text(const Callable &p_callable, const Variant **p_argptrs, int p_argcount, const Callable::CallError &ce) { Vector binds; - int args_bound; - p_callable.get_bound_arguments_ref(binds, args_bound); - if (args_bound <= 0) { - return get_call_error_text(p_callable.get_object(), p_callable.get_method(), p_argptrs, MAX(0, p_argcount + args_bound), ce); + p_callable.get_bound_arguments_ref(binds); + + int args_unbound = p_callable.get_unbound_arguments_count(); + + if (p_argcount - args_unbound < 0) { + return "Callable unbinds " + itos(args_unbound) + " arguments, but called with " + itos(p_argcount); } else { Vector argptrs; - argptrs.resize(p_argcount + binds.size()); - for (int i = 0; i < p_argcount; i++) { + argptrs.resize(p_argcount - args_unbound + binds.size()); + for (int i = 0; i < p_argcount - args_unbound; i++) { argptrs.write[i] = p_argptrs[i]; } for (int i = 0; i < binds.size(); i++) { - argptrs.write[i + p_argcount] = &binds[i]; + argptrs.write[i + p_argcount - args_unbound] = &binds[i]; } return get_call_error_text(p_callable.get_object(), p_callable.get_method(), (const Variant **)argptrs.ptr(), argptrs.size(), ce); } diff --git a/core/variant/variant_call.cpp b/core/variant/variant_call.cpp index 29e11462c92..381b848b2b4 100644 --- a/core/variant/variant_call.cpp +++ b/core/variant/variant_call.cpp @@ -2116,6 +2116,7 @@ static void _register_variant_builtin_methods_misc() { bind_function(Callable, get_argument_count, _VariantCall::func_Callable_get_argument_count, sarray(), varray()); bind_method(Callable, get_bound_arguments_count, sarray(), varray()); bind_method(Callable, get_bound_arguments, sarray(), varray()); + bind_method(Callable, get_unbound_arguments_count, sarray(), varray()); bind_method(Callable, hash, sarray(), varray()); bind_method(Callable, bindv, sarray("arguments"), varray()); bind_method(Callable, unbind, sarray("argcount"), varray()); diff --git a/doc/classes/Callable.xml b/doc/classes/Callable.xml index 0c8f3c66f57..cf3c3e06fd5 100644 --- a/doc/classes/Callable.xml +++ b/doc/classes/Callable.xml @@ -155,13 +155,21 @@ - Return the bound arguments (as long as [method get_bound_arguments_count] is greater than zero), or empty (if [method get_bound_arguments_count] is less than or equal to zero). + Returns the array of arguments bound via successive [method bind] or [method unbind] calls. These arguments will be added [i]after[/i] the arguments passed to the call, from which [method get_unbound_arguments_count] arguments on the right have been previously excluded. + [codeblock] + func get_effective_arguments(callable, call_args): + assert(call_args.size() - callable.get_unbound_arguments_count() >= 0) + var result = call_args.slice(0, call_args.size() - callable.get_unbound_arguments_count()) + result.append_array(callable.get_bound_arguments()) + return result + [/codeblock] - Returns the total amount of arguments bound (or unbound) via successive [method bind] or [method unbind] calls. If the amount of arguments unbound is greater than the ones bound, this function returns a value less than zero. + Returns the total amount of arguments bound via successive [method bind] or [method unbind] calls. This is the same as the size of the array returned by [method get_bound_arguments]. See [method get_bound_arguments] for details. + [b]Note:[/b] The [method get_bound_arguments_count] and [method get_unbound_arguments_count] methods can both return positive values. @@ -182,6 +190,13 @@ Returns the ID of this [Callable]'s object (see [method Object.get_instance_id]). + + + + Returns the total amount of arguments unbound via successive [method bind] or [method unbind] calls. See [method get_bound_arguments] for details. + [b]Note:[/b] The [method get_bound_arguments_count] and [method get_unbound_arguments_count] methods can both return positive values. + + diff --git a/scene/main/node.cpp b/scene/main/node.cpp index 8dc7b4a87cc..5063f0d6d0e 100644 --- a/scene/main/node.cpp +++ b/scene/main/node.cpp @@ -3055,11 +3055,12 @@ void Node::_duplicate_signals(const Node *p_original, Node *p_copy) const { if (copy && copytarget && E.callable.get_method() != StringName()) { Callable copy_callable = Callable(copytarget, E.callable.get_method()); if (!copy->is_connected(E.signal.get_name(), copy_callable)) { - int arg_count = E.callable.get_bound_arguments_count(); - if (arg_count > 0) { + int unbound_arg_count = E.callable.get_unbound_arguments_count(); + if (unbound_arg_count > 0) { + copy_callable = copy_callable.unbind(unbound_arg_count); + } + if (E.callable.get_bound_arguments_count() > 0) { copy_callable = copy_callable.bindv(E.callable.get_bound_arguments()); - } else if (arg_count < 0) { - copy_callable = copy_callable.unbind(-arg_count); } copy->connect(E.signal.get_name(), copy_callable, E.flags); } diff --git a/tests/core/variant/test_callable.h b/tests/core/variant/test_callable.h index 3228e0a583b..34ea8fad5ca 100644 --- a/tests/core/variant/test_callable.h +++ b/tests/core/variant/test_callable.h @@ -135,6 +135,70 @@ TEST_CASE("[Callable] Argument count") { memdelete(my_test); } + +class TestBoundUnboundArgumentCount : public Object { + GDCLASS(TestBoundUnboundArgumentCount, Object); + +protected: + static void _bind_methods() { + ClassDB::bind_vararg_method(METHOD_FLAGS_DEFAULT, "test_func", &TestBoundUnboundArgumentCount::test_func, MethodInfo("test_func")); + } + +public: + Variant test_func(const Variant **p_args, int p_argcount, Callable::CallError &r_error) { + Array result; + result.resize(p_argcount); + for (int i = 0; i < p_argcount; i++) { + result[i] = *p_args[i]; + } + return result; + } + + static String get_output(const Callable &p_callable) { + Array effective_args; + effective_args.push_back(7); + effective_args.push_back(8); + effective_args.push_back(9); + + effective_args.resize(3 - p_callable.get_unbound_arguments_count()); + effective_args.append_array(p_callable.get_bound_arguments()); + + return vformat( + "%d %d %s %s %s", + p_callable.get_unbound_arguments_count(), + p_callable.get_bound_arguments_count(), + p_callable.get_bound_arguments(), + p_callable.call(7, 8, 9), + effective_args); + } +}; + +TEST_CASE("[Callable] Bound and unbound argument count") { + String (*get_output)(const Callable &) = TestBoundUnboundArgumentCount::get_output; + + TestBoundUnboundArgumentCount *test_instance = memnew(TestBoundUnboundArgumentCount); + + Callable test_func = Callable(test_instance, "test_func"); + + CHECK(get_output(test_func) == "0 0 [] [7, 8, 9] [7, 8, 9]"); + CHECK(get_output(test_func.bind(1, 2)) == "0 2 [1, 2] [7, 8, 9, 1, 2] [7, 8, 9, 1, 2]"); + CHECK(get_output(test_func.bind(1, 2).unbind(1)) == "1 2 [1, 2] [7, 8, 1, 2] [7, 8, 1, 2]"); + CHECK(get_output(test_func.bind(1, 2).unbind(1).bind(3, 4)) == "0 3 [3, 1, 2] [7, 8, 9, 3, 1, 2] [7, 8, 9, 3, 1, 2]"); + CHECK(get_output(test_func.bind(1, 2).unbind(1).bind(3, 4).unbind(1)) == "1 3 [3, 1, 2] [7, 8, 3, 1, 2] [7, 8, 3, 1, 2]"); + + CHECK(get_output(test_func.bind(1).bind(2).bind(3).unbind(1)) == "1 3 [3, 2, 1] [7, 8, 3, 2, 1] [7, 8, 3, 2, 1]"); + CHECK(get_output(test_func.bind(1).bind(2).unbind(1).bind(3)) == "0 2 [2, 1] [7, 8, 9, 2, 1] [7, 8, 9, 2, 1]"); + CHECK(get_output(test_func.bind(1).unbind(1).bind(2).bind(3)) == "0 2 [3, 1] [7, 8, 9, 3, 1] [7, 8, 9, 3, 1]"); + CHECK(get_output(test_func.unbind(1).bind(1).bind(2).bind(3)) == "0 2 [3, 2] [7, 8, 9, 3, 2] [7, 8, 9, 3, 2]"); + + CHECK(get_output(test_func.unbind(1).unbind(1).unbind(1).bind(1, 2, 3)) == "0 0 [] [7, 8, 9] [7, 8, 9]"); + CHECK(get_output(test_func.unbind(1).unbind(1).bind(1, 2, 3).unbind(1)) == "1 1 [1] [7, 8, 1] [7, 8, 1]"); + CHECK(get_output(test_func.unbind(1).bind(1, 2, 3).unbind(1).unbind(1)) == "2 2 [1, 2] [7, 1, 2] [7, 1, 2]"); + CHECK(get_output(test_func.bind(1, 2, 3).unbind(1).unbind(1).unbind(1)) == "3 3 [1, 2, 3] [1, 2, 3] [1, 2, 3]"); + + memdelete(test_instance); +} + } // namespace TestCallable #endif // TEST_CALLABLE_H