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

C#: Populate Type field on godot_variant created in managed #65061

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Aug 29, 2022

This is a fix for #65055.
When created from NativeFuncs.godotsharp_variant_new_copy, variants always had their type property defaulted to Nil. I wondered if it'd be better to populate the type directly in the setters of godot_variant but methods in VariantUtils pass in the type like that.

@raulsntos raulsntos added this to the 4.0 milestone Aug 29, 2022
@raulsntos raulsntos requested a review from a team August 29, 2022 22:45
@markdibarry

This comment was marked as off-topic.

@aaronfranke
Copy link
Member

@markdibarry By design, Variant's only integer type is 64-bit (note: Vector2i/3i/4i use 32-bit ints only).

@markdibarry
Copy link
Contributor

@aaronfranke Ah. That's very weird, but good to know! I've been using object up until the .NET 6 merge, so I'm not as familiar with all of Variant's quirks. I'll keep in mind to avoid int from now on when using Variant and use longs instead. Thanks!

@akien-mga
Copy link
Member

This code looks like it's missing Vector4, Vector4i and Projection, but that could be left for a separate PR (I assume there might be more locations where those new types haven't been added).

@paulloz
Copy link
Member Author

paulloz commented Aug 30, 2022

This code looks like it's missing Vector4, Vector4i and Projection, but that could be left for a separate PR

I can do that in a second PR if need be 😉

Although, looking at it, I think they are handled on the native side? Someone else can probably confirm/infirm.

@neikeq
Copy link
Contributor

neikeq commented Aug 30, 2022

This code looks like it's missing Vector4, Vector4i and Projection, but that could be left for a separate PR

I can do that in a second PR if need be 😉

Although, looking at it, I think they are handled on the native side? Someone else can probably confirm/infirm.

It's handled on the native side, but Vector4 could be moved to the switch for better performance. That being said, it seems Vector4 is not stored properly in godot_variant. I'll fix this on a separate PR.

@neikeq neikeq merged commit 706d988 into godotengine:master Aug 30, 2022
@neikeq
Copy link
Contributor

neikeq commented Aug 30, 2022

I'll keep in mind to avoid int from now on when using Variant and use longs instead. Thanks!

@markdibarry You can use int just fine, but if the Variant was assigned a larger value, it won't warn you when casting it to int (just like it doesn't with (int)long.MaxValue).

@paulloz paulloz deleted the dotnet/fix-godot_variants-with-empty-type-field branch August 30, 2022 16:08
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.

(C# 4.0) Godot.Collections.Dictionary using Variant doesn't work with numbers
6 participants