-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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.
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
Use custom callables for GDScript static methods #82755
Use custom callables for GDScript static methods #82755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There are no tests.
- See also Core: Fix
Object::has_method()
for script static methods #82767. Let's wait for a decision before updating this PR.
@@ -886,7 +887,7 @@ bool GDScript::_get(const StringName &p_name, Variant &r_ret) const { | |||
if (top->rpc_config.has(p_name)) { | |||
r_ret = Callable(memnew(GDScriptRPCCallable(const_cast<GDScript *>(top), E->key))); | |||
} else { | |||
r_ret = Callable(const_cast<GDScript *>(top), E->key); | |||
r_ret = Callable(memnew(GDScriptStaticCallable(const_cast<GDScript *>(top), E->key))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not the only place. You can also access a static method via an instance.
godot/modules/gdscript/gdscript.cpp
Lines 1615 to 1623 in f3566bc
HashMap<StringName, GDScriptFunction *>::ConstIterator E = sptr->member_functions.find(p_name); | |
if (E) { | |
if (sptr->rpc_config.has(p_name)) { | |
r_ret = Callable(memnew(GDScriptRPCCallable(this->owner, E->key))); | |
} else { | |
r_ret = Callable(this->owner, E->key); | |
} | |
return true; | |
} |
But according to the issue this already works. I find it a little weird that in this implementation, is_custom()
and is_standard()
will return different values for static_func
and Test.static_func
.
|
||
class GDScript; | ||
|
||
class GDScriptStaticCallable : public CallableCustom { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worth adding a comment that this wrapper was added only as a workaround to the issue described in #79521 (comment).
|
||
class GDScriptStaticCallable : public CallableCustom { | ||
Ref<GDScript> script; | ||
StringName method; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of StringName
, you can use a GDScriptFunction
pointer here.
StringName method; | |
GDScriptFunction *function = nullptr; |
Although your approach with StringName
is safer.
} | ||
|
||
void GDScriptStaticCallable::call(const Variant **p_arguments, int p_argcount, Variant &r_return_value, Callable::CallError &r_call_error) const { | ||
r_return_value = script->callp(method, p_arguments, p_argcount, r_call_error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r_return_value = script->callp(method, p_arguments, p_argcount, r_call_error); | |
r_return_value = function->call(nullptr, p_arguments, p_argcount, r_call_error); |
Also needs script.is_valid()
check and hot reloading implementation, see #81628. Your approach with StringName
is safer.
} | ||
|
||
String GDScriptStaticCallable::get_as_text() const { | ||
String class_name = script->get_class(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs script.is_valid()
check.
} | ||
|
||
ObjectID GDScriptStaticCallable::get_object() const { | ||
return script->get_instance_id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs script.is_valid()
check.
Superseded by #82767. |
Fixes #79521
Needed to unblock #82695