Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[3.x] Increase VARIANT_ARG_MAX to 8 #54188

Merged
merged 1 commit into from
Mar 8, 2022
Merged

[3.x] Increase VARIANT_ARG_MAX to 8 #54188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions core/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@

#include <atomic>

#define VARIANT_ARG_LIST const Variant &p_arg1 = Variant(), const Variant &p_arg2 = Variant(), const Variant &p_arg3 = Variant(), const Variant &p_arg4 = Variant(), const Variant &p_arg5 = Variant()
#define VARIANT_ARG_PASS p_arg1, p_arg2, p_arg3, p_arg4, p_arg5
#define VARIANT_ARG_DECLARE const Variant &p_arg1, const Variant &p_arg2, const Variant &p_arg3, const Variant &p_arg4, const Variant &p_arg5
#define VARIANT_ARG_MAX 5
#define VARIANT_ARGPTRS const Variant *argptr[5] = { &p_arg1, &p_arg2, &p_arg3, &p_arg4, &p_arg5 };
#define VARIANT_ARGPTRS_PASS *argptr[0], *argptr[1], *argptr[2], *argptr[3], *argptr[4]
#define VARIANT_ARGS_FROM_ARRAY(m_arr) m_arr[0], m_arr[1], m_arr[2], m_arr[3], m_arr[4]
#define VARIANT_ARG_LIST const Variant &p_arg1 = Variant(), const Variant &p_arg2 = Variant(), const Variant &p_arg3 = Variant(), const Variant &p_arg4 = Variant(), const Variant &p_arg5 = Variant(), const Variant &p_arg6 = Variant(), const Variant &p_arg7 = Variant(), const Variant &p_arg8 = Variant()
#define VARIANT_ARG_PASS p_arg1, p_arg2, p_arg3, p_arg4, p_arg5, p_arg6, p_arg7, p_arg8
#define VARIANT_ARG_DECLARE const Variant &p_arg1, const Variant &p_arg2, const Variant &p_arg3, const Variant &p_arg4, const Variant &p_arg5, const Variant &p_arg6, const Variant &p_arg7, const Variant &p_arg8
#define VARIANT_ARG_MAX 8
#define VARIANT_ARGPTRS const Variant *argptr[8] = { &p_arg1, &p_arg2, &p_arg3, &p_arg4, &p_arg5, &p_arg6, &p_arg7, &p_arg8 };
#define VARIANT_ARGPTRS_PASS *argptr[0], *argptr[1], *argptr[2], *argptr[3], *argptr[4], *argptr[5], *argptr[6], *argptr[7]
#define VARIANT_ARGS_FROM_ARRAY(m_arr) m_arr[0], m_arr[1], m_arr[2], m_arr[3], m_arr[4], m_arr[5], m_arr[6], m_arr[7]

/**
@author Juan Linietsky <reduzio@gmail.com>
Expand Down
6 changes: 4 additions & 2 deletions core/undo_redo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,8 @@ Variant UndoRedo::_add_do_method(const Variant **p_args, int p_argcount, Variant
v[i] = *p_args[i + 2];
}

add_do_method(object, method, v[0], v[1], v[2], v[3], v[4]);
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
add_do_method(object, method, v[0], v[1], v[2], v[3], v[4], v[5], v[6], v[7]);
return Variant();
}

Expand Down Expand Up @@ -492,7 +493,8 @@ Variant UndoRedo::_add_undo_method(const Variant **p_args, int p_argcount, Varia
v[i] = *p_args[i + 2];
}

add_undo_method(object, method, v[0], v[1], v[2], v[3], v[4]);
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
add_undo_method(object, method, v[0], v[1], v[2], v[3], v[4], v[5], v[6], v[7]);
return Variant();
}

Expand Down
2 changes: 1 addition & 1 deletion core/variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ class Variant {

void call_ptr(const StringName &p_method, const Variant **p_args, int p_argcount, Variant *r_ret, CallError &r_error);
Variant call(const StringName &p_method, const Variant **p_args, int p_argcount, CallError &r_error);
Variant call(const StringName &p_method, const Variant &p_arg1 = Variant(), const Variant &p_arg2 = Variant(), const Variant &p_arg3 = Variant(), const Variant &p_arg4 = Variant(), const Variant &p_arg5 = Variant());
Variant call(const StringName &p_method, const Variant &p_arg1 = Variant(), const Variant &p_arg2 = Variant(), const Variant &p_arg3 = Variant(), const Variant &p_arg4 = Variant(), const Variant &p_arg5 = Variant(), const Variant &p_arg6 = Variant(), const Variant &p_arg7 = Variant(), const Variant &p_arg8 = Variant());

static String get_call_error_text(Object *p_base, const StringName &p_method, const Variant **p_argptrs, int p_argcount, const Variant::CallError &ce);

Expand Down
6 changes: 6 additions & 0 deletions doc/classes/Tween.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@
<argument index="5" name="arg3" type="Variant" default="null" />
<argument index="6" name="arg4" type="Variant" default="null" />
<argument index="7" name="arg5" type="Variant" default="null" />
<argument index="8" name="arg6" type="Variant" default="null" />
<argument index="9" name="arg7" type="Variant" default="null" />
<argument index="10" name="arg8" type="Variant" default="null" />
<description>
Calls [code]callback[/code] of [code]object[/code] after [code]duration[/code]. [code]arg1[/code]-[code]arg5[/code] are arguments to be passed to the callback.
</description>
Expand All @@ -83,6 +86,9 @@
<argument index="5" name="arg3" type="Variant" default="null" />
<argument index="6" name="arg4" type="Variant" default="null" />
<argument index="7" name="arg5" type="Variant" default="null" />
<argument index="8" name="arg6" type="Variant" default="null" />
<argument index="9" name="arg7" type="Variant" default="null" />
<argument index="10" name="arg8" type="Variant" default="null" />
<description>
Calls [code]callback[/code] of [code]object[/code] after [code]duration[/code] on the main thread (similar to [method Object.call_deferred]). [code]arg1[/code]-[code]arg5[/code] are arguments to be passed to the callback.
</description>
Expand Down
6 changes: 4 additions & 2 deletions editor/animation_track_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,8 @@ class AnimationTrackKeyEdit : public Object {
} break;
case Animation::TYPE_METHOD: {
p_list->push_back(PropertyInfo(Variant::STRING, "name"));
p_list->push_back(PropertyInfo(Variant::INT, "arg_count", PROPERTY_HINT_RANGE, "0,5,1"));
static_assert(VARIANT_ARG_MAX == 8, "PROPERTY_HINT_RANGE needs to be updated if VARIANT_ARG_MAX != 8");
p_list->push_back(PropertyInfo(Variant::INT, "arg_count", PROPERTY_HINT_RANGE, "0,8,1"));
Comment on lines +559 to +560
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in a PR review meeting and we're not sure it's worth changing the whole VARIANT_ARG_MAX just for this - the limit on animation code doesn't seem related to VARIANT_ARG_MAX and could likely be lifted to allow any number of arguments, no?

Copy link
Member Author

@Rubonnek Rubonnek Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this code the user is modifying a key within a method-type track of an Animation, right? The number of arguments for a method-type track in an Animation is bound by the maximum number of arguments the AnimationPlayer itself supports when playing this track. VARIANT_ARG_MAX is related. I fail to see how it could not. Keep an eye on Animation::TYPE_METHOD.

When the AnimationPlayer attempts to play the method-type track, it will run the following code (only pay attention to the first line, the last line, and params near the bottom):

godot/scene/animation/animation_player.cpp

Lines 527 to 548 in b197de6

case Animation::TYPE_METHOD: {
if (!nc->node) {
continue;
}
if (p_delta == 0) {
continue;
}
if (!p_is_current) {
break;
}
List<int> indices;
a->method_track_get_key_indices(i, p_time, p_delta, &indices);
for (List<int>::Element *E = indices.front(); E; E = E->next()) {
StringName method = a->method_track_get_name(i, E->get());
Vector<Variant> params = a->method_track_get_params(i, E->get());
int s = params.size();
ERR_CONTINUE(s > VARIANT_ARG_MAX);

The very last line of that code block has a direct reference to VARIANT_ARG_MAX. That check is there because further below AnimationPlayer reconstructs the method call extracted from the track:

godot/scene/animation/animation_player.cpp

Lines 556 to 573 in b197de6

if (method_call_mode == ANIMATION_METHOD_CALL_DEFERRED) {
MessageQueue::get_singleton()->push_call(
nc->node,
method,
s >= 1 ? params[0] : Variant(),
s >= 2 ? params[1] : Variant(),
s >= 3 ? params[2] : Variant(),
s >= 4 ? params[3] : Variant(),
s >= 5 ? params[4] : Variant());
} else {
nc->node->call(
method,
s >= 1 ? params[0] : Variant(),
s >= 2 ? params[1] : Variant(),
s >= 3 ? params[2] : Variant(),
s >= 4 ? params[3] : Variant(),
s >= 5 ? params[4] : Variant());
}

The params array holds the number of arguments previously saved on the track through the AnimationTrackEditor. Everytime VARIANT_ARG_MAX is updated, that last code block needs to be updated to support the maximum number of arguments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akien-mga is there anything holding back this PR?

For what is worth, sure, we could remove the limit on this referenced code but that doesn't mean the AnimationPlayer can execute the saved tracks -- that's what ties VARIANT_ARG_MAX to the code you've referenced.


Dictionary d = animation->track_get_key_value(track, key);
ERR_FAIL_COND(!d.has("args"));
Expand Down Expand Up @@ -1205,7 +1206,8 @@ class AnimationMultiTrackKeyEdit : public Object {
} break;
case Animation::TYPE_METHOD: {
p_list->push_back(PropertyInfo(Variant::STRING, "name"));
p_list->push_back(PropertyInfo(Variant::INT, "arg_count", PROPERTY_HINT_RANGE, "0,5,1"));
static_assert(VARIANT_ARG_MAX == 8, "PROPERTY_HINT_RANGE needs to be updated if VARIANT_ARG_MAX != 8");
p_list->push_back(PropertyInfo(Variant::INT, "arg_count", PROPERTY_HINT_RANGE, "0,8,1"));

Dictionary d = animation->track_get_key_value(first_track, first_key);
ERR_FAIL_COND(!d.has("args"));
Expand Down
3 changes: 2 additions & 1 deletion platform/android/java_godot_lib_jni.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,8 @@ JNIEXPORT void JNICALL Java_org_godotengine_godot_GodotLib_calldeferred(JNIEnv *
env->DeleteLocalRef(obj);
};

obj->call_deferred(str_method, args[0], args[1], args[2], args[3], args[4]);
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
obj->call_deferred(str_method, args[0], args[1], args[2], args[3], args[4], args[5], args[6], args[7]);
// something
env->PopLocalFrame(NULL);
}
Expand Down
11 changes: 9 additions & 2 deletions scene/animation/animation_player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,7 @@ void AnimationPlayer::_animation_process_animation(AnimationData *p_anim, float
}
#endif

static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
if (can_call) {
if (method_call_mode == ANIMATION_METHOD_CALL_DEFERRED) {
MessageQueue::get_singleton()->push_call(
Expand All @@ -561,15 +562,21 @@ void AnimationPlayer::_animation_process_animation(AnimationData *p_anim, float
s >= 2 ? params[1] : Variant(),
s >= 3 ? params[2] : Variant(),
s >= 4 ? params[3] : Variant(),
s >= 5 ? params[4] : Variant());
s >= 5 ? params[4] : Variant(),
s >= 6 ? params[5] : Variant(),
s >= 7 ? params[6] : Variant(),
s >= 8 ? params[7] : Variant());
} else {
nc->node->call(
method,
s >= 1 ? params[0] : Variant(),
s >= 2 ? params[1] : Variant(),
s >= 3 ? params[2] : Variant(),
s >= 4 ? params[3] : Variant(),
s >= 5 ? params[4] : Variant());
s >= 5 ? params[4] : Variant(),
s >= 6 ? params[5] : Variant(),
s >= 7 ? params[6] : Variant(),
s >= 8 ? params[7] : Variant());
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion scene/animation/animation_tree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,7 @@ void AnimationTree::_process_graph(float p_delta) {

int s = params.size();

static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
ERR_CONTINUE(s > VARIANT_ARG_MAX);
if (can_call) {
t->object->call_deferred(
Expand All @@ -989,7 +990,10 @@ void AnimationTree::_process_graph(float p_delta) {
s >= 2 ? params[1] : Variant(),
s >= 3 ? params[2] : Variant(),
s >= 4 ? params[3] : Variant(),
s >= 5 ? params[4] : Variant());
s >= 5 ? params[4] : Variant(),
s >= 6 ? params[5] : Variant(),
s >= 7 ? params[6] : Variant(),
s >= 8 ? params[7] : Variant());
}
}

Expand Down
32 changes: 28 additions & 4 deletions scene/animation/tween.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ void Tween::_bind_methods() {
// Bind interpolation and follow methods
ClassDB::bind_method(D_METHOD("interpolate_property", "object", "property", "initial_val", "final_val", "duration", "trans_type", "ease_type", "delay"), &Tween::interpolate_property, DEFVAL(TRANS_LINEAR), DEFVAL(EASE_IN_OUT), DEFVAL(0));
ClassDB::bind_method(D_METHOD("interpolate_method", "object", "method", "initial_val", "final_val", "duration", "trans_type", "ease_type", "delay"), &Tween::interpolate_method, DEFVAL(TRANS_LINEAR), DEFVAL(EASE_IN_OUT), DEFVAL(0));
ClassDB::bind_method(D_METHOD("interpolate_callback", "object", "duration", "callback", "arg1", "arg2", "arg3", "arg4", "arg5"), &Tween::interpolate_callback, DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()));
ClassDB::bind_method(D_METHOD("interpolate_deferred_callback", "object", "duration", "callback", "arg1", "arg2", "arg3", "arg4", "arg5"), &Tween::interpolate_deferred_callback, DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()));
ClassDB::bind_method(D_METHOD("interpolate_callback", "object", "duration", "callback", "arg1", "arg2", "arg3", "arg4", "arg5", "arg6", "arg7", "arg8"), &Tween::interpolate_callback, DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()));
ClassDB::bind_method(D_METHOD("interpolate_deferred_callback", "object", "duration", "callback", "arg1", "arg2", "arg3", "arg4", "arg5", "arg6", "arg7", "arg8"), &Tween::interpolate_deferred_callback, DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()), DEFVAL(Variant()));
ClassDB::bind_method(D_METHOD("follow_property", "object", "property", "initial_val", "target", "target_property", "duration", "trans_type", "ease_type", "delay"), &Tween::follow_property, DEFVAL(TRANS_LINEAR), DEFVAL(EASE_IN_OUT), DEFVAL(0));
ClassDB::bind_method(D_METHOD("follow_method", "object", "method", "initial_val", "target", "target_method", "duration", "trans_type", "ease_type", "delay"), &Tween::follow_method, DEFVAL(TRANS_LINEAR), DEFVAL(EASE_IN_OUT), DEFVAL(0));
ClassDB::bind_method(D_METHOD("targeting_property", "object", "property", "initial", "initial_val", "final_val", "duration", "trans_type", "ease_type", "delay"), &Tween::targeting_property, DEFVAL(TRANS_LINEAR), DEFVAL(EASE_IN_OUT), DEFVAL(0));
Expand Down Expand Up @@ -744,6 +744,8 @@ void Tween::_tween_process(float p_delta) {
if (data.type == INTER_CALLBACK) {
// Is the tween completed?
if (data.finish) {
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");

// Are we calling this callback deferred or immediately?
if (data.call_deferred) {
// Run the deferred function callback, applying the correct number of arguments
Expand All @@ -766,16 +768,28 @@ void Tween::_tween_process(float p_delta) {
case 5:
object->call_deferred(data.key[0], data.arg[0], data.arg[1], data.arg[2], data.arg[3], data.arg[4]);
break;
case 6:
object->call_deferred(data.key[0], data.arg[0], data.arg[1], data.arg[2], data.arg[3], data.arg[4], data.arg[5]);
break;
case 7:
object->call_deferred(data.key[0], data.arg[0], data.arg[1], data.arg[2], data.arg[3], data.arg[4], data.arg[5], data.arg[6]);
break;
case 8:
object->call_deferred(data.key[0], data.arg[0], data.arg[1], data.arg[2], data.arg[3], data.arg[4], data.arg[5], data.arg[6], data.arg[7]);
break;
}
} else {
// Call the function directly with the arguments
Variant::CallError error;
Variant *arg[5] = {
Variant *arg[VARIANT_ARG_MAX] = {
&data.arg[0],
&data.arg[1],
&data.arg[2],
&data.arg[3],
&data.arg[4],
&data.arg[5],
&data.arg[6],
&data.arg[7],
};
object->call(data.key[0], (const Variant **)arg, data.args, error);
}
Expand Down Expand Up @@ -1520,8 +1534,15 @@ bool Tween::interpolate_deferred_callback(Object *p_object, real_t p_duration, S
data.delay = 0;

// Collect arguments for the callback
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
int args = 0;
if (p_arg5.get_type() != Variant::NIL) {
if (p_arg8.get_type() != Variant::NIL) {
args = 8;
} else if (p_arg7.get_type() != Variant::NIL) {
args = 7;
} else if (p_arg6.get_type() != Variant::NIL) {
args = 6;
} else if (p_arg5.get_type() != Variant::NIL) {
args = 5;
} else if (p_arg4.get_type() != Variant::NIL) {
args = 4;
Expand All @@ -1541,6 +1562,9 @@ bool Tween::interpolate_deferred_callback(Object *p_object, real_t p_duration, S
data.arg[2] = p_arg3;
data.arg[3] = p_arg4;
data.arg[4] = p_arg5;
data.arg[5] = p_arg6;
data.arg[6] = p_arg7;
data.arg[7] = p_arg8;

// Add the new interpolation
_push_interpolate_data(data);
Expand Down
2 changes: 1 addition & 1 deletion scene/animation/tween.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class Tween : public Node {
EaseType ease_type;
real_t delay;
int args;
Variant arg[5];
Variant arg[VARIANT_ARG_MAX];
int uid;
InterpolateData() {
active = false;
Expand Down
4 changes: 2 additions & 2 deletions scene/debugger/script_debugger_remote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,10 +517,10 @@ bool ScriptDebuggerRemote::_parse_live_edit(const Array &p_command) {
scene_tree->_live_edit_res_set_func(p_command[1], p_command[2], p_command[3]);

} else if (cmdstr == "live_node_call") {
scene_tree->_live_edit_node_call_func(p_command[1], p_command[2], p_command[3], p_command[4], p_command[5], p_command[6], p_command[7]);
scene_tree->_live_edit_node_call_func(p_command[1], p_command[2], p_command[3], p_command[4], p_command[5], p_command[6], p_command[7], p_command[8], p_command[9], p_command[10]);

} else if (cmdstr == "live_res_call") {
scene_tree->_live_edit_res_call_func(p_command[1], p_command[2], p_command[3], p_command[4], p_command[5], p_command[6], p_command[7]);
scene_tree->_live_edit_res_call_func(p_command[1], p_command[2], p_command[3], p_command[4], p_command[5], p_command[6], p_command[7], p_command[8], p_command[9], p_command[10]);

} else if (cmdstr == "live_create_node") {
scene_tree->_live_edit_create_node_func(p_command[1], p_command[2], p_command[3]);
Expand Down
8 changes: 5 additions & 3 deletions scene/main/scene_tree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ void SceneTree::_flush_ugc() {
v[i] = E->get()[i];
}

call_group_flags(GROUP_CALL_REALTIME, E->key().group, E->key().call, v[0], v[1], v[2], v[3], v[4]);
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
call_group_flags(GROUP_CALL_REALTIME, E->key().group, E->key().call, v[0], v[1], v[2], v[3], v[4], v[5], v[6], v[7]);

unique_group_calls.erase(E);
}
Expand Down Expand Up @@ -1021,11 +1022,12 @@ Variant SceneTree::_call_group_flags(const Variant **p_args, int p_argcount, Var
StringName method = *p_args[2];
Variant v[VARIANT_ARG_MAX];

for (int i = 0; i < MIN(p_argcount - 3, 5); i++) {
for (int i = 0; i < MIN(p_argcount - 3, VARIANT_ARG_MAX); i++) {
v[i] = *p_args[i + 3];
}

call_group_flags(flags, group, method, v[0], v[1], v[2], v[3], v[4]);
static_assert(VARIANT_ARG_MAX == 8, "This code needs to be updated if VARIANT_ARG_MAX != 8");
call_group_flags(flags, group, method, v[0], v[1], v[2], v[3], v[4], v[5], v[6], v[7]);
return Variant();
}

Expand Down