pandora icon indicating copy to clipboard operation
pandora copied to clipboard

Array properties do not properly deserialize all types

Open nkrisc opened this issue 2 years ago • 2 comments

Godot version: 4.1.3

Describe the bug Array properties that have their type set to property types that use custom parsing logic do not properly deserialize their values when an entity has overrides for that property.

To Reproduce Steps to reproduce the behavior:

  1. Create a category with an Array property
  2. Change Array property type setting to "color"
  3. Create an entity in that category
  4. Set default color values for the Array property on the entity
  5. Try to retrieve the property values for that entity

PandoraEntity.get_array will return an array of strings (the serialized color values), and not instances of Color. This is true for Resource, Reference, and Vector type arrays.

Here is a unit test that demonstrates the issue:

func test_array_property_custom_parsers() -> void:
	var array_type = load("res://addons/pandora/model/types/array.gd")
	var category = Pandora.create_category("Test Category")
	var property = Pandora.create_property(category, "property", "array")
	property.set_setting_override(array_type.SETTING_ARRAY_TYPE, "color")
	var entity = Pandora.create_entity("entity", category)
	var entity_property = entity.get_entity_property("property")
	entity_property.set_default_value([Color.WHITE])
	entity.load_data(entity.save_data())
	assert_that(entity.get_array("property")[0]).is_equal(Color.WHITE)
	assert_that(typeof(entity.get_array("property")[0])).is_not_equal(TYPE_STRING)

Result:

Expecting:
 'Color(1, 1, 1, 1)'
 but was
 '(1, 1, 1, 1)ffffffff' 

 Expecting:
 '4'
 not equal to
 '4'

EDIT: Added assertion for type of array value

The test fails, and I don't think I messed it up (though certainly possible as I had to really dive in to figure out how to craft it). I also confirmed while troubleshooting the initial issue that it is in fact returning the String "(1, 1, 1, 1)ffffffff".

I looked into it a bit to find the source of the issue, and to the best of my knowledge it occurs because PandoraEntity._load_overrides does not pass the property settings dict to type.parse_value() (here) thus the Array property implementation of PandoraPropertyType.parse_value (here) defaults to an empty dict for the settings and the custom parsing logic is skipped.

Expected behavior Getting the value of an array property from an entity should have values of the correct type per the property settings.

Additionally, I believe that in the array property the actual implementation of parse_value for each type should be used directly, because as it stands now the deserialization logic is duplicated in the array property, and in each respective class that uses logic.

I imagine something like:

func parse_value(variant:Variant, settings:Dictionary = {}) -> Variant:
	if variant is Dictionary:
		var array = []
		var dict = variant as Dictionary
		for i in range(dict.size()):
			var value = dict[str(i)]
			if not settings.is_empty():
				value = PandoraPropertyType.lookup(settings[SETTING_ARRAY_TYPE]).parse_value(value)
			array.append(value)
		return array
	return variant

Other types like float or int are unaffected because they are properly handled by the JSON deserialization before this step.

nkrisc avatar Dec 06 '23 01:12 nkrisc

Feel free to raise a PR for this.

bitbrain avatar Dec 06 '23 08:12 bitbrain

I still haven’t quite grasped the entire workflow this is a part of. I was having trouble getting the property settings at the right moment.

I will do so though if I get something working.

nkrisc avatar Dec 06 '23 13:12 nkrisc