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

Add @required annotation for exported properties #8343

Open
kerelape opened this issue Nov 3, 2023 · 9 comments
Open

Add @required annotation for exported properties #8343

kerelape opened this issue Nov 3, 2023 · 9 comments

Comments

@kerelape
Copy link

kerelape commented Nov 3, 2023

Describe the project you are working on

3D adventure

Describe the problem or limitation you are having in your project

I am heavily using @export var node: Node where Node is any Node descendant to avoid depending much on node paths, but sometimes s.. happens and I simply forget to set a value to the field in the inspector, and in such cases I will only know about that in runtime.

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

It would be great to have something like an annotation (e.g. @required or @export_required)

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

When a property is marked as required the inspector would promt you about it the same way configuration warnings do, but also highlight the missing property (e.g. the yellow triangle may be placed in front of property saying it's required to be set to a value).

This would not only work for nodes, but also for any Object descendant, like Resource.

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

A possible workaround would be to mark scripts with @tool and check for the properties in _get_configuration_warnings

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

It requires changes to gdscript

@kerelape kerelape changed the title Add @required annotation for exported fields Add @required annotation for exported properties Nov 3, 2023
@Calinou
Copy link
Member

Calinou commented Nov 3, 2023

I would prefer something like #1729, which is a more universal solution and doesn't error if the variable isn't used anywhere.

@kerelape
Copy link
Author

kerelape commented Nov 3, 2023

I would prefer something like #1729, which is a more universal solution and doesn't error if the variable isn't used anywhere.

This would not solve the issue with Resource-type properties.

And still the issue would only arrive when the game is already running.

What I want is to have a built-in problem reporting for exported properties the same way it's done for physics objects missing collision nodes

@RedMser
Copy link

RedMser commented Nov 3, 2023

the inspector would promt you about it the same way configuration warnings do, but also highlight the missing property (e.g. the yellow triangle may be placed in front of property saying it's required to be set to a value).

You could use configuration warnings combined with godotengine/godot#68420

@kerelape
Copy link
Author

kerelape commented Nov 3, 2023

the inspector would promt you about it the same way configuration warnings do, but also highlight the missing property (e.g. the yellow triangle may be placed in front of property saying it's required to be set to a value).

You could use configuration warnings combined with godotengine/godot#68420

Yes, this is what I am currently doing, but it requires me make my scripts @tool, which requires me to add those Engine.is_editor_hint() checks. Also it's a quite frequent case and almost every script in my game has that line-to-line similar are-properties-set checks, which could be nice to simplify to a single annotation.

I think it's an overkill to do all those check by hand for a simple is-null validation. 68420 is great for heavy cases, where you do more than that

@miv391
Copy link

miv391 commented Dec 14, 2023

@export is the way that nodes need be to referenced. @export was most likely not invented for exactly this kind of use, but until Godot gets a feature that automatically fixes the following code when camera node's name changes or the node is moved, @export is so much better than getting nodes by name. (Yes, I know % is meant for fixing breaks caused by moving nodes, but it doesn't help when node is renamed.)

var cam1 = get("sub/camera")
var cam2 = $sub/camera

So it would be nice to have the proposed feature. I would guess that getting new features to export variables would be more possible than automatically renaming node paths.

Currently I'm doing something like this:

@export var camera: Camera2D

func _ready():
	assert(camera)

The feature could also be something like automatically asserting on _ready that the variable is not null. The sooner you get the error, the better.

@Pizzaandy
Copy link

Pizzaandy commented Dec 23, 2023

Similar to #2519

Right now, the intended way to customize inspector properties is to either:
a) Override one of the property-related methods like _get_property_list or _validate_property
b) Write an inspector plugin

It's not very ergonomic to implement reusable property editors at the moment.
I like the way Unity allows users to create property attributes and drawers, and I think that's the right way forward for Godot.

Give users the power to make custom annotations that change how exported properties are rendered in the inspector, then provide a few built-in validation annotations like @required (since this behavior is all over the place in the core editor).

This doesn't necessarily have to be implemented via annotations, but there should be some way of declaratively composing inspectors rather than messing with _get_property_list or plugins.

@nlupugla
Copy link

nlupugla commented Jan 2, 2024

What about a slightly more general approach where you can tag properties as described in this proposal: #8752

Instead of writing

@required
@export var node : Node

you could write

@tool

@tag("required")
@export var node : Node :
    set(value : Node) :
        node = value
        update_configuration_warnings()


func _get_configuration_warnings() -> PackedStringArray:
    var warnings : PackedStringArray = []
    var properties : Array[Dictionary] = get_tagged_properties("required")
    for property in properties:
        if get(property.name) == null:
            warnings.append(property.name + " is required")

@leonardoraele
Copy link

What about a slightly more general approach where you can tag properties as described in this proposal: #8752

[...]

Because then you have to reimplement this feature in every script you create.

@Nohac
Copy link

Nohac commented Apr 21, 2024

I would also like to see this in some form, but I think this could be implicit whenever #162 is implemented.
If we get nullable types in godot, then any type that is not marked as nullable could show up as a required field and show a warning in the editor if it's not set.

I understand that nullable types would be a breaking change, so I hope they go the same rout as they did with static typing and make it an opt in setting.

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

No branches or pull requests

9 participants