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: Add @export_custom annotation #72912

Merged

Conversation

vnen
Copy link
Member

@vnen vnen commented Feb 8, 2023

Allows setting any arbitrary hint and hint string. Useful for more complex hints or potential future hints not available as a dedicated annotation.

Closes #61325
Supersedes #65935

Relates to #82122

Production edit: added relates to #82122

@vnen vnen added this to the 4.0 milestone Feb 8, 2023
@vnen vnen requested a review from a team as a code owner February 8, 2023 19:19
@dalexeev
Copy link
Member

dalexeev commented Feb 9, 2023

Looks good. But, it seems to me that {substitutions} in Godot are more common than %SUBSTITUTIONS%. The property hint syntax does not use either % or {}.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 9, 2023
@vnen
Copy link
Member Author

vnen commented Feb 9, 2023

I'll leave this on hold for now since it won't be merged for 4.0. I'm open to suggestions about the substitution syntax and what values are needed.

@nathanfranke
Copy link
Contributor

I do prefer {type_value} or {TYPE_VALUE}, but I'm more curious about example values for each substitution and how they might be used in the hint string.

@vnen
Copy link
Member Author

vnen commented Feb 24, 2023

At first I was thinking on some array hints that require the element type on the hint string. But later I reconsidered as that might not really come from the variable type (unless there's a substitution for the element type).

I'll probably remove the substitutions once I revisit this. This also comes because IINM the % formatting operator is not reduced at compile time and thus cannot be used in annotations, so maybe solving that is a better option.

@ajreckof
Copy link
Member

ajreckof commented Feb 25, 2023

Just discovered this PR and felt stupid. As this was possible in 3.x and not in 4.x i thought it was a choice to reduce what was possible to do with exports. So I had made my own solution that i have been maintaining. I cleaned it a bit and published it here.
The major difference between what i did and this PR is that instead of a new export annotation i added a complementary annotation that is added between any export annotation and its variable and that enables the user to modify the type hint and the hint string.

For reference I was using it to create Inspector plugins as it is easy to set up an inspector plugin with it and it makes really clear code on the script side

@dalexeev
Copy link
Member

Postponed to 4.3 due to feature freeze.

@vnen vnen force-pushed the gdscript-export-custom-annotation branch from 80aeb3c to 5e248a9 Compare December 13, 2023 17:48
@vnen
Copy link
Member Author

vnen commented Dec 15, 2023

I rebased this and removed the template substitutions. If they're needed we can find a good way to add them later.

@YuriSizov
Copy link
Contributor

Should we allow specifying usage as well? Everything else from property's metadata can be defined by other means (name, type, class), but usage cannot.

@vnen
Copy link
Member Author

vnen commented Jan 2, 2024

I guess it does make sense to specify the usage too.

@akien-mga akien-mga changed the title GDScript: Add @export_custom annotation GDScript: Add @export_custom annotation Jan 9, 2024
@akien-mga
Copy link
Member

I think it's a nice addition. I also support the idea to allow specifying the usage flags.

@dalexeev
Copy link
Member

dalexeev commented Jan 9, 2024

Note that users may accidentally remove PROPERTY_USAGE_SCRIPT_VARIABLE, PROPERTY_USAGE_NIL_IS_VARIANT, PROPERTY_USAGE_CLASS_IS_ENUM, and other flags. Of course, users can already do this in _get_property_list() for "virtual" properties or in _validate_property() for existing variables. But the question arises: should annotation flags be added to existing ones (|=) or should they overwrite the initial ones (=)?

@YuriSizov
Copy link
Contributor

@dalexeev I think they should behave exactly as properties added dynamically with _get_property_list. I would not want any hidden behavior. As far as footguns go, this is not a severe problem and it can be explained in the docs for the annotation.

@YuriSizov
Copy link
Contributor

@vnen Do you plan to work on this soon? 🙃

@vnen vnen force-pushed the gdscript-export-custom-annotation branch from 5e248a9 to 298087a Compare January 22, 2024 19:22
@vnen
Copy link
Member Author

vnen commented Jan 22, 2024

Added the usage flags.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me. Needs a rebase.

modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved
Allows setting any arbitrary hint, hint string, and usage flags.
Useful for more complex hints or potential future hints not
available as a dedicated annotation.
@vnen vnen force-pushed the gdscript-export-custom-annotation branch from 298087a to 8e52045 Compare March 7, 2024 13:56
@vnen
Copy link
Member Author

vnen commented Mar 7, 2024

Updated to apply the suggestions and fix conflicts.

@akien-mga akien-mga merged commit 5724949 into godotengine:master Mar 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@caimantilla
Copy link

Is there any reason not to allow overriding class_name? It doesn't have a ton of use cases, but there are some, eg. PROPERTY_USAGE_ARRAY (though since the script has to be a tool for that regardless, I guess that _validate_property takes care of this...?)

@dalexeev
Copy link
Member

dalexeev commented Jun 5, 2024

For the same reason as type. We do have a few exceptions when class_name is used due to the small number of fields in the PropertyInfo structure, but in most cases you should not overwrite type and class_name.

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

Successfully merging this pull request may close these issues.

gdscript: @export does not support custom hints (e.g. suffix:)
8 participants