godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add `@export_toggle` to GDScript #8717

Open DexterFstone opened 10 months ago

DexterFstone commented 10 months ago

Describe the project you are working on

tower defense

Describe the problem or limitation you are having in your project

Sometimes, the inspector becomes too messy and the number of properties becomes too many also, I want to be able to hide some of the unused properties of the node image

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

@export_toggle let me make my inspector cleaner than before image

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

@export_category("Troop")
@export_group("Attack","attack_")
@export_toggle var attack_enable : bool
@export var attack_damage : int
@export var attack_range : float
@export_subgroup("DPS","dps_")
@export_toggle var dps_enable : bool
@export var dps_damage : int
@export var dps_interval : float
@export_subgroup("Stun","stun_")
@export_toggle var stun_enable : bool
@export var stun_duration : float
@export_toggle var stun_repeat : bool
@export var stun_cooldown : float
@export_subgroup("Multi Target","multi_target")
@export_toggle var multi_target_enable : bool
@export var multi_target_number : int

Untitled Project

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

I don't think it is possible

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

I don't think it is possible

AThousandShips commented 10 months ago

You can already do this with _validate_property

DexterFstone commented 10 months ago

You can already do this with (_validate_property)[https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-private-method-validate-property]

Complicated and time wasting, needs faster way

AThousandShips commented 10 months ago

Not really, also how would this work if you want to control properties in inherited classes? What if you want to hide properties based in a property in a base type?

At least you need to add this to the workaround part as it's not at all impossible to do, in fact it's very easy to do

So simple issues that this proposal can't handle but that is easily done with _validate_property:

This is a very specific case, which is very, very, easy to do with only a little code, and is very limited

Calinou commented 10 months ago

This is already available since 4.2 thanks to _validate_property() now being usable in scripts: https://github.com/godotengine/godot-proposals/issues/1056#issuecomment-1777025581

This is as close as it gets to the engine's own implementation for hiding certain properties depending on the values of other properties. It requires defining a virtual method, but this allows it to be flexible enough to cover advanced use cases.

It's technically feasible to define an automated way to do this that could work with the first boolean property of a category (if its name ends with _enabled), but Godot tends not to go the "convention over configuration" route. For instance, if you have a category named Follow and the first property is a boolean follow_enabled, toggling this boolean would hide/show all other properties in the Follow category (except follow_enabled).

Mickeon commented 10 months ago

I do think there needs to be better shortcuts for the most common export use-cases.

A property group "toggle" could be one of them, but I'm not sure about solving this with its own export type. It would also need its own PROPERTY_HINT and way to be visualized in the editor, even though, as mentioned above, this can already be accomplished, not just with _validate_property().

Also, the proposal has the property automatically infer what it relates to, although in practice it would be better to have more control over it.

@export_toggle("attack_") var attack_enabled
poohcom1 commented 10 months ago

This is a very specific case, which is very, very, easy to do with only a little code, and is very limited

This is a very common use case in my code base and it results in a lot of boilerplate. IMO, if engine dev finds this feature useful enough to use it in built-in nodes semi-regularly (not unlike export groups and categories), I think there are more than a few users who would find it useful.

Using _validate_property might be simple to implement, but it quickly adds up when you're constantly using it. Atleast in C#, this means that you need to:

  1. Turn the script into a tool script. (already not ideal because of all the potential issues)
  2. Add Engine.IsEditorHint() checks in various places.
  3. Turn the toggle field into a property to call NotifyPropertyListChanged().
  4. Add a backing field to the property so the value can be stored.

For a real example, this is a script that only uses 2 toggles (only the export logic):

Current code ```cs [Tool] public partial class AttackHitbox : Area2D { [Export] public bool IsNeutral { get => _isNeutral; set { _isNeutral = value; NotifyPropertyListChanged(); } } private bool _isNeutral = false; [Export] public Node? CombatHandlerNode { set; get; } [Export] public Node? FactionProviderNode { get; set; } [Export] public bool AutoSetAttackDirection { set { _autoSetAttackDirection = value; NotifyPropertyListChanged(); } get => _autoSetAttackDirection; } private bool _autoSetAttackDirection = true; [Export] public Node2D? FollowFacing; [Export] public AttackDirection AttackDirection = AttackDirection.Right; public override void _ValidateProperty(Dictionary propertyDict) { var property = new GodotProperty(propertyDict); // My custom typesafe wrapper, just ignore it // Neutral if (IsNeutral) { if (property.Name == nameof(FactionProviderNode) || property.Name == nameof(CombatHandlerNode)) property.Usage = PropertyUsageFlags.NoEditor; } // Attack direction if (_autoSetAttackDirection) { if (property.Name == nameof(AttackDirection)) { property.Usage = PropertyUsageFlags.NoEditor; } } else { if (property.Name == nameof(FollowFacing)) { property.Usage = PropertyUsageFlags.NoEditor; } } } } ```
Using toggle ```cs public partial class AttackHitbox : Area2D { [ExportToggle("Affiliation_")] [Export] public bool IsNeutral; [Export] public Node? Affiliation_CombatHandlerNode { set; get; } [Export] public Node? Affiliation_FactionProviderNode { get; set; } [ExportToggle("Facing_")] [Export] public bool AutoSetAttackDirection = true; [Export] public Node2D? Facing_FollowFacing; [Export] public AttackDirection AttackDirection = AttackDirection.Right; } ```

This is about 45 extra lines of code just for two toggles (probably less for GDScript since C# wastes a lot of lines). I'm aware that I can turn this into a generic function to reduce most of the boilerplate, but it still requires tool scripts and all of its ramifications.

I do agree that there are a lot of details to clear up in this proposal because turning it into a generic annotation requires simplifying the logic a lot and leaves out certain use cases (for example, my AutoSetAttackDirection is supposed to show FollowFacing when true, but also show AttackDirection when false, which won't really be solved by a basic annotation). However, I would be satisfied with a basic hide/show annotation for groups.

DexterFstone commented 10 months ago

I copied the document example of _validate_property and the result is nothing or did I miss something? Untitled Project

Update: I checked it in Godot 4.3 dev 1 and it works, but just with property PROPERTY_USAGE_READ_ONLY Untitled Project

AThousandShips commented 10 months ago

You can't use property.usage |= PROPERTY_USAGE_NONE, you need to use property.usage = PROPERTY_USAGE_NONE

idbrii commented 10 months ago

@poohcom1 In addition to a helper function, using the proposed system's name prefix pattern in the current system simplifies the code a lot:

Code

```cs [Tool] public partial class AttackHitbox : Area2D { // This initial section is mostly the same, but some renames. [Export] public bool IsNeutral { get => _isNeutral; set { _isNeutral = value; NotifyPropertyListChanged(); } } private bool _isNeutral = false; [Export] public Node? Affiliation_CombatHandlerNode { set; get; } [Export] public Node? Affiliation_FactionProviderNode { get; set; } [Export] public bool AutoSetAttackDirection { set { _autoSetAttackDirection = value; NotifyPropertyListChanged(); } get => _autoSetAttackDirection; } private bool _autoSetAttackDirection = true; [Export] public Node2D? Facing_FollowFacing; [Export] public AttackDirection AttackDirection = AttackDirection.Right; // NEW! This could be a project-wide shared static or a member of GodotProperty. static PropertyUsageFlags HideMatchingProperty(bool is_hidden, string name_prefix, ref GodotProperty property) { if (is_hidden && property.Name.StartsWith(name_prefix)) { property.Usage = PropertyUsageFlags.NoEditor; } } public override void _ValidateProperty(Dictionary propertyDict) { var property = new GodotProperty(propertyDict); // My custom typesafe wrapper, just ignore it // So simple! HideMatchingProperty(IsNeutral, "Affiliation_", property); HideMatchingProperty(_autoSetAttackDirection, "AttackDirection", property); HideMatchingProperty(!_autoSetAttackDirection, "Facing_", property); } } ```

Unfortunately, the requirement to call NotifyPropertyListChanged makes the getters quite a bit more verbose.

If we could add a [NotifyPropertyListChanged] to fields so it automatically generates a setter that calls NotifyPropertyListChanged, then most of the boilerplate is gone. Not sure if such an attribute is possible or more appealing to engine developers.

nessinby commented 10 months ago

I would like to also say that adding the original proposal helps this functionality be discoverable as well.

If someone's looking for the ability to toggle exports, they'd first look under the docs for @export or browse the auto-correct for @export, and see that it's not there. They'd (rightfully) assume it doesn't exist. Heck, that's what I thought, up until I saw this thread by chance.

tokengamedev commented 9 months ago

In one of projects, I had a need for three states, where variables can be displayed based on three states rather than toggled state (dual state). I have to use a Enum drop down. when I implemented through _validate_property, I faced some issues, as listed below:

  1. The script itself became too large (in my perspective).
  2. Too many calls happening on _validate_property method. It is called multiple times from the engine itself, so you cannot write more than few if statements (Although it does not matter in the editor, but debugging was hard).
  3. Unstable PROPERTY USAGE Enum values. Experimented a few Enum values, some of them working, and some not (especially the ARRAY and STRING ones) as per the documentation.
  4. adding @tool in the script

After long thought through, I redesigned by using Godot Composition Pattern and avoided the whole feature itself. Instead of on/off flags I added components as I needed in the scene.

I will suggest thinking through your approach if you have too many toggle items to be done on the Inspector.

damvcoool commented 9 months ago

I was able to achieve this with some elbow grease, and help from the forums. https://forum.godotengine.org/t/conditionally-show-exported-variables/43678

However I was thinking it would be far easier if there was some sort of conditional export like

@exportif("attack", true) var attack_power:float = 10.0

Also it would be great of @tool would not be needed to use such an export type.

AThousandShips commented 9 months ago

Please put single quotes around annotations instead of pinging random accounts 🙂, like so "`@tool`"

JHDev2006 commented 7 months ago

bit of necro posting here, apologies, but having something like this would be INCREDIBLY helpful, as while _get_property_list() DOES work, its not exactly easy to figure out (source: myself lol), and it also requires the script its a part of to be a @tool script, which means that this functionality isnt possible if a tool script cannot be used.

edit: just saw that its possible to use without tool scripts, but it still would be great to have this functionality, regardless of whether its already possible to achieve with _get_property_list(), as it dosent matter if the annotation cannot have the full functionality as _get_property_list() since most people that want more complicated and intricate solutions will probably use get_property_list() anyways, so a basic show hide feature would be especially helpful, it can be something as simple as:

@export var var_1 := false
var var_2 := 0
var var_3 := "Test"
var var_4 := {}
@export_vars([var_2, var_3, var_4], var_1 == true) ## Arg 1 is an array of vars to show or hide, Arg 2 is the condition

hell, if theres loads of vars that need to be exported conditionally, you could do something like @export_group_if(GroupName, var1 == true) or @export_category_if(CategoryName, var1 == true)

Mickeon commented 7 months ago

bit of necro posting here

No such thing in here, so long as what you have to add is meaningful (which it is).

hell, if theres loads of vars that need to be exported conditionally, you could do something like @export_group_if(GroupName, var1 == true) or @export_category_if(CategoryName, var1 == true)

This is probably not possible as annotations are purely solved at compile-time. The closest equivalent to achieve this may be by passing a Callable, instead.

JHDev2006 commented 7 months ago

Ah see what your saying, if that's the case then would it be possible for the vars that have their visibility affected to be exported initially then?

@export var var_1 := false

@export var var_2 := "Test"

@show_exports_if([var_2], check_call)

func check_call() -> bool:
    return var_1 == true
Mickeon commented 7 months ago

I don't see why not, but I have no knowledge of the inner workings.

I do like this general suggestion of @export_group_if over the original proposal. It feels more dynamic and general-purpose. Although it doesn't quite solve it, because some boilerplate code would still be necessary. Some may argue it's a good thing because it's more understandable, but yeah.

@show_exports_if([var_2], check_call)

This would not work because var_2 would be passed by value. It would need the property's name as a String. Either way, I feel like @export_group_if reads more naturally.

JHDev2006 commented 7 months ago

Although it doesn't quite solve it, because some boilerplate code would still be necessary.

would it not be possible to have the condition be an annonymous lambda function?

@export var var_1 := false
@export_group(Group1)
@export var var_2 := 0

@export_group_if(Group1, func(): return var_1 == true)

something like this could work? or maybe show_export_group_if() would read better?

StalOlympus commented 7 months ago

I think this would be very useful for non-advanced users like me who are terrified of @tool.