godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

`@export var x : bool = null` should be allowed or `@export var x : bool` should be `null` #9269

Closed ballerburg9005 closed 2 months ago

ballerburg9005 commented 4 months ago

Describe the project you are working on

I want a radio button for exported variable that is either unset (no circle-arrow) or set to true or false (indicated by circled arrow). Radio buttons are only possible with "bool" as type.

This is needed all the time to override dozens of default values pulled from a Resource for individual instances. E.g. a Resource class for NPCs can either hold enemies or friends, indicated by the boolean value "hostile". But sometimes and only sometimes you want to override it, which is why you need an additional instance variable that can hold an unset value.

So if a typed @export variable such as bool can never be null, it only follows that we can never use any of the types to create radio buttons etc. in the first place, and always have to use untyped variables or enum workarounds, which defeats the purpose of having types in @export variables.

The current behavior is counter-intuitive and clumsy, because the variable is initialized with "false" if unset and cannot be null to indicate unset state, even though from a usability-perspective having @export to allow null for any sort of type is totally possible without creating any issues (and also doesn't break compatibility when only null if explicitly set to null).

Describe the problem or limitation you are having in your project

Currently, I have to use the following workaround to overcome the limitation:

@export_enum("null", "false", "true") var disallow_rotate: String = "null"
var no_rotate
@export_enum("null", "false", "true") var disallow_drag: String = "null"
var no_drag

@export var component : Component # Resource class

func _ready():
    no_rotate = component.no_rotate if disallow_rotate == "null" else false if disallow_rotate == "false" else true
    no_drag = component.no_drag if disallow_drag == "null" else false if disallow_drag == "false" else true

It doesn't create a radio button as desired, but a drop down menu.

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

This is how the code would work without the limitation:

@export var no_rotate : bool = null
@export var no_drag : bool = null

@export var component : Component # Resource class

func _ready():
    no_rotate = no_rotate if no_rotate != null else component.no_rotate
    no_drag = no_drag if no_drag != null else component.no_drag

This would create a radio button and you would see if the value has been set by the circle arrow symbol. To make it even more obvious that it is unset, the checkbox could contain a question mark.

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

see above

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

It will be used quite often, workaround see above.

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

it has to be core

Production edit: added syntax highlight

AThousandShips commented 4 months ago

The built in types are not nullable, so this would require changes to the engine that would break compatibility, as currently errors occur when casting or assigning null to these types, which acts as safety, see here, existing code would break with this change

I suspect this will solve your issue as well:

ballerburg9005 commented 4 months ago

Well @export is a special case for which an exception could be made before the type is initialized by the engine.

It also wouldn't break compatibility if as suggested the default value only became null if explicitly declared (like in the example code).

Existing code would not break with this change.

AThousandShips commented 4 months ago

That can't happen though, an explicitly typed bool can't be null

Also please put any annotation between back ticks or you'll tag accounts

ballerburg9005 commented 4 months ago

An equivalent solution would be something like this, the only difference being simply a matter of parsing the @export line. This only illustrates that this is not a type issue at all, but nothing more than an issue of how @export is interpreted to create different kinds of UI elements, such as checkboxes.

@export_bool var no_rotate = null

This then creates a checkbox as suggested, and when clicked it puts false/true into it. I think this was even how it worked in Godot 3, but it was removed in favor of the new typed notation. It shows the circled-arrow as usual to indicate the set state, and allows unsetting by clicking the symbol.

The variable declared in @export doesn't have to be explicit bool, if explicitly set to null, it is just as simple as that.

AThousandShips commented 4 months ago

The variable declared in @export doesn't have to be explicit bool, if explicitly set to null, it is just as simple as that

No, all exported properties must be typed. They can't be Variant

This is a type issue, please test that for yourself and you'll see (it won't be a bool, and can't be a bool)

AThousandShips commented 4 months ago

Also again, use backticks like so "`@export`"

Mickeon commented 4 months ago

Wouldn't this basically come from granted with nullable static typing? https://github.com/godotengine/godot-proposals/issues/162

AThousandShips commented 4 months ago

It would, hence why I linked it already above :)

KoBeWi commented 4 months ago

https://github.com/godotengine/godot-proposals/assets/2223172/de37e35c-57eb-4b8f-8866-d513888e67a4

@tool
extends EditorPlugin

var state3 := TriStateBoolInspectorPlugin.new()

func _enter_tree() -> void:
    add_inspector_plugin(state3)

func _exit_tree() -> void:
    remove_inspector_plugin(state3)

class TriStateBoolInspectorPlugin extends EditorInspectorPlugin:
    func _can_handle(object: Object) -> bool:
        return true

    func _parse_property(object: Object, type: Variant.Type, name: String, hint_type: PropertyHint, hint_string: String, usage_flags: int, wide: bool) -> bool:
        if type == TYPE_INT and name.ends_with("_3"):
            add_property_editor(name, TriStateBoolEditor.new())
            return true
        return false

class TriStateBoolEditor extends EditorProperty:
    var button: TriStateCheckbox

    func _init() -> void:
        button = TriStateCheckbox.new()
        add_child(button)
        button.pressed.connect(func(): get_edited_object().set(get_edited_property(), button.state))

    func _update_property() -> void:
        button.state = get_edited_object().get(get_edited_property())

class TriStateCheckbox extends Button:
    var textures: Array[Texture2D]
    var state: int:
        set(s):
            state = s
            icon = textures[state]

    func _init() -> void:
        size_flags_horizontal = SIZE_SHRINK_BEGIN

        for color in [Color.RED, Color.GREEN, Color.BLUE]:
            var gradient := Gradient.new()
            gradient.remove_point(1)
            gradient.colors = [color]

            var texture := GradientTexture2D.new()
            texture.gradient = gradient
            texture.width = 16
            texture.height = 16
            textures.append(texture)

    func _pressed() -> void:
        state = (state + 1) % 3
ballerburg9005 commented 4 months ago

The variable declared in @export doesn't have to be explicit bool, if explicitly set to null, it is just as simple as that

No, all exported properties must be typed. They can't be Variant

This is a type issue, please test that for yourself and you'll see

But isn't that itself an issue of simply parsing @export in a way that makes sense to the user?

I feel that it is inappropriate/misleading to make this an issue about types. Because it is simply a matter of choice how the engine interprets the @export statement, and whether or not it allows variant and null type in UI elements.

This is not an issue of types, but an issue of how @export is behaving inflexible and user-unfriendly, by simply converting types 1:1 to UI elements, without any further considerations.

This creates this bad situation where factually unset variables are force-initialized with values. In the case of bool this means that you can only choose to set a single value, i.e. it is transformed from a bool into unary. And this makes it totally useless, when the initialization value isn't always false by default, but simply unknown information.

To say that it is a type issue would be as if saying that a door without door knob would be an issue of the hinges, because the hinges are all that is keeping the door closed in its current design state. I think this is a really odd way to think of it.

AThousandShips commented 4 months ago

I feel that it is inappropriate/misleading to make this an issue about types

Inappropriate to state the truth? Please... How would you represent it then? bool cannot be null, it can't, so how do you handle it?

This isn't some weird feature as you present it, this is simply how boolean values work, in every single programming language

You are asking for one of two things:

This isn't a limitation in the engine, you are asking a type to do what it simply is not designed to do

ballerburg9005 commented 4 months ago

Yes, you are talking hinges here. Hinges are a joint that cannot be disjointed, hence the door has to remain closed, because it is a hinge-issue. This is the truth and how every single joint works in every building.

AThousandShips commented 4 months ago

This creates this bad situation where factually unset variables are force-initialized with values

This is a feature not an issue... Uninitialized values are dangerous, unset variables should have a well defined value by default, what else would it be? Would int be just some random value?

dalexeev commented 4 months ago

You can do this with _validate_property() (note that this requires @tool). We also recently added the @export_custom annotation, but it only supports hint, hint_string and usage, not type.

Finally, I would recommend using an enum instead of bool:

enum Trilean {FALSE, UNKNOWN, TRUE}
@export var my_var := Trilean.UNKNOWN

bool by its nature has only two states, nullable built-in Variant types are not present in the core and GDScript.

Mickeon commented 4 months ago

I'm sorry, I do not see why exporting an integer with @export_enum is not the simpler solution to all of this. The solution provided to @dalexeev is much more ideal at all times.

Is it something to do with the current UI? Would you prefer if it were a series of radio buttons, or something else entirely? If yes, the existing @export_enum type could be improved to allow a different visualization.

But nullable bool is not it, as discussed above. If you're concerned about memory usage, all numerical Variants occupy the same space and it would be no different than an integer.

The different visualization can be done with addons, by the way. It could even be ad-hoc, and specifically tailored for your needs, as @KoBeWi provided.

KoBeWi commented 4 months ago

Well technically, we could allow Variant as a valid export type and it would be edited like Array's values, i.e. you select type and write a value. Then you could customize it further with plugins like the one I posted above and it would allow for tri-state bools or quad-state nulls or whatever.

Mickeon commented 4 months ago

That's true, and it would be quite lovely. But it's unlikely one would want ALL Variants available, so they'd need to be able to be filtered out. And all in all, it wouldn't be pretty with just a few choices (if the issue of this proposal is the UI).

dalexeev commented 4 months ago

Exported properties have 2 types: the variable type (it's a language feature) and the property info type (get_property_list()), which is used by the Inspector.

The type specifier restricts the type of the variable and this restriction should not be possible to bypass. This is how it works in most languages with static typing. The type specifier is not just a metadata for the editor.

Export annotations read the variable's static type (or initial value), eliminating the possibility/necessity to duplicate the type for the property info. However this is possible in 3.x:

export(bool) var a # Dynamic.
export var b: bool # Static.
export(bool) var c: bool # Both.

In 4.x, with the replacement of the keyword by annotation, this feature was removed. We could add type parameter to @export_custom, but I'm not sure that's a good idea. Most often, you don't need to change the type or export a variable that doesn't have both a static type and a constant initial value.

ballerburg9005 commented 4 months ago

First let me thank you for all the provided answers and solutions so far.

I agree this is a small issue and I regret to make such a huge deal out of it.

However long the following text is, I think it is important for @AThousandShips to read it.

This creates this bad situation where factually unset variables are force-initialized with values

This is a feature not an issue... Uninitialized values are dangerous, unset variables should have a well defined value by default, what else would it be? Would int be just some random value?

Sorry, but your posts come off as trolling, because you just ignore most of the situation and all of this has already been explained. If you just want to say you don't feel that this all blends in well, or that it is too much effort for the advantage, then just say so. Instead you make those inflaming ignorant arguments, that maybe sound reasonable at first glance but are clearly not, if you just follow the whole context.

I will explain it again for the sake of good faith:

From a usability-perspective, a property ought to be unset until the user explicitly sets it in the editor to reflect the reality of usage of the editor. It would be wrong and it is deceptive to set a factually unset property to some value, because this does not reflect the truth of the situation. Because it doesn't reflect the truth, in order to be useful and truthful to editor usage (and in an attempt to derive sensible conclusions from it), the value set as default effectively has to be used to reflect the unset state. This results in bool to be only settable to a single value (true) and it eliminates the second value (false). Even if this makes sense inside some kind of mentally limited logic of programming language syntax, in the reality of the situation, it makes bool absolutely useless. This is unless you are dealing with the special exception of a-priori knowledge about the value of the boolean before the user can set it. In this case by clever design you can perform a trick to eliminate the importance of unset properties, that arises from what is actually true to using the editor (opposed to what is true only inside whatever programming the engine currently follows internally). There are tons of situations where this is the case, and there are tons of situations where this is not the case (e.g. when the default value comes from another class). And it only leaves you with bad design and ugly workarounds, totally defeating the purpose of having types in exported variables, and basically making you use enum Strings all the time. This has in principle nothing to do with types, but only with creating user-settable properties with the @export statement, which happens to be strictly tied to the type system in the current state of the engine, which as explained is an obvious shortcoming.

Having a property reflect the truth is not dangerous, it is sensible and necessary. To the contrary having it not reflect the truth limits its usefulness and is counter-intuitive and possibly misleading. Whatever syntax @export uses in its current state, and whether or not that relates to types or is tied to types is not really important, when only considering whether or not this is useful and makes sense.

After considering this, you can consider all the different ways in which it can be realized, and whether or not those ways are worth it. Which certainly I wouldn't be very demanding and insisting about. To the contrary, if someone starts to just mindlessly pull out random stuff in order to undermine and invalidate a feature request, this is where I would take great offense and start complaining a lot. When my first suggestion is "@export var x : bool = null" as syntax, you claim "existing code would break with this change", apparently on purpose in ignorance of even the very first sentence, only because I have merely mentioned a secondary alternative later to this where this is true. Whatever you have said and commented follows the same troll-ish scheme of mischaracterization, to ignore the context, create lines of internally-consistent reasoning that are blatantly false or irrelevant to the whole situation, and then trying to make it look like my request is invalid or I have claimed false things. I mean it sounds insulting to say this, but just go over the conversation again, there is no other way for me to put it. I hope this was not actually your intention, and why I didn't just simply chose to ignore you but explain my perspective to you.

I was only suggesting to use the syntax as found in the first statement of the title, because it wouldn't break existing code, it wouldn't break compatibility, and it would seem to be the easiest and most intuitive change to accomplish the goal (albeit in a totally benign way, it is syntactically inconsistent). I assumed though that like in Godot 3 exported variables could still be variants, so if that had been true, such a change seemed really trivial to me.

But in fact when I just exported an uninitialized enum from @tool, it would show me an empty drop down menu and return indeed a null value (but not when running the game). I have similarly often experienced in the past that exported variables would return null values (don't know if only @tool or not), but then later they seemed to have been overwritten automatically to a set value, if I saved the scene or did some other action. So it would seem the assumption is not correct that exported variables can't be null due to engine limitations, and such a solution might be much easier than assumed.

Of course like a door without a door knob, in the current design state of the engine you only have types (=hinges) performing the metaphorical function of opening the door. But this is always true to virtually any situation, where a feature is missing or a feature is not flexible enough to properly reflect on the reality of using the program. So it is a non sequitur to turn this into an argument, about whether or not something makes sense, or is possible to implement, for any sort of feature. Certainly it is an important consideration. But it is not the premise of all considerations. And if made so, it can only result in circular logic, where most of all features are impossible to realize. Unless by freak chance whatever design that was already present allows for it.

AThousandShips commented 4 months ago

I'm not trolling, behave professionally and listen to people who know more than you, read the code of conduct...

CC @godotengine/code-of-conduct please take a look

AThousandShips commented 4 months ago

I approached you and helped and tried to educate you on your misconceptions and failure to understand the scope of this, and gave you ideas and solutions, but you decided to accuse me of trolling, you are the kind of people who really break my heart when working and contributing, honestly... Thankfully most people I interact with are reasonable, nice people who can behave like adults and be professional, otherwise I'd give up honestly

ballerburg9005 commented 4 months ago

I'm sorry, I do not see why exporting an integer with @export_enum is not the simpler solution to all of this. The solution provided to @dalexeev is much more ideal at all times.

Is it something to do with the current UI? Would you prefer if it were a series of radio buttons, or something else entirely? If yes, the existing @export_enum type could be improved to allow a different visualization.

But nullable bool is not it, as discussed above. If you're concerned about memory usage, all numerical Variants occupy the same space and it would be no different than an integer.

The different visualization can be done with addons, by the way. It could even be ad-hoc, and specifically tailored for your needs, as @KoBeWi provided.

Like it is now, you can't tell whether or not an exported variable has been set in the editor, and values are initialized that have not been set in the editor UI. For boolean the workaround is enum, for integer the workaround is default = -2147483648, for array the workaround is a double-array, and so forth.

Yes you can make it work this way most of the time. And often workarounds are not needed and it is advantageous to have no null values. But overall it is not an ideal situation that always works in a sensible manner.

When you make use of classes and resources like explained, you need to be able to tell if the property was set from the UI, in order to make it or not make it override the default value from the class. I think this is more important with more advanced usage and less simple design choices, that make Godot easier and more powerful.

Although you can work around the limitation easily, with this advanced use, it defeats the point of having types in exported variables in the first place. This is why I raised this issue. I mean the typing system should be enabling to type usage and not do the opposite thing.

It seems simple to me to allow null values for exported variables as an exception, and I believe the engine is already functioning in such a way as to facilitate this easily. Like I have written, new and unset exported variables do already return null values sometimes in @tool mode (e.g. initially, then somehow being overwritten) and I also loosely recall that it can happen otherwise at random in other unknown circumstances.

As discussed, this is not an issue of types, which means that nullable bool is not necessary to solve this (although it could solve this also). It is a simple matter of how @export is interpreted. Whether the statement reads as suggested previously from 3.x "@export(bool) var a" to initialize a variant to null, then provide a boolean UI element, or whether it reads "@export var x : bool = null" and ends up doing the same thing, or some other thing to the same ends, it does not really matter. The only important thing is that the variable can reflect the actual user choice in the UI and that it doesn't override this information.

ballerburg9005 commented 4 months ago

I approached you and helped and tried to educate you on your misconceptions and failure to understand the scope of this, and gave you ideas and solutions, but you decided to accuse me of trolling, you are the kind of people who really break my heart when working and contributing, honestly... Thankfully most people I interact with are reasonable, nice people who can behave like adults and be professional, otherwise I'd give up honestly

I am sorry, but I didn't just want to ignore you. I thought it was important for you to understand those problems with the conversation. I don't have the best communication skills myself.

AThousandShips commented 4 months ago

You just went in a long rant accusing me of trolling you, that's on you, and you can't just dismiss that with that. This isn't about communication skills, this is about your behaviour, and don't accuse my communication skills of creating a situation...

It honestly saddens me more that you respond in such a way to someone telling you that your attitude upsets them and that your unwillingness to be reasonable and listen instead of accusing others of trolling or lying, and that your refusal to accept when you're wrong goes so deep, yeah...

I'll let that be my last response to this and the CoC team will give their opinion

ballerburg9005 commented 4 months ago

It is not always the right place, but it is good to talk about things.

I would have written this in DM, but Github no longer has DMs ...

dalexeev commented 4 months ago

From a usability-perspective, a property ought to be unset until the user explicitly sets it in the editor to reflect the reality of usage of the editor. [...] This results in bool to be only settable to a single value (true) and it eliminates the second value (false).

Perhaps you mean that scenes/resources do not serialize a property if it's equal to default values. This has pros and cons, see godotengine/godot#86494 for details.

If you need only two possible values (true and false), but would like to "pin" the default value, then see the above issue.

But if your variable has three possible values, then bool is not suitable as the static type. This has nothing to do with export annotations, it's a feature of the Godot/GDScript type system as I explained above.

I have similarly often experienced in the past that exported variables would return null values (don't know if only @tool or not), but then later they seemed to have been overwritten automatically to a set value, if I saved the scene or did some other action.

Probably, it's a bug. Under the hood, all variables in GDScript are Variants, but they are restricted by specified types (and the type is not a part of the export syntax). In some cases you can get null for a variable typed as bool, but this is incorrect behavior and can even lead to a crash in the case of validated opcodes.

Even if this makes sense inside some kind of mentally limited logic of programming language syntax, in the reality of the situation, it makes bool absolutely useless.

This has in principle nothing to do with types, but only with creating user-settable properties with the @export statement, which happens to be strictly tied to the type system in the current state

I was only suggesting to use the syntax as found in the first statement of the title, because it wouldn't break existing code, it wouldn't break compatibility, and it would seem to be the easiest and most intuitive change

I assumed though that like in Godot 3 exported variables could still be variants, so if that had been true, such a change seemed really trivial to me.

I think it's more complicated than you perceive. In this case, we are dealing with the interaction of several systems at once (GDScript static typing, property export mechanism, scene/resource serialization, inspector UI). I think the problem you are encountering is not where you are proposing the changes. This is probably why the first post has negative reactions.

Instead you make those inflaming ignorant arguments, that maybe sound reasonable at first glance but are clearly not, if you just follow the whole context.

if someone starts to just mindlessly pull out random stuff in order to undermine and invalidate a feature request

Please be polite, patient and assume positive intentions, including when faced with criticism. If you don't understand your opponent's argument, politely ask for clarification or rephrasing rather than making accusations. Remember that development is generally quite complex and our initial understanding often turns out to be incorrect/incomplete upon further consideration.


Whether the statement reads as suggested previously from 3.x "@export(bool) var a" to initialize a variant to null, then provide a boolean UI element, or whether it reads "@export var x : bool = null" and ends up doing the same thing, or some other thing to the same ends, it does not really matter.

It's not the same thing, that's your mistake. The static type is not just metadata, it's a fundamentally different thing. Compare:

# 3.x:
export(bool) var a # a: Variant = null
export(bool) var b = false # b: Variant = false
export var c: bool # c: bool = false

func _ready():
    a = null # OK.
    b = null # OK.
    c = null # Error.

# 4.x:
# No Option A available. It might be something like:
#@export_custom_type(TYPE_BOOL) var a # a: Variant = null
# No exact Option B available. There is a similar option:
@export var b1 = false # b1: Variant = false
@export var c: bool # c: bool = false

func _ready():
    #a = null # OK.
    b1 = null # OK.
    c = null # Error.

Also, for other annotations (like @export_range) the type specifier and initial value are still optional, unlike bare @export. We're even thinking about changing this since users often expect a non-null value, see godotengine/godot#77875.

trickster721 commented 3 months ago

While it's nice to see unusual proposals being taken seriously and considered at face value (even just as a novel thought experiment), it seems like trying to avoid bluntness can sometimes result in unnecessary grief.

Booleans have two possible values, that's the definition of a boolean, and one of the most universal assumptions in computer programming. Changing that fundamental rule to make exporting radio buttons more convenient would be like repairing a ladder by changing the laws of gravity, it's a solution wildly out of scale with the problem.

Calinou commented 2 months ago

Closing due to lack of support (see comments and reactions on OP).