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

Array properties do not properly deserialize all types #159

Open
nkrisc opened this issue Dec 6, 2023 · 2 comments
Open

Array properties do not properly deserialize all types #159

nkrisc opened this issue Dec 6, 2023 · 2 comments
Labels
🐛 bug Something isn't working
Milestone

Comments

@nkrisc
Copy link
Contributor

nkrisc commented Dec 6, 2023

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 nkrisc added the 🐛 bug Something isn't working label Dec 6, 2023
@bitbrain
Copy link
Owner

bitbrain commented Dec 6, 2023

Feel free to raise a PR for this.

@nkrisc
Copy link
Contributor Author

nkrisc commented Dec 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants