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 an annotation to only display property if node is root of current scene #10024

Open
Meorge opened this issue Jun 24, 2024 · 6 comments
Open

Comments

@Meorge
Copy link

Meorge commented Jun 24, 2024

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: #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.

  • @export @if_root var my_property - This reads a bit more naturally and takes up less space. However it may be confusing to see on its own, and/or incorrectly imply that it modifies the behavior of other decorators as well.
  • @export(true) var my_property - Making this annotation into an argument for @export would certainly be the most succinct approach, but it would have to be added onto every variant of @export. On top of that, the lack of keyword arguments means that it's not immediately clear what the argument refers to. One could reasonably - yet incorrectly - assume that @export(true) means "export this variable" and @export(false) means "don't export this variable".

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.

@Calinou Calinou changed the title Annotation to only display property if node is root of current scene Add an annotation to only display property if node is root of current scene Jun 24, 2024
@theraot
Copy link

theraot commented Jun 24, 2024

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
Copy link

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

@Meorge
Copy link
Author

Meorge commented Jun 24, 2024

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
Copy link

Mickeon commented Jun 24, 2024

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
Copy link
Author

Meorge commented Jun 24, 2024

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
Copy link

Mickeon commented Jun 24, 2024

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

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

Successfully merging a pull request may close this issue.

5 participants