-
-
Notifications
You must be signed in to change notification settings - Fork 18.8k
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
CSharpScript should not own method infos of the base class #77273
CSharpScript should not own method infos of the base class #77273
Conversation
It feels weird to change this loop for methods and not for the others (rpc functions and event signals). I think the proper fix would be to remove the loop from |
I suggest there to be some clarification for the meaning of Script::has_method and ScriptInstance::has_method. In current implementation, godot/modules/gdscript/gdscript.cpp Lines 357 to 359 in 809a982
godot/modules/mono/csharp_script.cpp Lines 2473 to 2485 in 809a982
godot/modules/gdscript/gdscript.cpp Lines 1757 to 1768 in 809a982
In current implementation, This PR tries to correct the contents in By removing the loop, Only remove the loop from I haven't check properties, rpc functions and signal. They might have similar issues, but I need more time to figure out. |
Emm.. I just found in editor mode, there might be no actual script instance, instead a
Sorry for my inaccurate explain in the first comment. The duplication in method picking dialogue actually comes from |
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.
I think it makes sense for CSharpScript
to only contain the members declared in the C# type that it represents, then the inherited members can be retrieved by iterating the hierarchy chain.
The same change should likely be made to the other members (properties and signals), and we must make sure the behavior matches GDScript (i.e. if GDScript::get_method_list
iterates the hierarchy chain and returns inherited methods, then CSharpScript::get_method_list
should return inherited methods too).
|
||
top = top->base_script.ptr(); | ||
} | ||
script->get_script_method_list(p_list); |
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.
I'm not sure this change is worth doing, and there might've been reasons why this code was duplicated before.
You should probably check that the script is valid, as it was done before.
script->get_script_method_list(p_list); | |
if (!script->is_valid() || !script->valid) { | |
return; | |
} | |
script->get_script_method_list(p_list); |
if (top != null && top != native) | ||
{ | ||
var methodList = GetMethodListForType(top); |
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.
If we are removing the loop, we can replace top
here with scriptType
which should never be null so no need to check.
if (top != null && top != native) | |
{ | |
var methodList = GetMethodListForType(top); | |
if (scriptType != native) | |
{ | |
var methodList = GetMethodListForType(scriptType); |
@huisedenanhai Will you be available to finish this PR based on the feedback above? It's not in scope for 4.2 at this point, but we could merge it as soon as it's fixed up and the consensus is reached. |
|
CSharpScript::methods should only hold methods defined by current class.
Previous implementation stores methods defined by current class and all base classes, thus
result returned by CSharpInstance::get_method_list (which gather all method infos from current to base classes) contains duplicated method infos of base class.
The duplication can be visualized in method picking dialogue of signal connection dialogue panel.
For example, there are two C# scripts
Attach the
Test
class to a node, and try to connect a signal to it. In method picking dialogue, theFoo
method defined byTestBase
is duplicated twice.This pr fix the duplication issue. The behaviour of methods like
CSharpScript::has_method
,CSharpInstance::get_method_list
are consistent with the corresponding methods of GDScript.