godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 93 forks source link

Add an annotation to only display property if node is root of current scene #10024

Open Meorge opened 3 months ago

Meorge commented 3 months ago

Describe the project you are working on

A top-down 2D shooter, as well as a rhythm-centric turn-based RPG (although these specifics aren't relevant here)

Describe the problem or limitation you are having in your project

Before making a proposal, this feature idea was discussed here: https://github.com/godotengine/godot-proposals/discussions/10017

I like to use @exported properties on my scripts to link sub-nodes to them. For example, I might have a Player scene like this:

CleanShot 2024-06-23 at 16 54 54@2x

With the following script, I can drag and drop the nodes into the property slots, and then access them in the Player script:

extends Node2D

@export var sprite: Sprite2D
@export var sound_player: AudioStreamPlayer2D
@export var health: int = 10
...

CleanShot 2024-06-23 at 16 55 34@2x

If I place this scene in another scene, those slots still take up space, despite the fact that they should be "fixed" at this level (if I want to change them, I'll go into the Player scene to do that).

CleanShot 2024-06-23 at 16 59 41@2x

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

I am proposing that functionality be added to GDScript that allows the user to specify that a property should only be drawn in the inspector if it is on the root node of the currently-edited scene. This will make it easier to adhere to the "black box" philosophy of scenes in Godot, and hide extraneous cluttering information from the user as they work with scenes on a higher level.

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

For now, I propose adding an annotation to GDScript, @export_if_root, which is added in addition to an existing @export annotation (or one of its many variants). When this annotation is added to a property, the property is only considered to be drawn if it is the root node of the scene currently being edited.

GDScript code of the current implementation:

extends Node2D

@export @export_if_root var sprite: Sprite2D
@export @export_if_root var sound_player: AudioStreamPlayer2D
@export @export_if_root var health: int = 10

func _process(_delta):
    print("Sprite = %s, sound player = %s, health = %s" % [sprite, sound_player, health])

This is currently implemented in my branch of Godot here: https://github.com/Meorge/godot/tree/export_if_root

I recognize that this is not necessarily the best exact syntax for this feature, and should be discussed before anything is merged. Here are a few alternatives I thought of.

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

As users take further advantage of nested scenes, this enhancement will be used more and more.

The behavior can be accomplished with pure GDScript, but it feels rather clunky to me when combined with regular game code:

@tool
extends Node2D

@export var sprite: Sprite2D
@export var sound_player: AudioStreamPlayer2D
@export var health: int = 10

func _process(_delta):
    if Engine.is_editor_hint():
        return
    print("Sprite = %s, sound player = %s, health = %s" % [sprite, sound_player, health])

func _validate_property(property):
    var scene_root = EditorInterface.get_edited_scene_root()
    if property.name in ["sprite", "sound_player"] and scene_root.is_ancestor_of(self) and not scene_root.is_editable_instance(self):
        property.usage = PROPERTY_USAGE_NO_EDITOR

This has a few drawbacks, in my opinion:

  1. When we add the @tool annotation to the script, making it run in the editor, we have to add checks in (at least) the _ready() and _process() functions to ensure that the actual script behavior only runs when the game is playing. This boilerplate feels unnecessary and bloated to me.
  2. The logic inside of _validate_property() function isn't immediately intuitive or readable in my opinion. Additionally, requiring the property names to be written in an array means that if for some reason the user changes the property names, they'll have to go back and change them here as well. With an annotation on the variable itself, this wouldn't happen.

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

I don't believe there is a way to create custom annotations or modify existing ones with how Godot and GDScript currently works. Thus, I think it would have to be core rather than an add-on.

theraot commented 3 months ago

On one hand we already have other @export_ annotations that imply @export so we don't need to have two annotations, and this might work like that. On the other hand, we might want to be able to combine this annotation with those since this is an orthogonal concern.

It is worth noting that with PROPERTY_USAGE_SHOW_IF_ROOT we can use @export_custom to get this feature, so a minimum version of this does not add a new annotation.

Hopefully somebody else can chime in on what is the best approach.

Spartan322 commented 3 months ago

Kinda feel like this fits more as an extension of #9842, at minimum they are related.

Meorge commented 3 months ago

I found #9842 when back when I was drafting this up; from there I found the much earlier #1056. They both certainly share the foundation of changing the visibility of properties based on specific criteria. The use case this proposal is targeting is standard enough that I think it would make sense to have it be more streamlined and intuitive than what a generic @export_if() decorator as described there would do.

One of the authors in #9842 also states:

Note that annotation arguments must be constant expressions. Annotations are resolved at compile time and are the same for all instances of the class. So the condition cannot be arbitrary

which makes it significantly harder, if not impossible, to implement @export_if() given how annotations currently work.

Since we have an implementation of this behavior working, I think it'd make sense to use it for now. Perhaps in the future, if annotations are expanded to support runtime, this behavior could be adapted over to it. Some code from my implementation of this behavior could possibly also be used to facilitate more general hide/show behavior for variables in the inspector.

Mickeon commented 3 months ago

I've always had the idea on the back of my head that if a @hide annotation or private keyword were to be implemented, it could be combined with @export to achieve the effect described in this proposal.

Meorge commented 3 months ago

I like the idea of private (possibly @private as an annotation?) being able to at least discourage the language server from suggesting variables/properties while in another script, and possibly even raising warnings/errors if accessed improperly.

Looks like proper access modifiers are being discussed at #641! As I skim through that thread, I don't see anyone saying they've actually taken the project on, but if someone did, then I think @private @export var my_var could make sense to achieve this behavior.

Mickeon commented 3 months ago

As an addendum, my described behavior is very similar to the addon Hide Private Properties which achieves this by filtering exported properties starting with "_". It's a few lines of code and may be a better workaround than the workaround in this proposal on a larger scale, as it is tied to the Inspector and not the individual script.

https://github.com/kenyoni-software/godot-addons/tree/main/addons/hide_private_properties