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

nkrisc · 2023-12-13T22:21:05Z

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:

If the supplied settings have a type value, that type will be used to parse the values.
If no settings are supplied, the type value present in the stored data will be used, unless it is UndefinedType
If the stored type is Undefined and no settings are supplied, the values will be passed alone as-is
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

closes Array properties do not properly deserialize all types #159

bitbrain · 2023-12-14T08:57:21Z

addons/pandora/model/type.gd

@@ -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"):


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.

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.

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

value type is explicitly set

bitbrain · 2023-12-14T17:45:08Z

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?

bitbrain · 2023-12-14T17:47:02Z

test/model/property_test.gd

@@ -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:


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 · 2024-01-13T13:42:54Z

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:

I believe the current state of the PR addresses the root issue, however
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.
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.
Need to finish adding additional tests, particularly for point 3 if I can get that working.

bitbrain · 2024-02-23T19:14:29Z

@nkrisc did you manage to progress on this?

imothee · 2024-03-04T22:44:04Z

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

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

c893926

bitbrain reviewed Dec 14, 2023

View reviewed changes

Removed unnecessary if statement and ensured array

173c589

value type is explicitly set

bitbrain reviewed Dec 14, 2023

View reviewed changes

Merge branch 'godot-4.x' into fix-array-parsing

4c268a5

imothee mentioned this pull request Mar 5, 2024

Seralization and deserialized of typed properties in array #175

Merged

Merge branch 'godot-4.x' into fix-array-parsing

d9daa6d

bitbrain closed this Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nkrisc commented Dec 13, 2023 •

edited by bitbrain

Loading

bitbrain Dec 14, 2023

nkrisc Dec 14, 2023

imothee Mar 4, 2024

bitbrain commented Dec 14, 2023

bitbrain Dec 14, 2023

nkrisc commented Jan 13, 2024

bitbrain commented Feb 23, 2024

imothee commented Mar 4, 2024 •

edited

Loading

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

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

Conversation

nkrisc commented Dec 13, 2023 • edited by bitbrain Loading

Description

Addressed issues

bitbrain Dec 14, 2023

Choose a reason for hiding this comment

nkrisc Dec 14, 2023

Choose a reason for hiding this comment

imothee Mar 4, 2024

Choose a reason for hiding this comment

bitbrain commented Dec 14, 2023

bitbrain Dec 14, 2023

Choose a reason for hiding this comment

nkrisc commented Jan 13, 2024

bitbrain commented Feb 23, 2024

imothee commented Mar 4, 2024 • edited Loading

nkrisc commented Dec 13, 2023 •

edited by bitbrain

Loading

imothee commented Mar 4, 2024 •

edited

Loading