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

Add missing flag when encode_variant writes math types with doubles #58205

Merged

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Feb 16, 2022

Fixes #57935

The remote inspector is based on encode_variant and decode_variant to send and receive data. decode_variant uses a flag within the type to know if math types in the data are using doubles, but encode_variant is always writing real_t, without specifying the flag. So I made it so the flag is set when the instance of Godot writing the data is compiled with doubles.

cc @aaronfranke

@Zylann Zylann requested a review from a team as a code owner February 16, 2022 20:16
@aaronfranke
Copy link
Member

I don't know if this is correct. The flag should be set when encoding Variant::VECTOR2 with a double-precision build of Godot, but what about some of the other cases? We don't want Variant::VECTOR2I or Variant::COLOR to have this flag set.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Does this work? Untested. Also, PACKED_INT64_ARRAY and PACKED_FLOAT64_ARRAY should be hard-coded to always have this set.

core/io/marshalls.cpp Outdated Show resolved Hide resolved
@Zylann
Copy link
Contributor Author

Zylann commented Feb 16, 2022

We don't want Variant::VECTOR2I or Variant::COLOR to have this flag set.

True, I wasn't actually sure about that. But when I checked, this flag is actually only used on floating-point math types, not on integer ones.
At the very least, maybe I can add the following to make it explicit?

		case Variant::VECTOR2:
		case Variant::VECTOR3:
		case Variant::PACKED_VECTOR2_ARRAY:
		case Variant::PACKED_VECTOR3_ARRAY:
		case Variant::QUATERNION:
		case Variant::PLANE:
		case Variant::TRANSFORM2D:
		case Variant::TRANSFORM3D:
		case Variant::BASIS:
		case Variant::RECT2:
		case Variant::AABB: {

Edit: ok you wrote the same suggestion :D

@Zylann Zylann force-pushed the fix_variant_encode_with_doubles branch from 8273074 to 9d4c70c Compare February 16, 2022 20:36
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I guess the PACKED_*64_ARRAYs don't need it. To me it would still make sense to include it anyway since it 64-bit arrays are indeed encoded as 64-bit, but I guess it doesn't matter.

core/io/marshalls.cpp Outdated Show resolved Hide resolved
@Zylann Zylann force-pushed the fix_variant_encode_with_doubles branch from 9d4c70c to c69d303 Compare February 16, 2022 20:47
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Tested and it works as expected. The code looks good.

@akien-mga akien-mga merged commit 420ad25 into godotengine:master Feb 19, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Remote Inspector doesn't populate certain node types in double precision build
5 participants