Allow ScriptExtension implementations to override callp#118595
Allow ScriptExtension implementations to override callp#118595Naros wants to merge 1 commit intogodotengine:masterfrom
Conversation
|
@Ivorforce @dsnopek here's my first pass at the implementation from today's GDExtension call. I used the Dictionary approach as we had discussed, and chose to handle it by using the following mapping:
Let me know what you think. |
7b5440f to
7a17cd2
Compare
7a17cd2 to
4bbc139
Compare
| if (ret.has("result")) { | ||
| Variant result = ret["result"]; | ||
| if (ret.has("error")) { | ||
| Dictionary err = ret["error"]; |
There was a problem hiding this comment.
So, the structure is:
{
'value': <variant>,
'error': {
'error': 0, // CALL_OK
'argument': 1, // just example values...
'expected': 1,
}
}
I'm not sure about having the nested Dictionary. Is there any "prior art" for this layout?
Alternatively, we could do this with a flat structure like:
{
'value': <variant>,
'error': 0, // CALL_OK
'argument': 1, // just example values...
'expected': 1,
}
There was a problem hiding this comment.
I looked across the code base, and there isn't any precedent for returning the CallError this way, so I can certainly make it a flat structure; that's not an issue.
4bbc139 to
bc114c5
Compare
|
|
||
| Dictionary ret; | ||
| if (GDVIRTUAL_CALL(_callp, p_method, args, ret)) { | ||
| if (ret.has("result")) { |
There was a problem hiding this comment.
So, if the dictionary doesn't have a "result" key, then this falls back on Script::callp(). Is that what we want?
I could see an argument that this should could as either (a) CALL_OK for a static method with no return value, or (b) an error because we're requiring all GDExtensions to return a "result" here
There was a problem hiding this comment.
My reasoning here is that normally I would implement _callp like this:
Variant OScript::_callp_internal(const StringName& p_method, const Variant** p_args, int p_argcount, GDExtensionCallError& r_error) {
HashMap<StringName, OScriptFunc*>::ConstIterator E = _member_functions.find(p_method);
if (E) {
ERR_FAIL_COND_V_MSG(!E->value->is_static(), Variant(), "Cannot call non-static function '" + p_method + "' in script.");
return E->value->call(nullptr, p_args, p_argcount, r_error);
}
if (native.is_valid()) {
return native->callp(p_method, p_args, p_argcount, r_error);
}
r_error.error = GDEXTENSION_CALL_ERROR_INVALID_METHOD;
return Variant();
}
Dictionary OScript::_callp(const StringName& p_method, const Array& p_args) {
Vector<const Variant*> argptrs;
argptrs.resize(p_args.size());
for (int i = 0; i < p_args.size(); i++) {
argptrs.write[i] = &p_args[i];
}
Dictionary ret;
GDExtensionCallError r_error;
ret["result"] = _callp_internal(p_method, argsptr, argptrs.size(), r_error);
if (r_error.error != GDEXTENSION_CALL_OK) {
ret["error"] = r_error.error;
ret["expected"] = r_error.expected;
ret["argument"] = r_error.argument;
}
return ret;
}The compiled function always returns an uninitialized Variant if the script function is void or fails with an error, and I get an initialized Variant if the function call succeeds, because to the VM it does not care whether the function's entry call is or is not static.
So my assumption here is I pass "result" and the optional "error" all the way back up the chain from the VM output. This way, for certain Godot functions that have a precedent for returning a String as the "result" to provide an error reason, while setting "error" to the generic error code, this avoids my _callp override from having to check this use case.
While I could do this, I don't like how it looks and I think passing result up if the VM was called logically makes more functional sense from a Godot PoV.
Variant result = _callp_internal(...);
if (result != Variant()) {
ret["result"] = result;
}I can see both ways, so I guess it's up to what everyone prefers as the "Godot way".
I left an empty Dictionary as a way for implementations, if they really want it to delegate up to Object, they have that option as well.
Fixes godotengine/godot-proposals#14614
Description
Introduces a virtual override handler on
ScriptExtensionfor hierarchy calls viacallp, delegating them to the underlying script implementation and enabling GDExtension-based scripting languages to handle calls from GDScript or CSharp that invoke statically declared script functions.