godotengine / godot-proposals

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

Implement setters and getters for Dictionary values #8612

Open IntangibleMatter opened 10 months ago

IntangibleMatter commented 10 months ago

Describe the project you are working on

A game with a large amount of values in the save data

Describe the problem or limitation you are having in your project

When accessing properties on the dictionary used to store the save data, there's no way to return a default value if a property doesn't exist, so a wrapper function needs to be created.

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

Setters and getters don't apply to individual items in a dictionary. Adding these to GDScript would enable writing much cleaner, more concise code.

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

Here's an example where you run a piece of code as many times as the value of foo in a dictionary, with a minimum of 1 time. With the getter you don't need to implement a safety check withDictionary.has(key), which you do in GDScript at the moment. While in this small scenario it can easily be worked around, in larger code bases it gets tedious.

var mydict := {"testval": 10}:
    get(key):
        if mydict.has(key):
            return mydict[key]
        return 1

func _ready() -> void:
    for i in mydict["foo"]:
        print(i)

As opposed to

var mydict := {"testval": 10}

func _ready() -> void:
    if mydict.has("foo"): # minor example, but these safety checks get tedious in a larger project
        for i in mydict["foo"]:
            print(i)
    else:
        print(1)

This may not be the best example, but it shows

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

It would likely be used very often, and while it can be worked around with a small wrapper function, it's not as clean and it doesn't scale as well.

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

It's part of GDScript.

AThousandShips commented 10 months ago

See:

AThousandShips commented 10 months ago

So what should happen if you reassign the property? Should it call this with an empty key? Or call it for all the changed keys? What if you remove a key?

IntangibleMatter commented 10 months ago

This is a proposal, and I figured it would be open for discussion. The solution in my mind would be adding a specific setter/getter keyword for keys and the like.

So for example:

var mydict := {"foo": "bar"}:
    set(val):
        mydict = val
    set_key(key, value):
        mydict[key] = value
    get:
        return mydict
    get_key(key):
        if mydict.has(key):
            return key
        return ""

This isn't a proposal to change the fundamental usage of dictionaries. This is an idea for how to improve QOL for dictionaries, improve safety by avoiding crashes when accessing values that don't exist, and the like.

If you want to check if a dictionary has a value you can still do dict.has(value), which would return false if the key doesn't exist, but if the getter for keys existed you could still get a value for the key without there being a crash.

AThousandShips commented 10 months ago

This is a proposal, and I figured it would be open for discussion.

Yes? That's literally what we're doing 🙃 I'm posing questions about what you propose, and pitfalls you might not have considered, which is literally part of this process 🙂

If you want to check if a dictionary has a value you can still do dict.has(value)

What is this in response to? I didn't talk about that, read what I wrote again 🙂

IntangibleMatter commented 10 months ago

I apologize, my intent with the previous comment may not have been clear.

So what should happen if you reassign the property?

If you're referring to the whole dictionary, then it would work as normal as if no dict_set had been implemented.

Should it call this with an empty key? Or call it for all the changed keys?

I'm unsure what you're referring to here.

What if you remove a key?

Removing a key should be perfectly fine. The getter could return the default value if the key isn't there, and return the value of the key if it is.

AThousandShips commented 10 months ago

I'm talking about how to process things like:

foo.my_dict.erase(key)

What method should be called?

IntangibleMatter commented 10 months ago

Assuming foo is a dictionary, and that my_dict is a dictionary within that dictionary, then my_dict would be returned as per the getter, then erase would be called on that dict.

In terms of erase, I assumed it would be used as usual, though a special kind of set or get called erase could be used, that would allow you to perform operations to erase in a special way if need be.

AThousandShips commented 10 months ago

No, foo is a class exporting it

That's what I meant, your proposal doesn't account for this, that's why I'm asking these questions

kleonc commented 10 months ago

Here's an example where you run a piece of code as many times as the value of foo in a dictionary, with a minimum of 1 time. With the getter you don't need to implement a safety check withDictionary.has(key), which you do in GDScript at the moment.

There's Dictionary.get(key, default) method fitting this specific use case (no safety check with Dictionary.has(key) needed).

IntangibleMatter commented 10 months ago

If Foo is a class exporting it, then the setter and getter of the dictionary would act as normal, as setters and getters do on other exported properties when setting, getting, or erasing.

AThousandShips commented 10 months ago

as setters and getters do on other exported properties when setting, getting, or erasing.

That's not how it works right now though, and again your proposal doesn't specify this, do you see what I'm trying to get across?

IntangibleMatter commented 10 months ago

Oh, my understanding of setters and getters was that they worked for properties that were exported as well. I'm not entirely sure I see what you're trying to get across, but I feel like there's a bit of a gap in communication on both ends.

AThousandShips commented 10 months ago

I'm saying your proposal lacks a lot of details... That's what I've been saying several times over, I'm trying to help by pointing out areas where your proposal doesn't cover things, and help you decide what to do in those cases

I'm sorry if me asking questions like "how would your idea work in X situation" wasn't obviously that 🙁

Without those details this proposal can't work 🙂

If you do foo.my_dict.erase(key) it does not call the setter, see the issue I linked above

For more details on why things work the way they do (and the current preferences for GDScript goes against this proposal) see https://github.com/godotengine/godot/issues/85578#issuecomment-1835923797, and also the suggestion in https://github.com/godotengine/godot/issues/85578#issuecomment-1847082079