godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Ability to declare exported properties that are not shown (aka hidden) in inspector #7794

Closed limbonaut closed 9 months ago

limbonaut commented 1 year ago

Describe the project you are working on

I used _get_property_list to hide properties in inspector on multiple occasions. It is especially useful for data serialization in the resource files when making editor plugins. However, the dictionary syntax is not easy to use, and it is quite easy to make a mistake.

Describe the problem or limitation you are having in your project

It's cumbersome and error-prone to implement _get_property_list to hide a property in inspector. There has to be a better way!

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Similar to @onready, there could be a @hidden annotation to hide a property in the inspector.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

@hidden @export var data: MyData

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around using _get_property_list:

extends Node

var data: Array

func _get_property_list():
    # Export `data` property, but don't show it in inspector.
    # (`data` property will be saved in the scene file.)
    var serializable = [
        {
            name = "data",
            usage = PROPERTY_USAGE_NO_EDITOR,
            type = TYPE_ARRAY,
        },
    ]
    return serializable

Is there a reason why this should be core and not an add-on in the asset library?

N/A

dalexeev commented 1 year ago

Given that hidden properties are not visible in the editor, there is no point in combining @hidden with @export_* annotations, only with @export. Therefore, I suggest adding a single annotation and name it @export_hidden, @export_storage or @export_no_editor.

Kakiroi commented 1 year ago

This can be useful for animation player to get property, or marking duplicatable properties in resource files without cluttering editor.

Jeremi360 commented 1 year ago

It would also useful for custom classes that want to overwrite some exported var by class after it inherits from.

For example I develop AdvancedTextLabel that inherits from RichTextLabel: so I would hide text and bbcode as my class it to be enabled by default

Maran23 commented 1 year ago

Another idea could be @export(false), so @export accepting a visibility parameter which defaults to true

dalexeev commented 1 year ago

Another idea could be @export(false)

Interesting idea, but we already have many specialized export annotations instead of just one annotation with parameters (like export keyword in 3.x). In addition, autocompletion will add parentheses and place the cursor between them, which will annoy users. It is logical that the basic and most often used export annotation has no parameters.

limbonaut commented 1 year ago

@export_hidden, @export_storage and @export_no_editor are all good fits imho. And the main question right now is, as discussed in the PR, which one would stand the test of time. I think @export_hidden is a little disconnected from internal implementation, but sounds better than @export_no_editor version and less typing. And @export_storage fits best to most common intention for this functionality, which is to serialize some variables with a scene/resource file.

Jeremi360 commented 1 year ago

I think @export_hidden and any other variant with prefix @export for this usage would be confusing as other @export make var visable in editor.

Also @export make it sound like it would be implented in way:

@export_hidden var new_var

Which make unuseable, in example I gave above I was thinking it would be this way:

For example I develop AdvancedTextLabel that inherits from RichTextLabel: so I would hide text and bbcode as my class it to be enabled by default

@hidden("property/path")
# or
@on_editor("property/path")
dalexeev commented 1 year ago

I think @export_hidden and any other variant with prefix @export for this usage would be confusing as other @export make var visable in editor.

"Export" does not mean "display in inspector", it means "get property value from scene/resource file". That is, serialization is primary. The fact that the property is editable in the inspector is secondary, although from the user’s point of view it seems the opposite. See also #3906.

Therefore, I would prefer that the annotation name start with @export_ rather than be something else entirely. It would be consistent and clear. In addition, we have a check that multiple export annotations cannot be applied to one variable. The naming exception would be confusing in my opinion.

@export_hidden looks pretty good, it means making the property serializable (exported) without displaying it in the editor. But the question is whether @export_hidden should correspond to PROPERTY_USAGE_STORAGE or PROPERTY_USAGE_NO_EDITOR. See godotengine/godot#82122 for details.

limbonaut commented 1 year ago

As I see it, there are two quite distinct use cases:

  1. Serialization: Export GDScript properties without exposing them to the Inspector with the intention of serialization (aka save some variables in a scene/resource file).
  2. Shadow/hide already existing core/script class properties in the inspector.

I'm not sure how much demand is for the second use-case. If you exported a property with the same name as a core class property, you should effectively shadow it for any script (but not the core C++ code). However, the property of the core class will likely show up in the inspector anyway, since it is exported in the C++ code.

As for the first use case, I also proposed using @serialize instead of @export_* to narrow down the purpose of the annotation. In my opinion, it is the clearest option when it comes to the first use-case (aka serialization). Reasons against it are: a) there is a code path in the engine that checks if only one @export* annotation used for any given property (it is considered an error); b) there is an established pattern of using @export_* for such things.

limbonaut commented 1 year ago

What are your opinions :+1: :-1: on renaming PROPERTY_USAGE_NO_EDITOR -> PROPERTY_USAGE_HIDDEN? Could be also utilized in other places like documentation parsing.

dalexeev commented 1 year ago

a) there is a code path in the engine that checks if only one @export* annotation used for any given property (it is considered an error)

No, that's not the problem. The parser does not use begins_with() for the check. All export annotations[^1] are processed in the GDScriptParser::export_annotations() method. The problem is that the error text will be confusing. For example:

@serialize
@export # <-- Error: Annotation "@export" cannot be used with another "@export" annotation.
var a = 1

This error is confusing because there are no other annotations applied to the variable that begin with @export and it would not be obvious to the user that @serialize is also an export annotation.

I also believe that the word "export" in our terminology already means serialization. The exported (serialized) property may or may not show up in the inspector, it doesn't matter. But introducing a new term, "serialization", which means the same thing as "export", will only confuse users more. Users may think that there is some difference between the two, that exported properties are not serialized, etc.

b) there is an established pattern of using @export_* for such things.

Please see the discussion in #3906 first. Even if the term "export" is not the best (there was a proposal to rename it to "expose", "storage", "serialize", etc.), this is established terminology and in any case cannot be renamed in 4.x due to compatibility.

What are your opinions 👍 👎 on renaming PROPERTY_USAGE_NO_EDITOR -> PROPERTY_USAGE_HIDDEN?

We can't really rename a constant, only deprecate it and add a new constant with the same value. But I don't think it makes sense now. Note that PROPERTY_USAGE_NO_EDITOR exists mainly for historical reasons.

3.x:

PROPERTY_USAGE_DEFAULT = PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_NETWORK,
PROPERTY_USAGE_NOEDITOR = PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_NETWORK,

4.x:

PROPERTY_USAGE_DEFAULT = PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_EDITOR,
PROPERTY_USAGE_NO_EDITOR = PROPERTY_USAGE_STORAGE,

Now PROPERTY_USAGE_NO_EDITOR == PROPERTY_USAGE_STORAGE, but in the future we may add other default flags.

[^1]: Except @export_category, @export_group and @export_subgroup which do not apply to variables, but are themselves "pseudo-members".

dalexeev commented 1 year ago

I thought about it from a different perspective. What if the user instead wants to display the field in the editor, but not store the value in a file? In this case, a modifier annotation makes more sense than duplicating all the export annotations. That is, export annotations by default use storage + editor, and we could add two modifier annotations @no_storage and @no_editor (the latter, however, still makes little sense to combine with anything other than @export).

dalexeev commented 9 months ago

I thought about it from a different perspective. What if the user instead wants to display the field in the editor, but not store the value in a file?