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

Ability to convert native engine types to JSON and back. #91503

Closed
wants to merge 3 commits into from

Conversation

reduz
Copy link
Member

@reduz reduz commented May 3, 2024

Closes godotengine/godot-proposals#9510

TODO: Unit tests.

@akien-mga
Copy link
Member

akien-mga commented May 3, 2024

Pushed an update with some style nitpicks, but also flagging some bugs in the implementation based on a first code review. Diff of my update: akien-mga@d06f058

core/io/json.cpp Outdated
Comment on lines 976 to 992
#if 0 // Uncomment after GH-85474 is merged.
case Variant::PACKED_VECTOR4_ARRAY: {
Dictionary d;
PackedVector4Array arr;
Array values;
for (int i = 0; i < arr.size(); i++) {
Vector4 v = arr[i];
values.push_back(v.x);
values.push_back(v.y);
values.push_back(v.z);
values.push_back(v.w);
}
d["values"] = values;
d[GDTYPE] = Variant::get_type_name(p_variant.get_type());
return d;
} break;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Depends on #85474.

Copy link
Member

Choose a reason for hiding this comment

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

Merged, so enabled this code.

core/io/json.cpp Outdated Show resolved Hide resolved
core/io/json.cpp Outdated
Comment on lines 1150 to 1149
} else if (type == Variant::get_type_name(Variant::COLOR)) {
// TODO
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have forgotten Color, I added this TODO.

Copy link
Member

@akien-mga akien-mga May 3, 2024

Choose a reason for hiding this comment

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

Added the missing implementation.

core/io/json.cpp Outdated
Comment on lines 1197 to 1196
} else if (type == Variant::get_type_name(Variant::ARRAY)) {
// TODO
Copy link
Member

Choose a reason for hiding this comment

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

Dictionary was there twice with the same code, seems like you forgot to edit it after copying to handle Array.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed, Array doesn't use GDTYPE so it doesn't need to be handled here (it's handled below).
I added an error check in case this changes, but we could also just remove this branch.

@akien-mga akien-mga force-pushed the json-to-native branch 3 times, most recently from a4b175e to 75171ed Compare May 3, 2024 10:27
@AThousandShips
Copy link
Member

This looks very interesting, I'll test it out and go through the code this weekend hopefully

@AThousandShips AThousandShips self-requested a review May 3, 2024 10:44
@akien-mga
Copy link
Member

akien-mga commented May 3, 2024

Did some testing locally with this project, which could be used as a base for further testing and unit tests:
testjson.zip

Some issues I noticed:

"material": {
	"__gdtype": "Object"
},

but the to_native code will currently just throw an error in this case:

ERROR: Condition "!d.has("type")" is true. Returning: Variant()
   at: to_native (./core/io/json.cpp:1167)

It needs better handling of null Objects as a valid format.

  • Array logic seems to crash, this in my test project causes a segfault:
@export var _array: Array = [42.0, 10, Vector2.AXIS_X
  • If converting an object to JSON and back with objects and scripts allowed, and the script has a class name, converting to native causes an error:
Parser Error: Class "MyNode2D" hides a global script class.
  • I'll admit this is arcane QA, but using PackedByteArray as key in a Dictionary doesn't seem to work:
@export var _packed_byte_array: PackedByteArray = PackedByteArray([10, 20, 30, 40])
@export var _dictionary: Dictionary = {_packed_byte_array: 1.0}

Output:

		"_dictionary": {
			"pairs": [
				{
					"key": {
						"values": [],
						"__gdtype": "PackedByteArray"
					},
					"value": 1
				}
			],
			"__gdtype": "Dictionary"
		},

The values are empty.

@reduz
Copy link
Member Author

reduz commented May 3, 2024

@akien-mga Ah thanks for the feedback and the fixes, I did not test the code much because I wanted to ask for implementation feedback first, then I was going to do the unit testing to ensure everything works properly.

Vector3i v;
v.x = values[0];
v.y = values[1];
v.z = values[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
v.z = values[1];
v.z = values[2];

} break;
case Variant::PACKED_BYTE_ARRAY: {
Dictionary d;
PackedByteArray arr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should arr be assigned to p_variant here? Same for the other packed array types

@AThousandShips
Copy link
Member

Tested and works as expected, at a glance the code looks good too, with some cases pointed out above

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I support this pr enhancement. What is needed to get this mergable for a future release?

Reduz wrote unit tests, but traditionally that has not been a blocker.

Edited:

Did some thinking, if we know the types of the Variant, we can coherce the float into int and int into floats. The minimal epsilon between floats is small until we use the exponential scientific notation in floats to scale.

@AThousandShips
Copy link
Member

I don't necessarily see anything that's needed save for the code errors that need resolving

@fire
Copy link
Member

fire commented Jun 1, 2024

I did some review, the entire Variant section is failing to convert. I'll probably write some unit tests.

@AThousandShips
Copy link
Member

I did some review, the entire Variant section is failing to convert

Probably the code errors reviewed above, try fixing those and see if it works then

@akien-mga
Copy link
Member

Superseded by #92656.

@akien-mga akien-mga closed this Aug 30, 2024
@akien-mga akien-mga removed this from the 4.x milestone Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for all engine types in JSON encoding/decoding
5 participants