Change dictionary var in func not working as expected

Godot Version

4.4.1

Question

I wrote a json parser with error checking and moved this code to its own script. For this to work i wanted to pass an empty dictionary var to this function, have it altered there and use it in my main code. Now when I do something like this in my script

ret = dict

The value of ret is set to the value of dict inside the function. But in the main script ret is empty.
If I instead to this

ret.msg = dict.msg
ret.content = dict.content

the value of ret is also there in the main script. Is there any better way to “copy” a dictionary var inside a function?

Following are the two template scripts im using:

extends Control


func _ready() -> void:

	var content : Dictionary = {}
	
	var parser = Parser.new()
	
	if not parser.overload("path_to_file", content):
		print("Error on Parser")
	else:
		print(content)
	
	breakpoint
	
	get_tree().quit()
	pass

class_name Parser extends RefCounted

var dict : Dictionary = {
	"msg" = null,
	"content" = null,
}

func overload(path, ret) -> bool:
	dict.msg = "Super Text"
	dict.content = "Even better Text"
	
	#NOTE: Does not work
	#ret = dict
	
	#NOTE: Does work (when line 13 is commented)
	ret.msg = dict.msg
	ret.content = dict.content
	
	
	breakpoint
	return true

Could I create an inner class within Parser that holds the dictionary (kinda like a struct definition) and use this to copy/pass it? Im starting to think im working against how gdscript works…

Why not just have your parser return either a dictionary with your data or an empty dictionary on parse or load error. Then in you script all you have to do is something like:

var content : Dictionary = parser.overload("path to file")
if content.is_empty():
     # deal with this however.
     return
1 Like

I was over eagerly trying to move most of the error handling to the function to keep the “main” function clean. And in this case I also have to end the scene if loading/parsing would fail (as I want people to use custom json files). Which would totally work with your code as well.

In the end i also boiled the code down to something like this, using a static function “json_load”. Just have to implement the return to before scene and add something better than just print:

func _ready() -> void:
	var ret : Dictionary = {}
	
	if not JsonParser.json_load(options_path, ret):
		print("Error parsing:", ret.msg)
	else:
		print(ret)
		options = ret.content
	
	pass

For completness sake here are working methods to have the passed argument remain the values after the function:

    ret.msg = dict.msg
	ret.content = dict.content
	
	for i in dict:
		ret[i] = dict[i]
	
	ret.assign(dict)
	
	ret = dict.duplicate(true)

Thank for the input.

I am not sure how the parser can ‘deal with the errors’ as the scene calling it has to ultimately deal with an erroneous file path. I mean you don’t want a user to experience a halt to the code or a crash.

You could return a dictionary with an error code in it like this:

func load_json_file(requested_file_path: String) -> Dictionary:
	var error_message: String = ""
	if not FileAccess.file_exists(requested_file_path):
		error_message = "Requested file does not exist: %s." % requested_file_path)
		return {"error_msg": error_message}
	var json_as_text: String = FileAccess.get_file_as_string(requested_file_path)
	var json_as_dict: Dictionary = JSON.parse_string(json_as_text)
	if json_as_dict == null:
		error_message = "Failed to parse JSON file: %s." % requested_file_path)
		return {"error_msg": error_message}
	return json_as_dict

Then in your script something like:

var content : Dictionary = parser.load_json_file("path to file")
if content.has("error_msg"):
     # deal with this however. Show it to user etc.
     print(content["error_msg"])

Anyway, as long as your solution works for you. There are lots of ways to approach this.

PS I don’t really understand what you are doing with your ret with copying the indices one by one, then assigning the properties, then assigning a duplicate with a deep copy. Perhaps it is just beyond my level, but it seems a lot of work to simply get a dictionary from another dictionary.

PPS I guess you are doing some complex stuff in between which you have simplified down for the example in your question. Otherwise I would have thought just duplicating would have been enough to copy the dictionary.

1 Like

To just illustrate the “problem” I had with the passed argument not keeping its value after the function call I made a new project with just the bare minimum. Which are the two scripts i posted in the starting post. The actual function json_load has some more lines. Kinda like the ones you have posted. (Will dump it at the end of the post here).
Putting the error message into the return value is a neat idea. And makes the code self understanding.

The parser only deals with the errors of the file access and json parsing. But also makes the message in case of an error look nice and be as complete as possible. Just a thrive for perfection i sometimes get caught up in.

Regarding your Post Scriptum: I just wrote down all possibilities to achive the same thing. Thus only one of those should be used. This is just me not making a good job at writing what i mean. PPS just sums it :slight_smile:

Here is the complete json_load function I came up with:

class_name JsonParser extends RefCounted

static func json_load(path : String, ret : Dictionary) -> bool:
	var json_ret : Dictionary = {
		"content" = null,
		"msg" = null,
		"ret" = true,
	}
	
	if FileAccess.file_exists(path):
			#INFO: Might change when exported. Then use ResourceLoader
			var content = FileAccess.get_file_as_string(path)
			
			if content.is_empty():
				var o_er = FileAccess.get_open_error()
				json_ret["msg"] = str(error_string(o_er), " '", path, "'")
				json_ret["ret"] = false
				
			else:
				#INFO: We read file now handle json
				var json = JSON.new()
				var j_er = json.parse(content)
				if j_er == OK:
					json_ret["content"] = json.data
				else:
					json_ret["msg"] = str("JSON Parse Error: ", json.get_error_message(), " in file '", path,  "' at line ", json.get_error_line())
					json_ret["ret"] = false

	else:
		#INFO: Path does not exists
		json_ret["msg"] = str("Could not find file '", path, "'")
		json_ret["ret"] = false

	#ret["content"] = json_ret["content"]
	#ret["msg"] = json_ret["msg"]
	ret.assign(json_ret)
		
	return json_ret["ret"]

@soupstitute I recommend trying @pauldrewett 's solution. Your solution is overly complex and honestly not good coding practice. It hides the fact that you’re passing by reference something that is going to have its value changed. Your code is going to be much cleaner and easier to debug if you assign the return value to content.

Also, ret is a very confusing variable name.

You’re definitely working against how GDScript works. TBH your code looks like really old C pointer dereferencing code.

Personally, even though Godot supports inner classes, I do not think they’re really helpful in GDScript. It makes the structure of your code harder to read and AFAIK, it doesn’t improve performance at all. I find it annoying enough to deal with in the Godot engine code itself in C.

This is because you are over-optimizing your code before getting it working. Specifically, you are using a RefCounted, which cleans itself up as soon as it’s no longer being used. So, you call it, it returns, thinks it’s done and is killing itself. Godot passes by reference, and you are losing the reference to the variable because you’re destroying it -potentially while it’s being passed.

Also, JsonParser should probably be a singleton - which in Godot means making it an Autoload. That also means just inheriting a good ole Node.

Here’s the code for my version of your JsonParser class:

extends Node

const SETTINGS_PATH = "user://configuration.settings"
const SAVE_GAME_PATH = "user://game.save"

## If this value is On, save_game() will be called when the player quits the game.
@export var save_on_quit: bool = false

var configuration_settings: Dictionary
var game_information: Dictionary
var is_ready = false


func _ready() -> void:
	if FileAccess.file_exists(SETTINGS_PATH):
		configuration_settings = _load_file(SETTINGS_PATH)
	ready.connect(func(): is_ready = true)


func _notification(what) -> void:
	match what:
		NOTIFICATION_WM_CLOSE_REQUEST: #Called when the application quits.
			if save_on_quit:
				save_game()


## To add a value to be saved, add a node to the "Persist" Global Group on
## the Nodes tab. Then implement save_node() and load_node() functions.
## The first function should return a value to store, and the second should
## use that value to load the information. Make sure the load function has a
## await Signal(self, "ready") line at the top so it doesn't try to load values
## before the node exists. If you need to store multiple values, use a
## dictionary or changes later will result in save/load errors.
func save_game() -> bool:
	var saved_nodes = get_tree().get_nodes_in_group("Persist")
	for node in saved_nodes:
		# Check the node has a save function.
		if not node.has_method("save_node"):
			print("Setting node '%s' is missing a save_node() function, skipped" % node.name)
			continue
		
		game_information[node.name] = node.save_node()
		print("Saving Info for %s: %s" % [node.name, game_information[node.name]])
	return _save_file(game_information, SAVE_GAME_PATH)


func load_game() -> void:
	game_information = _load_file(SAVE_GAME_PATH)
	if game_information.is_empty():
		return
	var saved_nodes = get_tree().get_nodes_in_group("Persist")
	for node in saved_nodes:
		# Check the node has a load function.
		if not node.has_method("load_node"):
			print("Setting node '%s' is missing a load_node() function, skipped" % node.name)
			continue
		# Check if we have information to load for the value
		if game_information.has(node.name):
			print("Loading Info for %s: %s" % [node.name, game_information[node.name]])
			node.load_node(game_information[node.name])


## To add a setting to be saved, implement save_setting() functions. It should
## return a value to store. If you need to store multiple values, use a
## dictionary or any later changes will result in save/load errors.
## Call this function using Setting.save_setting(self)
## NOTE: The node name must be unique or settings will get overwritten.
func save_setting(data: Variant, category: String) -> void:
	configuration_settings[category] = data
	_save_file(configuration_settings, SETTINGS_PATH)


## To add a setting to be loaded, implement a load_setting() function. It should
## use the passed value(s) to configure the node. Make sure the load function
## has an await Signal(self, "ready") line at the top so it doesn't try to load 
## values before the node exists.
## Call this function using Setting.load_setting(self)
func load_setting(category: String) -> Variant:
	if !is_ready:
		if FileAccess.file_exists(SETTINGS_PATH):
			configuration_settings = _load_file(SETTINGS_PATH)
	if configuration_settings.has(category):
		return configuration_settings[category]
	return ERR_DOES_NOT_EXIST


## Takes data and serializes it for saving.
func _serialize_data(data: Variant) -> String:
	return JSON.stringify(data)


## Takes serialized data and deserializes it for loading.
func _deserialize_data(data: String) -> Variant:
	var json = JSON.new()
	var error = json.parse(data)
	if error == OK:
		return json.data
	else:
		print("JSON Parse Error: ", json.get_error_message(), " in ", data, " at line ", json.get_error_line())
	return null


func _save_file(save_information: Dictionary, path: String) -> bool:
	var file = FileAccess.open(path, FileAccess.WRITE)
	if file == null:
		print("File '%s' could not be opened. File not saved." % path)
		return false
	file.store_var(save_information)
	return true


func _load_file(path: String) -> Variant:
	if not FileAccess.file_exists(path):
		print("File '%s' does not exist. File not loaded." % path)
		var return_value: Dictionary = {}
		return return_value
	var file = FileAccess.open(path, FileAccess.READ)
	return file.get_var()

I call it Disk. I made it a plugin. It you want to try it out, you can take a look here and get a copy here: GitHub - dragonforge-dev/dragonforge-disk: An Autoload singleton to handle saving and loading of game data

1 Like

Thanks for pointing out my age to me :slight_smile:

To be fair thats why I wrote it like this. Using this approach to many times with great success. Also C syntax would make the pass as reference more obvious. Anyways ret is as close to return as nill is to null.

I have to deny the claim of over-optimization, thus cant bring any evidence.
The sepuku of the RefCounted is not the problem. When using other means instead of simple assigning one Dictionary to another it works on the return.

I did not want to use an autoload, cause I dont need it project wide. Its only in this specific scene and by using just a script I can have it remain self contained.

All in all both of you are totally right as I did not adhere to gdscript principles. But my lesson on static function and variables is done with this journey starting from pass by reference to get bitten in two month when you read your own code.

I think I already saw the script you posted. Might be on your page or from a net search.

np, but to be fair, it was the ret variable name that really says, “I learned to code when variable names had to be short.”

I get that, but you asked about the GDScript way. IMO, you’re trying to force an approach that isn’t needed. You’re not passing a pointer, so you’re not saving anything by doing it.

Which raises the question, why not use the .NET version and do it in C# or write the code in C++ and use GDExtension?

Yes but return_value is a lot easier to read. Especially when you’re posting code for other people to read. Honestly, I was trying to figure out if you’d copied the code from somewhere else, gotten it from an LLM, or were just - you know - really old like me.

Well, it was an educated guess. I’ve never seen a return value crap out on itself like that before.

So why not just make it part of the script that uses it instead of a whole separate object?

In the end, it’s whatever works. My GDScript architecture has changed a lot since I started using Godot about 3 years ago. I really love the language though. It reminds me a lot of the accessibility of Ruby (though it is much more accessible than the Ruby game engine.) It is a modern language, and I think ultimately that’s where my criticism of your code and accusation of over-optimization comes in - because your code appears to be using it as if it were not a modern language.

When I started using Godot, I also used RefCounted for things. Since then I have found that there’s almost always a reason for me to use Node or Resource instead as the base class. So, now I start there and optimize down if I need to.

Recently I’ve been working on a multiplayer game and realized that RPCs cannot pass even Object or RefCounted, and so I’m turning everything into a Dictionary. From a convenience point of view, it’s easier for me to make something a Resource so I can edit it easily in the editor and then turn it to/from a dictionary for passing.

GDScript is meant to be a higher level language and handle some things we are used to taking care of ourselves. It takes advantage of duck typing, but allows strict typing. It prefers composition over inheritance, but it allows you to make whatever choices you want. (As of 4.5 beta 1 is supports the Abstract keyword.) And it only passes by reference, not pointer. :wink:

Cool, glad it’s getting out there. :slight_smile:

1 Like

Jup and that parts me from a learned/well versed programmer. I still follow ways I gobbled up from (old) “high level” languages and dont adopt or really learn the intended ways of new languages. Something im trying to change.
Also passing by reference and passing a pointer always was the same for me. You also mention that when it comes to the Abstract keyword. Besides there is something tickeling the back of my brain that its not quite the same.

GDExtension is next on the list. But as I have no need for it now i try to stick with gdscript and have a proper understanding of it. This also brings up the use of RefCounted. To my understanding its (one of) the “lowest” base class which should bring the least “size”. Here goes the over-optimization.

The next quotes go with my full comprehension. Just let me say this was all me fillding around, finding something I did not understand, creating a bare minimum of example code and then you guys set me head straight. Which im greatfull for. It all just started as a learning experience for me and got me so much further. Albeit setting me back on realising what i still have to gasp when using GDScript.

As your criticism of the code comes from a person that went down the road i have glimpsed into today, I will tread more carefully and reread the documentation on the base classes. I guess there is a reason when creating a new script the default class is Node. Sometimes its tempting to look into corners of a language and see if it would work. I had the freedom to use what ever my mind came up with due to not working with RPcalls. Curse and a blessing it is.

1 Like

I picked Pauls second reply as a solutions cause it fits best and has the least lines of code.

As notes for me and others there is a subtle difference between “pass by reference” and “pass by pointer” at least in the C/C++ language context. Besides the grammar, pointers can be NULL while reference(s) can not be NULL.

Case closed. :stuck_out_tongue:

1 Like