godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.96k stars 21.08k forks source link

Godot integers are sliced to 32-bit integers when marshalled to Mono/C# #39609

Closed MichaelBelousov closed 4 years ago

MichaelBelousov commented 4 years ago

Godot version:

3.2 (hash: be5a47e75d), somewhat specifically a custom editor build with godot steam 3.2 which has no modifications except for the addition of the godot steam module to the trunk. It is a public github repository and I can link to it/include it if anyone wants to investigate.

Issue description:

When marshaling a godot int variant to the mono runtime/C#, a boxed 32-bit int is returned, slicing integers that only fit in a 64-bit integer, which godot supports. I am trying to use the GodotSteam engine module with C#, but SteamIds are 64-bit integers, and they are sliced when results are returned to C#. The engine code at: modules/mono/mono_gd/gd_mono_marshal.cpp:557 (at line 557) marshals ints to a boxed 32-bit integer in the mono runtime. I have made some experimental changes sending to 64-bit. I wouldn't mind making a PR, but certainly pending discussion since I'm new to the godot engine, and this sounds like it could be a breaking change if all C# projects now need to use the long type in C# to refer to godot ints.

case Variant::INT: {
  int32_t val = p_var->operator signed int();
  return BOX_INT32(val);}

Steps to reproduce:

  1. Create any Godot project
  2. add a C# script
  3. access any godot function that returns an int (in my case it was GodotSteam's LobbyId)

Minimal reproduction project: I can upload an example if necessary, but GodotSteam is rather intense of a minimal viable reproduction, and admittedly I'm having csproj issues building on my windows machine with a freshly installed mono-capable release version of 3.2, But here is my untested MVR. mvr-godot-slices-mono-ints.zip

Calinou commented 4 years ago

cc @neikeq

neikeq commented 4 years ago

The lines you mentions should indeed be changed to return a long:

https://github.com/godotengine/godot/blob/02f7908d460bea7d580fe7526949be7f294e6783/modules/mono/mono_gd/gd_mono_marshal.cpp#L628

However, that is for assigning Variant::INT to something that expects object (anything). In your example project that's not what's happening. Your method receives an int instead of a long so it's correct to slice it. I think your example project doesn't reflect correctly the actual issue you're experiencing with GodotSteam.

MichaelBelousov commented 4 years ago

I think my problem is then in iterating an array (Godot.Collections.Array) of SteamIds returned from godotsteam. I tried to use array.OfType() (which obviously doesn't work). But why slice to a 32-bit integer when in a dynamically typed context?

neikeq commented 4 years ago

Yes, in a dynamically typed context it should be fixed to return long instead.

MichaelBelousov commented 4 years ago

I created a very small starting pull request #39629, please let me know if/how you want me to proceed, I drafted it for now.

akien-mga commented 4 years ago

Fixed by #39629.

redthing1 commented 2 years ago

Pretty sure this is still broken:

In Godot 3.4.4

C# in debugger: image

image

output: image

GDScript: image

redthing1 commented 2 years ago

AH yes. https://github.com/godotengine/godot/blob/3.4/modules/mono/mono_gd/gd_mono_marshal.cpp#L576-L579