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

Add static method support to core Variant types #46378

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Add static method support to core Variant types #46378

merged 1 commit into from
Mar 16, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Feb 24, 2021 •
edited by vnen
Loading

  • Properly exposed, including validated and variant call
  • Bound static functions in String and Color
  • Did not add support for scripting languages or GDNative, will have to be added manually by @vnen

image

@reduz reduz requested a review from a team as a code owner February 24, 2021 14:00
reduz
reduz commented Feb 24, 2021
View reviewed changes
editor/doc_tools.cpp
@@ -394,13 +394,22 @@ void DocTools::generate(bool p_basic_types) {
method.qualifiers += " ";
}
method.qualifiers += "const";
} else if (E->get().flags & METHOD_FLAG_VARARG) {
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed wrong somehow, not quite sure why it was like this, so I changed it.

@Calinou Calinou added this to the 4.0 milestone Feb 24, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Feb 24, 2021

Does it support non-variant classes like e.g. File?

@reduz
Copy link
Member Author

reduz commented Feb 24, 2021

@KoBeWi not for now, maybe in a future one. One thing at a time.

@akien-mga
Copy link
Member

This error is shown when starting the project manager or editing any project:

ERROR: Condition "!imi.is_vararg && imi.argument_count != imi.argument_names.size()" is true.
   at: register_builtin_method (core/variant/variant_call.cpp:691)

Otherwise seems fine. A description will need to be added for the new static qualifier in:

godot/doc/tools/makerst.py

Lines 1050 to 1056 in 369dffc

return (
".. |virtual| replace:: :abbr:`virtual (This method should typically be overridden by the user to have any effect.)`\n"
".. |const| replace:: :abbr:`const (This method has no side effects. It doesn't modify any of the instance's member variables.)`\n"
".. |vararg| replace:: :abbr:`vararg (This method accepts any number of arguments after the ones described here.)`\n"
".. |constructor| replace:: :abbr:`constructor (This method is used to construct a type.)`\n"
".. |operator| replace:: :abbr:`operator (This method describes a valid operator to use with this type as left-hand operand.)`\n"
)

(As a side note, it could be nice to have those abbr tooltips in the in-editor docs too @Calinou.)

@Calinou
Copy link
Member

Calinou commented Feb 25, 2021 •
edited
Loading

(As a side note, it could be nice to have those abbr tooltips in the in-editor docs too @Calinou.)

Tooltips in the editor help require godotengine/godot-proposals#1285 to be implemented first.

@vnen
Copy link
Member

vnen commented Feb 25, 2021

This error is shown when starting the project manager or editing any project:

ERROR: Condition "!imi.is_vararg && imi.argument_count != imi.argument_names.size()" is true.
   at: register_builtin_method (core/variant/variant_call.cpp:691)

This is because of String::num. It is bound with 3 arguments but the actual function only has 2.

vnen
vnen reviewed Feb 28, 2021
View reviewed changes
core/variant/variant.h Outdated
static bool is_builtin_method_vararg(Variant::Type p_type, const StringName &p_method);
static void get_builtin_method_list(Variant::Type p_type, List<StringName> *p_list);
static int get_builtin_method_count(Variant::Type p_type);

void call(const StringName &p_method, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::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());

void call_static(Variant::Type p_type, const StringName &p_method, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be static?

@vnen vnen mentioned this pull request Feb 28, 2021
Merged
@vnen
Copy link
Member

vnen commented Feb 28, 2021 •
edited
Loading

Also (not sure if you want to change this here) Color::from_hsv() should be made static too.

@vnen vnen force-pushed the static-method-in-variant-types branch from 2172798 to d734667 Compare March 16, 2021 13:40
@vnen
Copy link
Member

vnen commented Mar 16, 2021

I updated this to make the call_static function static and fixed the binding of String::num to match the signature.

For the Color::from_hsv and other instances (like whether we want the String::num_int64 which allows to set the base) can be left for other PRs.

@reduz
Add static method support to core Variant types
ecfa570 
* Properly exposed, including validated and variant call
* Bound static functions in String and Color
* Did not add support for scripting languages, will have to be added manually.
@vnen vnen force-pushed the static-method-in-variant-types branch from d734667 to ecfa570 Compare March 16, 2021 13:53
@akien-mga akien-mga merged commit 224f5ca into godotengine:master Mar 16, 2021
@akien-mga
Copy link
Member

Thanks!

@vnen vnen mentioned this pull request Mar 16, 2021
Closed
4 tasks
@KoBeWi KoBeWi mentioned this pull request Apr 27, 2021
Closed
@raulsntos raulsntos mentioned this pull request Aug 10, 2022
Closed
@dalexeev dalexeev mentioned this pull request Oct 21, 2022
Merged
@raulsntos raulsntos mentioned this pull request Dec 10, 2022
Merged
@dalexeev dalexeev mentioned this pull request Aug 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnen vnen vnen left review comments

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@reduz @KoBeWi @akien-mga @Calinou @vnen