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

Dictionary element are not available by index #44241

Closed
TotCac opened this issue Dec 9, 2020 · 9 comments · Fixed by #48139
Closed

Dictionary element are not available by index #44241

TotCac opened this issue Dec 9, 2020 · 9 comments · Fixed by #48139

Comments

@TotCac
Copy link

TotCac commented Dec 9, 2020

Godot version:
custom d0f1c03

OS/device including version:
Windows 10

Issue description:
Cannot find existing key in Dictionary

Steps to reproduce:
Empty project
Create node
Add script
func _ready(): var dict: Dictionary = { "elem1": 1, "elem2": 2, } print(dict) print(dict["elem1"])

Output:
{elem1:1, elem2:2}

Debug:
Invalid get index 'elem1' (on base: 'Dictionary').

Minimal reproduction project:
bugreport.zip

@pouleyKetchoupp
Copy link
Contributor

I can confirm dict["elem1"] or dict.elem1 don't work in current master. It seems to be related to failed comparison between String and StringName types.

For info, as a workaround you can use the syntax dict.get("elem1") which still works.

@AndreaCatania
Copy link
Contributor

I can confirm it too.

@lyuma
Copy link
Contributor

lyuma commented Dec 30, 2020

hit this when looking up bone names in a dictionary. since I already made it, here is a reproduction project: run dictnames.tscn
gdscriptbugs.zip

had some trouble with .get() ... I mean, it avoids a crash yes but produces the wrong result. I just worked around by iterating through all keys and checking equality.

@pouleyKetchoupp
Copy link
Contributor

@lyuma I've checked your project and dictionary accesses with get() seem to work ok.

The only clunky thing I had to do is to use nodes.get(&"Camera3D") because node names are StringName type, so nodes.get("Camera3D") would return null (that needs to be fixed too).

I'm not sure what the expected result from the scene is though. What wrong result did you get?

@lahouaridc
Copy link

lahouaridc commented Jan 12, 2021

so I have this example related to this issue (in GDScript):

	var space_state = get_world_3d().get_direct_space_state()
	
	if space_state != null:
		target_result = space_state.intersect_ray(from, to)
		if !target_result.is_empty():
			var target_position = target_result[String("position")]
			target_position = target_result["position"] #<-- ERROR 

I am not certain why is string literal converted to StringName in here instead of String

@TotCac TotCac changed the title Dictionary are broken in current build Dictionary element are not available by index Jan 13, 2021
@nemerle
Copy link
Contributor

nemerle commented Feb 2, 2021

The problem is likely related to Variant::hash_compare that is used internally to compare keys in Dictionary class.
Currently, it's possible to do:

Dictionary oh_lawd;
oh_lawd[StringName("chunky")] = "cat";
oh_lawd["chunky"] = "dog";

and end up with Dictionary of size()==2

Current Variant::hash_compare assumes that Variant::STRING and Variant::STRING_NAME are not hash-comparable:

bool Variant::hash_compare(const Variant &p_variant) const {
	if (type != p_variant.type) {
		return false;
	}

and it bails in the first conditional check.

@vonflyhighace2
Copy link

vonflyhighace2 commented Mar 17, 2021

looks as if this has hit a brick wall. Dictionary's might need an overhaul similar to what vnen is doing with Arrays.

@paulhocker
Copy link

does anyone know if this is going to be fixed/addressed? I have to think that there are many people that are expecting Dictionaries to work as they did before.

@Calinou
Copy link
Member

Calinou commented Mar 20, 2021

does anyone know if this is going to be fixed/addressed? I have to think that there are many people that are expecting Dictionaries to work as they did before.

The master branch isn't meant to be used for production, so it may take a while for this to be fixed.

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