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

Modify Array type to encode values type, if possible, when serialized #162

Closed
wants to merge 4 commits into from

Conversation

nkrisc
Copy link
Contributor

@nkrisc nkrisc commented Dec 13, 2023

Description

This PR modifies the Array type to encode the type of its values when it is serialized. I spent quite a lot of time trying to solve the issue I reported in #159, but could not do so without significantly altering the loading process.

So this is the alternative I am proposing.

When parsing serialized Array data, if it includes the new keys:

  1. If the supplied settings have a type value, that type will be used to parse the values.
  2. If no settings are supplied, the type value present in the stored data will be used, unless it is UndefinedType
  3. If the stored type is Undefined and no settings are supplied, the values will be passed alone as-is
  4. If the new keys are not present, the old (existing) logic is used.

When writing an Array type, a new format is used that includes the type of the values. If the value types are mixed, it will be "undefined". This should update old data to the new format when it's next saved.

Finally, I fixed an error that resulted from trying to look up a non-empty type name that wasn't a valid type.

Addressed issues

@@ -60,6 +60,8 @@ func allow_nesting() -> bool:
static func lookup(name:String) -> PandoraPropertyType:
if name == "":
return UndefinedType.new()
if not ResourceLoader.exists("res://addons/pandora/model/types/" + name + ".gd"):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should avoid using absolute paths, can you make this a relative lookup? (in other places too if applicable)

In case this turns out to be difficult, I am happy to keep it absolute for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it and unfortunately it doesn't seem possible for the moment (at least not trivially).

  • DirAccess.open requires an absolute path: "Static methods only support absolute paths (including res:// and user://)." docs
  • load also requires an absolute path: "Important: The path must be absolute. A relative path will always return null." docs
  • ResourceLoader.load doesn't specify but after testing it seems to be the case as well.

It seems that lookup and get_all_types are implicitly covered by some existing tests (because I broke them when trying) but I could add some tests for PandoraPropertyType.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best way to do this is by taking the resource path of the existing class and using it as the root dir for all loading.

ie.

        var klass = PandoraPropertyType
	var types_dir = klass.resource_path.get_base_dir()
	print(types_dir)
	var script_type = types_dir + "/" + name + ".gd"
	print(script_type)

Prints
res://addons/pandora/model
res://addons/pandora/model/int.gd

@bitbrain
Copy link
Owner

One more thing: could we set up an array like this in the demo project so we can see how it is stored in data.pandora?

@@ -146,6 +146,18 @@ func test_array_property_wrong_type() -> void:
new_property.load_data(property.save_data())
assert_that(new_property.get_default_value()).is_equal("")

func test_array_property_custom_parsers() -> void:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably also want a test that verifies that when the array is saved to json and then re-read, that the type is still there and not lost somehow. Could we extend one of the other existing tests (in entity backend) that deal with that sort of use case by adding a typed array there?

@nkrisc
Copy link
Contributor Author

nkrisc commented Jan 13, 2024

Quick status update. Had to take a brief hiatus from my game project (and this issue by extension) due to other priorities.

Here's where things lay:

  1. I believe the current state of the PR addresses the root issue, however
  2. Due to the nature of the issue, existing serialized array properties for types that require special deserialization won't be properly deserialized, so the fixes implemented in this PR can't catch them. New properties created with this fix in place will be fine.
  3. I am working on a way to try and catch these pre-fix properties and fix them the next time they're loaded with this fix in place. This will likely need to occur at a stage in the loading process when the PandoraProperty objects themselves are fully loaded as it will be necessary to read their array type setting and get the correct contents from each entity's override of the property.
  4. Need to finish adding additional tests, particularly for point 3 if I can get that working.

@bitbrain
Copy link
Owner

@nkrisc did you manage to progress on this?

@imothee
Copy link
Contributor

imothee commented Mar 4, 2024

Since we're not passing back typed arrays (since the typing would require an inherited protocol which would still require casting the array anyway) I feel like it would be simpler to just write out the type into a sub-dictionary against each key. This has the nice effect of being upgrade path since it will just treat previous non-dict values as existing types so the current state is maintained but all new values are written as type dicts.

I'd be happy to take over this PR and flesh out my below pseudocode and add some tests. Will work on it tonight.

read_value

     if variant is Dictionary:
		var array = []
		var dict = variant as Dictionary

		for i in range(dict.size()):
			var value_dict = dict[str(i)]

			if value_dict is Dictionary:
				var value_type = value_dict["type"]
				var value = value_dict["value"]

				var type = PandoraPropertyType.lookup(value_type)
				if type is not null:
					value = type.read_value(value)
					array.append(value)
					continue
			
			array.append(value_dict)

write_value

var dict = {}
	if not array.is_empty():
			var types = PandoraPropertyType.get_all_types()
			var values_type : PandoraPropertyType
			for i in range(array.size()):
					var value = array[i]
					var value_type = UndefinedType.new()

					for type in types:
							if type.is_valid(value):
								value_type = type
								value = type.write_value(value)

					var value_dict = {
							"type": values_type.get_type_name()
							"value": value
					}

					dict[str(i)] = value_dict

@bitbrain bitbrain closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array properties do not properly deserialize all types
3 participants