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.

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

GDScript: Fix get_*_list() methods return incorrect info #81079

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

adamscott
adamscott previously approved these changes Aug 28, 2023
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Looks fine! And even simplify a lot the code. Thanks as always, @dalexeev

@bitsawer
Copy link
Member

Do you know if this helps or affects #66218 in any way?

@dalexeev
Copy link
Member Author

Do you know if this helps or affects #66218 in any way?

In the case of non-constant expressions (as well as arrays and dictionaries), the default value is unknown. We can only insert null instead.

@bitsawer
Copy link
Member

I guess null could be a reasonable choice, at least that way it would be possible to figure out at least some default arguments. If understood that issue issue right, it's not currently possible in all cases. Then again maybe we could use some magic sentinel value instead of null, like return certain type of object etc. to mark that the value could not be used. That way it would also be possible to tell a difference between an actual null default vs non-constant expression.

Not really relevant to this PR, but thanks for answering!

@dalexeev dalexeev force-pushed the gds-fix-get-method-list branch 3 times, most recently from 89c85b2 to ea22367 Compare August 31, 2023 13:01
@dalexeev dalexeev dismissed adamscott’s stale review August 31, 2023 13:38

Too early approval.

@dalexeev dalexeev changed the title GDScript: Fix get_method_list() for custom functions GDScript: Fix get_*_list() methods return incorrect info Sep 1, 2023
@dalexeev dalexeev force-pushed the gds-fix-get-method-list branch 2 times, most recently from 343e7ad to 3a78b16 Compare September 3, 2023 14:52
@dalexeev dalexeev marked this pull request as ready for review September 3, 2023 14:53
@dalexeev dalexeev requested a review from a team as a code owner September 3, 2023 14:53
@adamscott
Copy link
Member

@dalexeev If you could comment the changes you made in this PR, as discussed in the GDScript meeting, it would be very appreciated!

@dalexeev
Copy link
Member Author

dalexeev commented Sep 4, 2023

This PR fixes GDScript "reflection" of properties (static and non-static variables, the latter can optionally be exported), methods and signals. There are many open duplicate issues, but in each of them there are some different details that I tried to cover everything.

1. Prior to this PR, there was no single method for converting a GDScript data type into a PropertyInfo that best matches it. Unfortunately, it is not always possible to ideally represent a GDScript type as a PropertyInfo (for example, in the case of non-global classes, inner classes). This is a core issue that is still needs to be unified.

The GDScriptDataType class (which represents runtime type) had an operator PropertyInfo(), but it a. was not used everywhere (for example, signals) and b. loses some information (for example, enums). This is now the to_property_info() method in the GDScriptParser::DataType class (which represents static type) and its implementation has been improved.

2. There was an issue with Object.get_*_list() and Script.get_script_*_list() methods returning different data due to duplicate implementations. Now this data is stored in GDScript (MemberInfo::property_info) and GDScriptFunction (method_info) structures. They are generated once in the compiler (in the analyzer for signals) and the GDScript/GDScriptInstance methods simply return the data, rather than generating it in different places.

3. This PR fixes the inconsistent use of the GDScript::name property in some cases as a global name (in the Script::get_global_name() override) and in others as a local (inner class identifier).

4. Minor reorganization of GDScriptFunction: dead code removed, properties and methods arranged more conveniently.

5. Fixed incorrect function parameter names for get_method_list(), argument names and default argument values are now available in release builds too.

6. Fixed bug with missing PROPERTY_USAGE_NIL_IS_VARIANT for Variant variables.

7. Fixed bug with missing METHOD_FLAG_STATIC for static functions.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

Approved by the GDScript team in meeting. Great work @dalexeev, and especially great explanation of what you did. Magnificent.

@akien-mga akien-mga merged commit 13f0ab8 into godotengine:master Sep 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment