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_storage annotation #82122

Merged

Conversation

dalexeev
Copy link
Member

I'm not sure about the name: @export_hidden, @export_storage or @export_no_editor.

Also PROPERTY_USAGE_NO_EDITOR is currently the same as PROPERTY_USAGE_STORAGE, but in the future we may add other default flags (PROPERTY_USAGE_NO_EDITOR is PROPERTY_USAGE_DEFAULT without PROPERTY_USAGE_EDITOR).

@export ≈ PROPERTY_USAGE_DEFAULT
@export_hidden = PROPERTY_USAGE_NO_EDITOR (default without editor)

vs

@export ≈ PROPERTY_USAGE_DEFAULT
@export_hidden = PROPERTY_USAGE_STORAGE (only storage)

@dalexeev dalexeev added this to the 4.x milestone Sep 22, 2023
@dalexeev dalexeev requested a review from a team as a code owner September 22, 2023 14:16
@YuriSizov
Copy link
Contributor

I think export_storage is the best option as it says what the annotation does do, not what it doesn't do. But this invites back the debate about the name and function of export annotations, kinda. Well, maybe this can give us closure.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 22, 2023

Using something like "no editor" like the flag could be the most uniform, but I think that hidden risks being confusing

Edit: as in, either first option with @export_no_editor or second with @export_storage

@limbonaut
Copy link
Contributor

Perhaps a new annotation would be more clear? For example:

@serialize var data: MyData  # == PROPERTY_USAGE_STORAGE

@dalexeev
Copy link
Member Author

I would prefer that the annotation name start with @export_ for consistency and clarity. We also have a check that several export annotations cannot be applied to one variable; an exception in the naming would be confusing in my opinion.

@limbonaut
Copy link
Contributor

The intent for this functionality is often to save some data with the scene/resource file without exposing it to the inspector. So I think @export_storage == PROPERTY_USAGE_STORAGE is a good fit.

@dalexeev
Copy link
Member Author

So I think @export_storage == PROPERTY_USAGE_STORAGE is a good fit.

Yes, this looks like a good option to me. But the question is what default flags we can add in the future, and whether we will later regret not choosing @export_no_editor == PROPERTY_USAGE_NO_EDITOR. Although in this case we will still be able to add this too and it will be consistent.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 23, 2023

I think @export_storage == PROPERTY_USAGE_STORAGE for user code would be fine and follow the principle of least surprise. If we expand "no editor"/"default" to do more, we'd have to consider what that changes for user scripts anyway, so I think we can cross that bridge when we get there.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Sep 23, 2023
@dalexeev dalexeev force-pushed the gds-add-export-hidden-annotation branch from bdcb7dc to 6d35dee Compare September 23, 2023 15:00
@dalexeev dalexeev changed the title GDScript: Add @export_hidden annotation GDScript: Add @export_storage annotation Sep 23, 2023
@dalexeev dalexeev force-pushed the gds-add-export-hidden-annotation branch from 6d35dee to 8ceeb1a Compare September 23, 2023 16:57
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Aside from a few comments, I'm approving the design of this (and implementation seems fine as well).

@Kakiroi
Copy link

Kakiroi commented Oct 12, 2023

Since #82843 now disallows using @ export to hold node reference in resource, will this be a viable option? Will this bypass the error?

@AThousandShips
Copy link
Member

AThousandShips commented Oct 12, 2023

This doesn't change the underlying issue of the access of Node from Resource, and specifically storing the value is what is not working, as opposed to having just a reference to it

So I'd say this PR doesn't change that PR at all

@Kakiroi
Copy link

Kakiroi commented Oct 12, 2023

This doesn't change the underlying issue of the access of Node from Resource, and specifically storing the value is what is not working, as opposed to having just a reference to it

So I'd say this PR doesn't change that PR at all

So this annotation will still forbid users from having duplicatable node variable in resource? The warning is there because of exported variable having node path, not reference. I was hoping using this annotation wouldn't trigger that warning. Since there's no way users can assign node path in the resource anyway.

@AThousandShips
Copy link
Member

It's not possible to store a Node reference, they are always stored as a path, that's why it's forbidden, you're getting it backwards, this PR allows you to have exports not visible in the editor but only in storage

@dalexeev dalexeev force-pushed the gds-add-export-hidden-annotation branch 2 times, most recently from e60668b to e63dacf Compare December 17, 2023 08:53
@dalexeev dalexeev force-pushed the gds-add-export-hidden-annotation branch from e63dacf to 138a58f Compare December 17, 2023 09:02
@YuriSizov
Copy link
Contributor

This needs a rebase (and a review from the @godotengine/gdscript team).

@dalexeev dalexeev force-pushed the gds-add-export-hidden-annotation branch from 138a58f to 3a3a201 Compare December 19, 2023 17:57
@adamscott
Copy link
Member

As shared by @dalexeev during the GDScript meeting, this PR seems to overlap with #72912.

Maybe we should only expose @export_custom with the ability to use PROPERTY_USAGE_STORAGE or PROPERTY_USAGE_NO_EDITOR as flags for the property.

We all agree that we should not have features that overlap too much.

@dalexeev
Copy link
Member Author

dalexeev commented Feb 13, 2024

We all agree that we should not have features that overlap too much.

Note that @export_custom is more verbose and resets the hint and usage flags ("reflection" with get_property_list()), unlike @export_storage. You can also replace any @export annotation with @export_custom, so this argument doesn't always work.

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

LGTM

@akien-mga akien-mga merged commit 21ee371 into godotengine:master Feb 27, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to declare exported properties that are not shown (aka hidden) in inspector
8 participants