godot icon indicating copy to clipboard operation
godot copied to clipboard

Implement a struct-like type that can be exposed to scripting.

Open nlupugla opened this issue 2 years ago • 17 comments
trafficstars

I am in the process of implementing the proposal https://github.com/godotengine/godot-proposals/issues/7329 of @reduz on adding structs to GDScript. This work is still very much in it's early stages and many features still have yet to be implemented. Nevertheless, I'm making the draft public now so I can get continual feedback on this highly requested feature.

Edit 2023-Oct-18 22:50: Added list of structable methods

I've gone through Godot's API and made a list of about 130 methods that might benefit from structification: https://gist.github.com/nlupugla/01d59094d5fa31230e15bd991b63d33f.

  • [x] Add struct attributes to the Array class.
  • [x] Create Struct<T> class as a child of Array.
  • [x] Create STRUCT_LAYOUT and related macros to facilitate creating Struct<T> specializations from actual C++ structs.
  • [x] Proof of concept: create a struct that is equivalent to the PropertyInfo used in Object::get_property_list.
  • [x] Implement type validation.
  • [x] Find a way to return a typed array of structs (Godot does not yet support TypedArray<TypedArray<T>>).
  • [x] Register structs with ClassDB.
  • [x] Proof of concept: expose a struct to GDScript.
  • [ ] Comprehensive unit tests.
  • [ ] Performance profiling.
  • [ ] Performance optimizations.
  • [ ] Probably a million other things I'm forgetting.
  • [x] Implement struct syntax in GDScript.
  • [x] Implement static analysis for structs in GDScript.
  • [ ] Figure out how structs will interoperate with C# and GDExtension.
  • [ ] User testing.

nlupugla avatar Sep 23 '23 16:09 nlupugla

Edited my OP to link to a list of potentially structable methods in Godot's API: https://gist.github.com/nlupugla/01d59094d5fa31230e15bd991b63d33f.

nlupugla avatar Oct 09 '23 02:10 nlupugla

As requested on RocketChat, I skimmed through the code related to compat methods for GDExtension, and left a few comments. I haven't had a chance to really dig into this PR, so it's possible my comments are missing some important context.

The CI does a great job of pointing out GDExtension compat issues, so I've started a CI run and we can see if it shows any issues.

Thanks dsnopek, really appreciate it!

So I'm getting from you that only the exposed methods need to go in the .inc file and the rest can live in the .h or .cpp file, is that right? And it sounds like the approach is that DISABLE_DEPRECATED should basically just change the external API, so internal things like the dictionary conversion methods that I "deprecated" should actually stick around so I don't have to go through and update the entire codebase (in places like script_language_extension as you mentioned) to use the new struct format instead.

nlupugla avatar Oct 10 '23 16:10 nlupugla

Yep, that sounds about right!

dsnopek avatar Oct 10 '23 18:10 dsnopek

Great, that makes sense!

I made the suggested changes, but I'm still failing some of the GDScript unit tests. I'm not entirely sure why, but at least one test is failing because the test is calling get_property_list and expecting an array of dictionaries instead of an array of structs. Obviously, I could change the tests, but I think if I do the compatibility right, I shouldn't have to do that, right? Is there a way for me to tell the tests to use the compatibility methods?

nlupugla avatar Oct 10 '23 18:10 nlupugla

Obviously, I could change the tests, but I think if I do the compatibility right, I shouldn't have to do that, right? Is there a way for me to tell the tests to use the compatibility methods?

The compatibility methods are only for GDExtension. For GDScript stuff you'll have to handle compatibility in a different way.

dsnopek avatar Oct 10 '23 18:10 dsnopek

Ah, interesting! Well, I guess that's an argument for moving GDScript to being a GDExtension :)

nlupugla avatar Oct 10 '23 19:10 nlupugla

~Something has broken as there's enums that's been shifted, something in the default generation code must have broken, see CodeEdit~

My bad missed that was a direct change, should be added at the end probably though to keep compatibility

AThousandShips avatar Feb 07 '24 15:02 AThousandShips

I'd strongly recommend rebasing this and fixing the format so we can get the CI to show some details

AThousandShips avatar Feb 07 '24 15:02 AThousandShips

My bad missed that was a direct change, should be added at the end probably though to keep compatibility

Are you saying I should put the enum at the end like this?

	enum CodeCompletionKind {
		KIND_CLASS,
		KIND_STRUCT, // should be moved from here
		KIND_FUNCTION,
		KIND_SIGNAL,
		KIND_VARIABLE,
		KIND_MEMBER,
		KIND_ENUM,
		KIND_CONSTANT,
		KIND_NODE_PATH,
		KIND_FILE_PATH,
		KIND_PLAIN_TEXT, // to just after here
	};

nlupugla avatar Feb 07 '24 16:02 nlupugla

Yes, possibly, otherwise it can cause some problems potentially, unsure exactly but it breaks compatibility to renumber enums

AThousandShips avatar Feb 07 '24 16:02 AThousandShips

Hi Folks! I was applying some fixes last week to get this branch to pass the CI and, as you can see, I'm now passing most of the tests and am now dealing with some more obscure errors to me that have less obvious solutions. If anyone more knowledgeable on some of these error messages can help but me on the right track, it would be much appreciated!

  1. Linux / Editor w/ Mono

Am I perhaps doing something wrong with the compatibility methods for core_bind?

Log

Godot.SourceGenerators.Internal -> /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/Godot.SourceGenerators.Internal/bin/Debug/netstandard2.0/Godot.SourceGenerators.Internal.dll /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDB.cs(348,48): error CS0111: Type 'ClassDB' already defines a member called 'ClassGetSignal' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDB.cs(359,73): error CS0111: Type 'ClassDB' already defines a member called 'ClassGetSignalList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDB.cs(370,73): error CS0111: Type 'ClassDB' already defines a member called 'ClassGetPropertyList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDB.cs(382,73): error CS0111: Type 'ClassDB' already defines a member called 'ClassGetMethodList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDBInstance.cs(356,41): error CS0111: Type 'ClassDBInstance' already defines a member called 'ClassGetSignal' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDBInstance.cs(367,66): error CS0111: Type 'ClassDBInstance' already defines a member called 'ClassGetSignalList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDBInstance.cs(378,66): error CS0111: Type 'ClassDBInstance' already defines a member called 'ClassGetPropertyList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/ClassDBInstance.cs(390,66): error CS0111: Type 'ClassDBInstance' already defines a member called 'ClassGetMethodList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GodotObject.cs(1056,66): error CS0111: Type 'GodotObject' already defines a member called 'GetPropertyList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GodotObject.cs(1074,66): error CS0111: Type 'GodotObject' already defines a member called 'GetMethodList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GodotObject.cs(1086,66): error CS0111: Type 'GodotObject' already defines a member called 'GetSignalList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GodotObject.cs(1100,66): error CS0111: Type 'GodotObject' already defines a member called 'GetSignalConnectionList' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GodotObject.cs(1114,66): error CS0111: Type 'GodotObject' already defines a member called 'GetIncomingConnections' with the same parameter types [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GltfAccessor.cs(287,18): warning CS0108: 'GltfAccessor.GetType()' hides inherited member 'object.GetType()'. Use the new keyword if hiding was intended. [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] /home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/GltfAccessor.cs(578,43): warning CS0108: 'GltfAccessor.MethodName.GetType' hides inherited member 'object.GetType()'. Use the new keyword if hiding was intended. [/home/runner/work/godot/godot/modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj] Error: Process completed with exit code 1.

  1. Linux / Editor with doubles and GCC sanitizers

Is this perhaps triggering because I memnewed a Node in my tests that I didn't free afterwards? https://github.com/godotengine/godot/pull/82198/files#diff-c8898cf9c08702e43ce9ad5f080e9b47ad7e5ad77617c7aa5afd5bba5b7174adR187

Log

Direct leak of 1072 byte(s) in 1 object(s) allocated from: #0 0x7ff3bebcf808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144 #1 0x55a795cb6441 in Memory::alloc_static(unsigned long, bool) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x52558441) #2 0x55a795cb6352 in operator new(unsigned long, char const*) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x52558352) #3 0x55a779dc3134 (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x36665134) #4 0x55a77b31b435 in doctest::Context::run() (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x37bbd435) #5 0x55a77a8f67e5 in test_main(int, char**) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x371987e5) #6 0x55a779652487 in Main::test_entrypoint(int, char**, bool&) (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x35ef4487) #7 0x55a7792c0841 in main (/home/runner/work/godot/godot/bin/godot.linuxbsd.editor.dev.double.x86_64.san+0x35b62841) #8 0x7ff3bde03082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)

SUMMARY: AddressSanitizer: 4288 byte(s) leaked in 4 allocation(s). Error: Process completed with exit code 1.

  1. Linux / Editor with clang sanitizers

Same as above?

  1. Linux / Minimal template

Maybe related to a mistake in my compatibility methods as well?

Log

/usr/bin/ld: core/libcore.linuxbsd.template_release.x86_64.a(register_core_types.linuxbsd.template_release.x86_64.o): in function register_core_singletons()': register_core_types.cpp:(.text+0x2668): undefined reference to core_bind::special::ClassDB::_bind_compatibility_methods()' /usr/bin/ld: register_core_types.cpp:(.text+0x266f): undefined reference to core_bind::special::ClassDB::_bind_compatibility_methods()' /usr/bin/ld: core/libcore.linuxbsd.template_release.x86_64.a(register_core_types.linuxbsd.template_release.x86_64.o): in function core_bind::special::ClassDB::_initialize_classv()': register_core_types.cpp:(.text._ZN9core_bind7special7ClassDB18_initialize_classvEv[_ZN9core_bind7special7ClassDB18_initialize_classvEv]+0x119): undefined reference to core_bind::special::ClassDB::_bind_compatibility_methods()' /usr/bin/ld: register_core_types.cpp:(.text._ZN9core_bind7special7ClassDB18_initialize_classvEv[_ZN9core_bind7special7ClassDB18_initialize_classvEv]+0x120): undefined reference to core_bind::special::ClassDB::_bind_compatibility_methods()' scons: building terminated because of errors. collect2: error: ld returned 1 exit status scons: *** [bin/godot.linuxbsd.template_release.x86_64] Error 1 [Time elapsed: 00:12:53.456] Error: Process completed with exit code 2.

nlupugla avatar Apr 15 '24 14:04 nlupugla

Hey @nlupugla , I pulled down your branch to take a look at it and (correct me if I'm horribly wrong) it seems like the Mono build is failing because the generated C# code found in ClassDB.cs (modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects) did not contain the new methods you added called class_get_signal_as_struct, class_get_signal_list_as_structs, etc. In the Linux CI it seems to duplicate existing methods from some reason (?) or have some other issue, for me on Windows it just doesn't generate them at all. To take a look at why that is, I tried to look into the generated code and I got intellisense working for C# through C# Dev Kit on VSC to track down some stuff; here is an example of an existing function:

public static Godot.Collections.Dictionary ClassGetSignal(StringName @class, StringName signal)
{
    return NativeCalls.godot_icall_2_260(MethodBind8, GodotObject.GetPtr(Singleton), (godot_string_name)(@class?.NativeValue ?? default), (godot_string_name)(signal?.NativeValue ?? default));
}

And I noticed the return type, tracked it down, and found that Collections are manually defined in GodotSharp/Core as a .cs file. Again I may be wrong so I apologize, but it's possible that it can't generate because there is no type Struct<MethodInfo> (used by class_get_signal_as_struct) to use. So one thing to try if you haven't already is to create the Struct in C# within the Godot.Collections namespace, track down if that needs to be added anywhere within the generator, and you can test whether it works by running Godot with --generate-mono-glue in a shell (the recommended process noted in the docs for compiling Godot with C#) and then see if the new functions are added to ClassDB. Hope that helped in at least narrowing the Mono CI issue!

Btw I'm very interested in seeing this merged because it would allow me to implement https://github.com/godotengine/godot-proposals/issues/438 very easily, which would be a huge win for C# users like me :) Would love to collaborate with you on this if you'd like, I'm pretty familiar with C# if you need help implementing the Struct type there!

RobProductions avatar Sep 08 '24 00:09 RobProductions

Hi @RobProductions, thanks for your interest! I would love some help on the C# side. I don't use C# myself so I was pretty well at a loss for how to fix these issues on my own :)

I'll try and rebase this branch today. Then, feel free to tinker around with it and let me know if you manage to come up with a fix.

Feel free to reach out on the Godot contributors chat too: https://chat.godotengine.org/channel/QTu9nG79BRxe8YyqD

nlupugla avatar Sep 08 '24 14:09 nlupugla

Thanks to some help from RobProductions, I finally got this branch to pass the CI. The main thing I needed to do was comment out all the places that expose Structs to the public API. Having Structs show up in the API was confusing the C# and GDExtension bindings generators, which as of yet do not understand Structs. I also split off all the doc gen code I added related to Structs into a separate PR: https://github.com/godotengine/godot/pull/97297.

I'd like to get some input from maintainers on how to organize the integration of Structs. As I see it, there are a few important milestones to hit, some or all of which might belong as separate commits in this PR or a different PR entirely.

  1. Add Structs to core. This should not change anything for the end user, it just lays groundwork. Accomplished in the current PR.
  2. Add Structs to GDScript. This allows users to create Structs in GDScript, but not export them to the editor. https://github.com/godotengine/godot/pull/90356
  3. Add Structs to editor. This allows users to export GDScript Structs to the editor. https://github.com/godotengine/godot/commit/f36d5a17be11f92740410ee10862bc14bfbd8351#diff-17781aa265945f5f8988f4d66150a04b73fcdd99f5600b24409d731d24e6fbf1R780
  4. Add doc gen for Structs. This enables Godot to automatically generate documentation for user and native Structs. https://github.com/godotengine/godot/pull/97297
  5. Add Structs to API. This would add Structs to the Godot API, which would enable Structs as properties, inputs, and returns. The current PR has some commented out code which starts this, but the correct way to integrate Structs into the API must consider compatibility with C# and GDExtension.

If the maintainers would prefer item 1. as a standalone PR, then this PR is ready for review.

nlupugla avatar Sep 21 '24 21:09 nlupugla

Amazing work!! I am not sure of the right place for this but I have a few questions about the implementation. For all of the methods that return a Dictionary and will be updated to return a struct, what will happen with that? Imo its worth breaking compatibility for such a great change for usability, but I'm not sure how the community would feel of such a breaking change. Maybe this was all figured out already and I didn't see it, I'm just curious.

FireCatMagic avatar Sep 23 '24 17:09 FireCatMagic

There are no concrete plans as of yet regarding replacing Dictionary with Structs in the API. As you say, this would be a massive compatibility break, so it's a delicate matter.

nlupugla avatar Sep 23 '24 21:09 nlupugla

I would say for replacing dictionaries it should be in a separate pr for stuff like raycast( which based on past conversation couldnkt scale with the modernized api of the engine), this way it leaves contributors time to review and check for regressions.

Saul2022 avatar Sep 28 '24 16:09 Saul2022

Is the plan for Structs in C# and GDExtension to be exposed as direct C#/ C++ structs? Even if the internals of how this will be implemented aren't know?

FireCatMagic avatar Oct 08 '24 06:10 FireCatMagic

Is the plan for Structs in C# and GDExtension to be exposed as direct C#/ C++ structs? Even if the internals of how this will be implemented aren't know?

This is a question that needs more discussion. My understanding of GDExtension (though I'd welcome someone more knowledgeable like @dsnopek to correct me if I'm wrong) is that native structs (e.g. PropertyInfo) are already exposed "directly" as C++ structs. I'm not so sure about C#, but I know it is in the process of being moved to something more like GDExtension.

nlupugla avatar Oct 08 '24 13:10 nlupugla

Is the plan for Structs in C# and GDExtension to be exposed as direct C#/ C++ structs? Even if the internals of how this will be implemented aren't know?

Ideally, the SourceGenerator should generate godot structs bindings for [Export]ted structs or records, but I don't know the implementation details.

Delsin-Yu avatar Oct 08 '24 14:10 Delsin-Yu

Is the plan for Structs in C# and GDExtension to be exposed as direct C#/ C++ structs?

The implementation of "structs" here is sufficiently different from C structs, that I'm not totally sure it makes sense to expose them to GDExtension as C structs. Mainly, it's the copying to/from C structs that we'd have to do that gives me pause - it may be more efficient to deal with them like Arrays.

My understanding of GDExtension (though I'd welcome someone more knowledgeable like @dsnopek to correct me if I'm wrong) is that native structs (e.g. PropertyInfo) are already exposed "directly" as C++ structs.

Yes, PropertyInfo has a C struct in the GDExtension interface, and we already have a wrapper in godot-cpp that allows it to work just like PropertyInfo in Godot itself.

And for other things where efficiency is important, there already is a system for Godot to expose C structs directly to GDExtension - it's used for the AudioFrame struct, for example. The main thing we don't have currently is a way for a GDExtension to efficiently expose a C struct to Godot or other GDExtensions - I don't know how many folks really need that, though?

dsnopek avatar Oct 08 '24 15:10 dsnopek

Is the plan for Structs in C# and GDExtension to be exposed as direct C#/ C++ structs? Even if the internals of how this will be implemented aren't know?

Ideally, the SourceGenerator should generate godot structs bindings for [Export]ted structs or records, but I don't know the implementation details.

I may be getting a bit confused from the terms being used interchangeably but from what I understand (in the C# perspective only) there are the new "Godot Structs" from this PR and native C# structs. So there are multiple processes for how each are handled and perhaps in the future the two could be merged but it's bit more difficult to go that route. Assuming they're distinct:

  1. The Godot Structs will be referenced in return types for API calls and can be exposed to C# via a "Struct" class. I started the first steps of this work in this commit though I didn't get far enough to connect it to the return types. Also the API is now no longer being modified by this PR so I'm waiting for us to reach that point before looking into it further. I can also shrink down the commit so that the Struct class reuses some functionality from Array but that's research I'll do in the future.
  2. The Godot Structs can be [Export]ed so that they display in the Inspector. I believe @ajreckof has started work on this here!
  3. The C# structs that you use within the engine can be distinct from Godot Structs hence they remain unchanged. However a neat feature I've been thinking about is that if you [Export] a C# struct it could internally convert it to a Godot Struct which could then be shown via point 2. and that would close https://github.com/godotengine/godot-proposals/issues/438 . Though people have suggested other ways of doing this that wouldn't rely on the Godot Struct, however I'm still unclear what those ways are and if they're more viable than this.

Again, this could change if someone is able to merge the two together so that the API returns generated C# structs but based on other Godot collections this would not be the norm (i.e. Godot Array and Godot Dictionary). So assuming we leave them distinct we just need point 1. to work for C# to be able to interact with the new stuff that the API PR will add :) Point 2 will be a necessary extension and point 3. is kind of a separate issue that just pairs nicely with this. Correct me if I got any of that wrong lol

RobProductions avatar Oct 08 '24 15:10 RobProductions

Usability-wise, I don't see any meaningful reason for publicizing the GodotStruct class, as it's just an array with each element's type and name annotated. In GodotSharp and godot-dotnet, the user should never directly deal with GodotStruct. Like what we marshal packed arrays into C# arrays, we have a godot_struct interop type similar to godot_array that only the source-generated code touches and populates.

Delsin-Yu avatar Oct 08 '24 15:10 Delsin-Yu

Like what we marshal packed arrays into C# arrays, we have a godot_struct interop type similar to godot_array that only the source-generated code touches and populates.

I agree that would be better for the end user, but how do you implement something like that for something as generic as a Godot Struct? I was assuming the implementation would be most similar to the Godot Array since as you said it's just an array with each element's type specified. With Packed Arrays I can kind of see how one is constructed in the marshalling code but what happens when you need to define types for fields of the struct, is that even possible without generating each API struct uniquely? Apologies for my lack of knowledge in this area

RobProductions avatar Oct 08 '24 16:10 RobProductions

You do have a point. I will look into it and try to create a demo this week. Still, I think it is possible to make the source generator emit the correct Godot struct info based on the implementation in this PR.

Because the GDScript also needs to address this issue somehow, I don't see a major blocker between the two languages in this context.

Delsin-Yu avatar Oct 08 '24 17:10 Delsin-Yu

@RobProductions My demo is based on your implementation except the Struct<T> or Struct class.

Let's start with an example user struct that has all of its member Variant Compatible:

public struct UserInfo
{
    public int UserId { get; set; }
    public string UserName { get; set; }
    public string UserPassword { get; set; }
    public int[] UserToken { get; set; }
    public double[] UserRecord { get; set; }
}

And a Node script that consumes it:

public partial class UserNode : Node
{
    [Export] private UserInfo _userInfo;

    public void PrintUserInfo(UserInfo userInfo)
    {
        GD.Print(userInfo.UserId);
    }
}

The ideal source generator output would be the following:

[NewFile] StructInterop.generated.cs

public static class StructInterop
{
    public static Variant CreateFromUserInfo(UserInfo value)
    {
        NativeFuncs.godotsharp_struct_new(out godot_struct godotStruct);
        NativeFuncs.godotsharp_struct_resize(ref godotStruct, 5);
        NativeFuncs.godotsharp_struct_add(ref godotStruct, VariantUtils.CreateFrom<int>(value.UserId));
        NativeFuncs.godotsharp_struct_add(ref godotStruct, VariantUtils.CreateFrom<string>(value.UserName));
        NativeFuncs.godotsharp_struct_add(ref godotStruct, VariantUtils.CreateFrom<string>(value.UserPassword));
        NativeFuncs.godotsharp_struct_add(ref godotStruct, VariantUtils.CreateFrom<int[]>(value.UserToken));
        NativeFuncs.godotsharp_struct_add(ref godotStruct, VariantUtils.CreateFrom<double[]>(value.UserRecord));
        NativeFuncs.godotsharp_variant_new_struct(out godot_variant ret, godotStruct);
        return ret;
    }

    public static UserInfo ConvertToUserInfo(Variant variant)
    {
        godot_struct godotStruct = NativeFuncs.godotsharp_variant_as_struct(variant);
        UserInfo value = new UserInfo();
        value.UserId = VariantUtils.ConvertTo<int>(godotStruct.Elements[0]);
        value.UserName = VariantUtils.ConvertTo<string>(godotStruct.Elements[1]);
        value.UserPassword = VariantUtils.ConvertTo<string>(godotStruct.Elements[2]);
        value.UserToken = VariantUtils.ConvertTo<int[]>(godotStruct.Elements[3]);
        value.UserRecord = VariantUtils.ConvertTo<double[]>(godotStruct.Elements[4]);
        return value;
    }
}

UserNode_ScriptSerialization.generated.cs

partial class UserNode
{
    protected override void SaveGodotObjectData(GodotSerializationInfo info)
    {
        base.SaveGodotObjectData(info);
        info.AddProperty(PropertyName.@_userInfo, StructInterop.CreateFromUserInfo(this.@_userInfo));
    }

    protected override void RestoreGodotObjectData(GodotSerializationInfo info)
    {
        base.RestoreGodotObjectData(info);
        if (info.TryGetProperty(PropertyName.@_userInfo, out var _value__userInfo))
            this.@_userInfo = StructInterop.ConvertToUserInfo(_value__userInfo);
    }
}

UserNode_ScriptMethods.generated.cs

partial class UserNode
{
    public new class PropertyName : Node.PropertyName
    {
        public new static readonly StringName @_userInfo = "_userInfo";
    }

    protected override bool SetGodotClassPropertyValue(in godot_string_name name, in godot_variant value)
    {
        if (name == PropertyName.@_userInfo) {
            this.@_userInfo = StructInterop.ConvertToUserInfo(value);
            return true;
        }
        return base.SetGodotClassPropertyValue(name, value);
    }

    protected override bool GetGodotClassPropertyValue(in godot_string_name name, out godot_variant value)
    {
        if (name == PropertyName.@_userInfo) {
            value = StructInterop.CreateFromUserInfo(this.@_userInfo);
            return true;
        }
        return base.GetGodotClassPropertyValue(name, out value);
    }

    internal new static List<PropertyInfo> GetGodotPropertyList()
    {
        var properties = new List<PropertyInfo>();

        // TODO: Use StructInfo Here
        properties.Add(
            new(
                type: Variant.Type.Array, 
                name: PropertyName.@_userInfo, 
                hint: PropertyHint.None, 
                hintString: "UserInfo", 
                usage: PropertyUsageFlags.Default | PropertyUsageFlags.ScriptVariable, 
                exported: true
            )
        );
        return properties;
    }
}

UserNode_ScriptMethods.generated.cs

partial class UserNode
{
    public new class MethodName : Node.MethodName
    {
        public new static readonly StringName @PrintUserInfo = "PrintUserInfo";
    }

    internal new static List<MethodInfo> GetGodotMethodList()
    {
        var methods = new List<MethodInfo>(1);
        methods.Add(
            new(
                name: MethodName.@PrintUserInfo,
                returnVal: new(
                    type: Variant.Type.Nil, 
                    name: "", 
                    hint: PropertyHint.None, 
                    hintString: "", 
                    usage: PropertyUsageFlags.Default,
                    exported: false
                ),
                flags: MethodFlags.Normal,
                arguments: new() {
                    new(
                        // TODO: Use StructInfo Here
                        type: Variant.Type.Array, 
                        name: "userInfo", 
                        hint: PropertyHint.None, 
                        hintString: "", 
                        usage: PropertyUsageFlags.Default,
                        exported: false
                    ),	
                },
                defaultArguments: null
            )
        );

        return methods;
    }
   
    protected override bool InvokeGodotClassMethod(in godot_string_name method, NativeVariantPtrArgs args, out godot_variant ret)
    {
        if (method == MethodName.@PrintUserInfo && args.Count == 1) {
            @PrintUserInfo(StructInterop.ConvertToUserInfo(args[0]));
            ret = default;
            return true;
        }
        return base.InvokeGodotClassMethod(method, args, out ret);
    }
  
    protected override bool HasGodotClassMethod(in godot_string_name method)
    {
        if (method == MethodName.@PrintUserInfo) {
           return true;
        }
        return base.HasGodotClassMethod(method);
    }
}

Delsin-Yu avatar Oct 09 '24 11:10 Delsin-Yu

So if they were exposed as native structs to the respective language when possible, I'm assuming it would be like, for example in C++ and C#

  • Calling get on something that returns a struct would result in the Array-based variant struct
  • Calling an exposed internal method directly (hypothetically with the dictionary replaced with a struct), ex PhysicsDirectSpaceState3D::intersect_ray, would return a language struct, for example the C# language struct or C structs, which both individually have a statically fixed size and wouldn't have any extra bloat of the dynamic allocations of the Array class
  • These native language structs would somehow be able to be implicitly exchanged with the Array based variant structs. (maybe they could all extend from an abstract struct class? not sure)
  • Avoiding the C++ struct -> dynamic Array struct -> back to C++ struct in GDextension / C# possibly and just converting a C++ struct to a language struct, only using the Array struct when used in GDScript?

I'm not sure of the feasibility of all of this, I'm just very curious in this discussion

FireCatMagic avatar Oct 09 '24 14:10 FireCatMagic

The struct internal is just array-based, but it can be mapped to a concrete struct type in the corresponding language (C# or GDExtensionCPP). I don't see a solid reason to use the godot-structs as arrays, even if it's technically possible. If we are passing these structs by references, a special wrapper type can be used to manage the reference behavior.

Delsin-Yu avatar Oct 09 '24 16:10 Delsin-Yu

@Delsin-Yu Thanks! I think I understand and your example sounds like a good way of doing it, I guess I'm a bit caught up in how the generator can create the StructInterop file but that's definitely a future concern. I worry about bloat for all the unique structs the interop file has to define across the API and user code but I guess that's not too important. And also I'm a bit confused on how the GDExtension implementation for C# will accomplish the same thing, will it also create the interop files? If so, will it require reflection, since I thought I read somewhere that they're trying avoid that? I've only done research into GodotSharp so that stuff is a bit beyond me lol

Sorry for clogging up the thread as these are concepts for later PRs but we should definitely look into that method since it'll be much cleaner for users :)

RobProductions avatar Oct 09 '24 16:10 RobProductions

@RobProductions It's my pleasue to be able to help!

I guess I'm a bit caught up in how the generator can create the StructInterop file but that's definitely a future concern.

I don't think it's that much of a blocker for Generator to create such bindings on the fly, as a side of the Source Generator is just a very advanced version of Reflections, and we can just enumerate all the public properties in the generator and dump all the properties into a string builder.

And also I'm a bit confused on how the GDExtension implementation for C# will accomplish the same thing, will it also create the interop files?

I apologize for my lack of knowledge about project godot-dotnet; Raul mentioned that the project should support languages without source generation capabilities, so asking him may be a good option. However, I do remember seeing C# console application based code generators in the Repo.

Delsin-Yu avatar Oct 09 '24 16:10 Delsin-Yu