godotengine / godot-proposals

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

Add static type hints for dictionary members #56

Closed ghost closed 1 week ago

ghost commented 5 years ago

Describe the project you are working on: Cutscene-heavy RPG

Describe how this feature / enhancement will help your project: I want to associate some object with some identifier to make it easy to reference them from the AnimationPlayer (in this case it's some Dialogue class with data for who is talking and what they're saying.) I would like to access this data using the format read_dialogue(id: String), where the id is the key in a dictionary that leads to the data. This causes my extended AnimationPlayer to pause the animation so it can autotype some text and wait for player input before moving on with the animation.

However, there's currently no way to define a dictionary to have any particular kinds of keys and values, so if, for example, I wanted to add 50 different lines of text to read in this animation, I'd have to set the key's type and the value's type manually every time, which is tedious.

image

Furthermore: without dictionary hints, it's not possible to add a hint to a particular kind of Object, since the list of types you can pick from is limited to built-ins and various primitive types. Contrast this to arrays, where you can use hints to specify just about any type not present in the drop-down. So, you're not going to be able to easily create resources in-place, resulting in the OmniResource issue (except worse since it's every object ever.)

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

export(Dictionary, String, Resource) var hinted_dictionary

64645509-ab31a700-d3da-11e9-8d57-f0ca866362b2

If this enhancement will not be used often, can it be worked around with a few lines of script?: I think just about everyone will be very pleased with this addition; however, in the meantime, it's not too bothersome to avoid the issue. Instead I'll give my class an id: String property and loop through an array. I assume this isn't as efficient as using dictionaries, but I likely won't notice a real performance hitch. Nonetheless, it's a shame exported dictionaries can't be more useful.

https://github.com/godotengine/godot/issues/25157

holywar20 commented 1 year ago

Just some more food for thought on this issue - ResourceSaver seems to depend on type hints for nested objects to get things to load correctly. A custom class_name saves as just 'Resource' unless that resource is hinted. If I'm storing that resource in a dictionary, it has no idea what the type is when it tries to reconstruct the object. Which means I need to add complex construction/deconstruction logic to nested objects when saved and loaded to ensure the correct type gets assigned. I use dictionaries of custom objects a lot for easy look-ups. It makes ResourceSaver not quite as reliable as I would like, and that coupled with it being really difficult to debug an incorrectly loaded complex object, I'm sort of leaning towards rolling my own json or DB based save at this point.

Def not a feature I need for anything but this purpose, I normally don't even bother type-hinting dictionaries, but it's actually essential for saving and loading non-trivial and complex objects.

Gnumaru commented 12 months ago

Would structs https://github.com/godotengine/godot-proposals/issues/7329 supersede this proposal?

Calinou commented 12 months ago

Would structs #7329 supersede this proposal?

Structs and typed dictionaries have different purposes:

Structs are good when you want to define something such as a method configuration or complex return value. On the other hand, typed dictionaries are more suited for large sets of data with little to no nesting (e.g. a dictionary that maps item names to colors).

I believe we should support both.

PrinceMerluza commented 11 months ago

Would structs #7329 supersede this proposal?

Structs and typed dictionaries have different purposes:

  • Structs define varied types in a single data structure.
  • Typed dictionaries define a single type for keys and values.

Structs are good when you want to define something such as a method configuration or complex return value. On the other hand, typed dictionaries are more suited for large sets of data with little to no nesting (e.g. a dictionary that maps item names to colors).

I believe we should support both.

We should. We could then have a dictionary that maps string to a struct. Oh, the confidence in the data structures that we do. Imagine being able to sleep well because GDscript has this level of type safety.

bitbrain commented 11 months ago

Once this proposal is implemented, this code should work:

var dict_a:Dictionary[String,String] = {"Hello":"World"}
var dict_b:Dictionary = {}
dict_b.assign(dict_a)
print(dict_b["Hello"]) # prints 'World'

and this one should not:

var dict_a:Dictionary[String,String] = {"Hello":"World"}
var dict_b:Dictionary[String,Integer] = {}
dict_b.assign(dict_a) # unable to assign because types are different

Is this assumption correct?

nullism commented 11 months ago

Regardless of syntax, I'd truly appreciate this feature.

Happy with any of the syntax suggested:

@export var dict : Dictionary<String, int>
@export var dict : Dictionary{String, int}
@export var dict : Dictionary[String, int]

Alternatively, a DictionaryElement base class that could be extended for editors with custom key and value types. Or even a new type, say TypedMap or something. Just some way, any way, to tackle this problem without writing an editor plugin for each type.

Frontrider commented 11 months ago

@bitbrain , yes I think that is how it has to work, else we need type conversion rules. Which we must not have.

CantyCanadian commented 10 months ago

Would such changes also apply to C#? Considering dictionaries are statically typed by default, I assume that the "fixing the inspector to reflect static typing" part would bubble up to the C# layer as well.

Shadowblitz16 commented 10 months ago

Can We get this for 4.2 or is it too late? I would like to use this for mapping scenes to string names so I can get around the cylindrical scene references bug

as for what it would look like... GDScript

var map : Dictionary[string, PackedScene]

C#

var map = new Dictionary<string, PackedScene>();
Gnumaru commented 10 months ago

Any new feature can only be added before the feature freeze, which ends with beta1 if I'm not mistaken. Since RC1 is already around the corner it is too late.

(...) cylindrical scene references bug

What is this? Is there an open issue about this?

mrpedrobraga commented 9 months ago

Any new feature can only be added before the feature freeze, which ends with beta1 if I'm not mistaken. Since RC1 is already around the corner it is too late.

(...) cylindrical scene references bug

What is this? Is there an open issue about this?

I imagine they are talking about the problem described in (https://github.com/godotengine/godot-proposals/issues/8483) and mistyped "cyclical" as "cylindrical."

The Dictionary would be used as a database of StringNames to PackedScenes.

This would be awesome to edit from the editor, for these cases specially (having databases of Resources)

squk commented 9 months ago

I've made a plugin that supports type hints for dictionaries! It currently only supporter string keys, but all Variant values work. The only downside for some people might be that it `extends Resource

https://github.com/squk/godot-dictionary-resource

Polatrite commented 9 months ago

I would greatly benefit from this feature. 👍

AThousandShips commented 9 months ago

@Polatrite Please don't bump issues without contributing significant new information. Use the :+1: reaction button on the first post instead.

StatisMike commented 7 months ago

Reading through the discussion there I came to a conclusion, that the static-type Dictionary isn't that good of a concept, and introducing such could be suboptimal and confusing.

Current dictionaries are powerful because every value can be of different type, so the key-value type declaration wouldn't really work in that context. The next questions asked would be to introduce mixed types declaration (or declarations like key Y should have X type - which is implemented in @squk plugin already).

IMO the best solution would be to introduce new type, like HashMap (key needs to be hashable, which many types already implements), where the type of both key and value would have fixed types. That way we could have properly typed key-value collections.

Additionally, from what I've seen in the source code there is actually something like HashMap implemented, but used only internally. For public usage some additional work would be needed for sure, but the foundations are already there.

It also have additional benefit for GdExtensions - many of statically typed languages support something similar to a HashMap. Having the separate type that have (at least to some extend) guarantee for type-safety on Godot's side may streamline the work on the GdExtension developers side.

TLDR:

BlackDragonBE commented 7 months ago

@StatisMike Hard disagree. Static dictionaries should work the same as Array, which means you can use it to store Variants until you decide to make it static, at which point it only accepts the type you defined. A static Array looks like this:

var my_array : Array[Node]

Here's what a static Dictionary should look like IMHO to keep the syntax similar:

var my_dictionary : Dictionary[String, Node]

That doesn't seem confusing to me at all. It keeps the existing flexibility, but gives you the choice to make it static when required. Adding another type just to add static typing seems silly to me.

StatisMike commented 7 months ago

@BlackDragonBE I agree with the take on static-typed array - it doesn't make sense to store X at the n position, and Y at the n+1 position (or would be just a very questionable design), so the Array[T] declaration was a great choice.

I don't think that Dictionary is as unequivocal, and it can have its use to expect T1 at K1, T2 at K2 end etc (as in linked plugin). On the other hand there seems to have been some work already done on C# representation of Dictionary to make it more like a HashMap than Named List. I don't agree wholeheartedly with that decision, but it makes sense to venture in that direction with these steps already done.

SquidgySapphic commented 7 months ago

@StatisMike I don't see why you believe it would be any more confusing than statically-typed arrays. You can still use varying-type Arrays and static-type Arrays in the engine with that style of syntax, so why does expanding that feature to a Dictionary cause confusion?

I'm possibly off the mark, but introducing HashMap as a GDScript-level structure only serves to add an extra definition of something that doesn't really address the issue completely - if you were suggesting something that would serve as a fix for the syntax of Dictionary[varying_type, static_type] in lieu of GDScript not currently providing a way to explicitly type-hint something as Variant then I could get behind that, but that's not what I'm assuming you mean.

mrpedrobraga commented 7 months ago

@StatisMike (& @SquidgySapphic)

Also, well, strong typed languages using hash maps have them have strongly typed, too. Using them like this, they are efficient storage/retrieval structures, where you want to store data of some type.

use hashbrown::HashMap;

fn main() {
    // this dictionary is my baby boy and i love him
    let mut m = HashMap::<&str, i32>::new();

    m.insert("A", 3);
    // look at him gooo
    dbg!(m.get("A"));
}

Allowing Dictionaries to be typed in this way can provide both more type hints for a smoother/safer programming experience, but might drastically improve the performance of Dictionary in necessary cases by removing the need for all the indirection and type checks.

Variant Type can perhaps be achieved by Dictionary[ String, ... ] or Dictionary[ ..., Transform2D ]. Perhaps not, but whatever it'd look like isn't a conversation about whether typed dictionaries should be a thing or not.

Frontrider commented 7 months ago

@StatisMike as it was stated previously, dictionaries are meant to be the exact same thing as a hashmap by design, but the explicit typing is missing.

The one "derailment" is that people use this hashmap as a struct.

Kiaazad commented 7 months ago

I'm adding a simple loot table to my game and an array with nested dictionaries can make my life easier. Something like: @export_dict_array[ { "item":Item, "weight":int } ] This way every item I add comes with the two item and weight keys already set, and there's no chance for typos. I imagine the simple dictionary version would be: @export_dict{ "key_1":int, "key_2":int }

cy1mat1auto commented 6 months ago

I don't see why something like [Export] Godot.Collections.Dictionary<string, Texture2D> shouldn't work in editor; it's already supported in code, where it's least useful.

I want to create a custom resource with character names as keys and a picture of the character as values. Without static type keys and values, I have scroll through every possible type of value to reach the bottom of the list to the "load" option in order to load a single texture2D for a single key:value pair. That's not sustainable for more than one or two textures, so dictionaries are essentially unusable for anything other than storing a few int/float/string pairs constructed in code.

One of the key purposes of dictionaries is to store MANY things.

TrickyFatCat commented 6 months ago

@StatisMike Hard disagree. Static dictionaries should work the same as Array, which means you can use it to store Variants until you decide to make it static, at which point it only accepts the type you defined. A static Array looks like this:

var my_array : Array[Node]

Here's what a static Dictionary should look like IMHO to keep the syntax similar:

var my_dictionary : Dictionary[String, Node]

That doesn't seem confusing to me at all. It keeps the existing flexibility, but gives you the choice to make it static when required. Adding another type just to add static typing seems silly to me.

Sounds great. It will give a choice and keep consistency.

duchainer commented 6 months ago

@mrpedrobraga, I think that we might not even need special case syntax here: Dictionary[ String, Variant ] or Dictionary[ Variant, Transform2D ] should look and work fine. as the default type of an "untyped" Dictionary would be Dictionary[ Variant, Variant ] But I might be bike-shedding here :sweat_smile:

chexmo commented 6 months ago

I kinda undestand the logic behind doing sth like Dictionary[Variant, Node2D] BUT I think sth like Dictionary[ _, Node2D] fits better with the GDScript style guide.

chrisl8 commented 6 months ago

Would structs #7329 supersede this proposal?

Structs and typed dictionaries have different purposes:

* Structs define _varied_ types in a single data structure.

* Typed dictionaries define a _single_ type for keys and values.

Structs are good when you want to define something such as a method configuration or complex return value. On the other hand, typed dictionaries are more suited for large sets of data with little to no nesting (e.g. a dictionary that maps item names to colors).

I believe we should support both.

If/when this and/or structs are added, something very similar to the above text should be added to the Godot documentation. It greatly helps to clarify when to use each.

produno commented 6 months ago

I kinda undestand the logic behind doing sth like Dictionary[Variant, Node2D]

BUT I think sth like Dictionary[ _, Node2D] fits better with the GDScript style guide.

We already use Variant in several other places, like parameters in functions for example. So i don't see why it would be any different here.

Frontrider commented 6 months ago

BUT I think sth like Dictionary[ _, Node2D] fits better with the GDScript style guide.

I don't think that is a good idea, because underscore may be used to mark an "unused" argument in some languages. Even at the best day this would not be an unused argument.

Spartan322 commented 6 months ago

Array[Variant] already does that, its already part of GDScript intrinsically, otherwise one is making the argument for Array[_] which makes no sense, plus this is type-specific, so you need an "any" type, which Variant already serves in GDScript like Object does in C# and Java.

chexmo commented 6 months ago

Array[Variant] already does that, its already part of GDScript intrinsically, otherwise one is making the argument for Array[_] which makes no sense, plus this is type-specific, so you need an "any" type, which Variant already serves in GDScript like Object does in C# and Java.

In the case you need Array[_], you are better doing just Array in GDScript (without square brackets), which is cleaner to the sight IMHO and you still don't have the specific type you need, tho.

chexmo commented 6 months ago

It took me a while, but i've read all the thread to know what are we discussing at this point (After 2 years of participating in this thread).

What I feel it's clear at this point is that Dictionary is a key-value system, and not a struct-like one. If you need a struct, a Resource is enough for most cases.

Then, we need a disambiguation here...

In the case of Godot.... if the feature is plugged in the editor both the inspector and all languages (GDScript, C#, and other through connectors) could benefit from it. Maybe this is enough for most of the non-GDScript users here, and it's a reasonable feature to refine and deliver.

In the case of GDScript, the problem with dictionaries and adding typing-hints to be type safe in GDScript is that you may need only keys or only values to be typed... There is no problem with Dictionary[String, Resource] because types are clear. The problem seems to appear when one of the types are not intended to be strictly typed. If the verdict of the developers decides that Dictionary[Variant, Resource] is the best way... I'll believe in them and use that as it is in the future, which is in fact what we actually need, the feature.

However, as long as it seems to be open for discussion what syntax we should use.... My grain of salt is "please don't use Variant for variants"... Why? There's a funny quote of Uncle Bob that I like a lot that says "People keep coding Fortran in many other modern languages"... GDScript is the possibility of offering a modern spiced language that adapts specifically to the Godot gamedev needs and still look so good people says "I want to write code with this too" when they see me code. I believe a refreshing syntax will emerge. Maybe _ isn't the best wildcard... How about using ? as a wildcard for variants? In some languages like Java you can use it to tell the compiler you don't want to specify a type in certain scenarios. Maybe we don't want wildcards at all... isn't any a keyboard in GDScript? And if at final you decide to keep with Variant, go ahead. Users who are here need the feature.

Spartan322 commented 6 months ago

@chexmo You're looking for #7329, this is only for type-strong dictionaries. Also is not a type in any language and GDScript it absolutely would make no sense, Variant is the type that already does what you want to do inherently, that's not what placeholder variable names are for, even in C# and JS _ has never referred to a type, its a variable name that means "I don't care about this variable's value".

speakk commented 6 months ago

It took me a while, but i've read all the thread to know what are we discussing at this point (After 2 years of participating in this thread).

What I feel it's clear at this point is that Dictionary is a key-value system, and not a struct-like one. If you need a struct, a Resource is enough for most cases.

Then, we need a disambiguation here...

* One thing is the feature to bring full typed dictionaries support to Godot (if it already hasn't yet).

* Other thing is to add typed dictionaries to GDScript language.

In the case of Godot.... if the feature is plugged in the editor both the inspector and all languages (GDScript, C#, and other through connectors) could benefit from it. Maybe this is enough for most of the non-GDScript users here, and it's a reasonable feature to refine and deliver.

In the case of GDScript, the problem with dictionaries and adding typing-hints to be type safe in GDScript is that you may need only keys or only values to be typed... There is no problem with Dictionary[String, Resource] because types are clear. The problem seems to appear when one of the types are not intended to be strictly typed. If the verdict of the developers decides that Dictionary[Variant, Resource] is the best way... I'll believe in them and use that as it is in the future, which is in fact what we actually need, the feature.

However, as long as it seems to be open for discussion what syntax we should use.... My grain of salt is "please don't use Variant for variants"... Why? There's a funny quote of Uncle Bob that I like a lot that says "People keep coding Fortran in many other modern languages"... GDScript is the possibility of offering a modern spiced language that adapts specifically to the Godot gamedev needs and still look so good people says "I want to write code with this too" when they see me code. I believe a refreshing syntax will emerge. Maybe _ isn't the best wildcard... How about using ? as a wildcard for variants? In some languages like Java you can use it to tell the compiler you don't want to specify a type in certain scenarios. Maybe we don't want wildcards at all... isn't any a keyboard in GDScript? And if at final you decide to keep with Variant, go ahead. Users who are here need the feature.

Imo this is not the thread to be discussing renaming variants to ? or _. This is about typed dictionaries. Whether variants get renamed at some point is not relevant to this topic.

(I'm saying this because this would be a huge improvement to gdscript, and imo we don't need any more derailment in this thread)

monkeez commented 5 months ago

Can We get this for 4.2 or is it too late? I would like to use this for mapping scenes to string names so I can get around the cylindrical scene references bug

as for what it would look like... GDScript

var map : Dictionary[string, PackedScene]

C#

var map = new Dictionary<string, PackedScene>();

Would this proposal solve this issue about not being able to use a PackedScene as an exported Dictionary value? Or is that something else entirely?

I was surprised to find that I couldn't add PackedScenes as exported Dictionary values already, as I was trying to make a simple database to help with loading scenes based on certain keys.

shinspiegel commented 4 months ago

Can We get this for 4.2 or is it too late? I would like to use this for mapping scenes to string names so I can get around the cylindrical scene references bug

as for what it would look like... GDScript

var map : Dictionary[string, PackedScene]

C#

var map = new Dictionary<string, PackedScene>();

After looking into the typed array I imagined that dictionaries would use the same pattern and this was already implemented.

If I may ask, how can we help on this feature?

AThousandShips commented 4 months ago

At this time there isn't really anything that needs doing from any contributor, we're in feature freeze for 4.3 so it won't be merged right now, but hopefully it can be reviewed early in the 4.4 release cycle, it's a large feature so it's been hard for reviewers to find the time to focus on it properly

Though testing the feature is always welcome, you'd need to check out the PR and rebase it and compile it yourself

Edit: Though I recall there has been some discussion on what might need to be figured out before merging that might still be important, like the way generics works in GDScript and how to resolve that, things like casting typed arrays etc., which is currently an issue, and there's been discussion on whether to fix those first and then merge the typed dicts or fix that after merging

btarg commented 1 month ago

Can We get this for 4.2 or is it too late? I would like to use this for mapping scenes to string names so I can get around the cylindrical scene references bug as for what it would look like... GDScript

var map : Dictionary[string, PackedScene]

C#

var map = new Dictionary<string, PackedScene>();

After looking into the typed array I imagined that dictionaries would use the same pattern and this was already implemented.

If I may ask, how can we help on this feature?

As a newbie to Godot I was very surprised to almost immediately learn that GDScript doesn't have this. I'm so used to being able to have statically typed key-value pairs in languages like Java; they're extremely useful! +1 for this suggestion

Mantissa-23 commented 2 weeks ago

@AThousandShips with Dev1 released for Godot 4.4, I noticed this feature was missing- I gotta say it's probably my most anticipated feature right now just for the inspector ergonomics/correctness.

Would you happen to know the status of this?

AThousandShips commented 2 weeks ago

I'm not in any loops around this feature so I can't really respond sorry