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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nlupugla
Copy link
Contributor

@nlupugla nlupugla commented Sep 23, 2023

I am in the process of implementing the proposal godotengine/godot-proposals#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.

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

core/object/object.cpp Outdated Show resolved Hide resolved
core/variant/array.cpp Outdated Show resolved Hide resolved
core/variant/struct.h Outdated Show resolved Hide resolved
/// structs

uint32_t struct_size = 0;
const StructMember *struct_members = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

I know you are trying to simplify the code, but this should work like the proposal for performance reasons. With separate arrays, this means less cache lines loaded for the lookup, hence faster performance.

#define STRUCT_MEMBER(m_name, m_type) StructMember(SNAME(m_name), m_type)
#define STRUCT_CLASS_MEMBER(m_name, m_class) StructMember(SNAME(m_name), Variant::OBJECT, m_class)
// TODO: is there a way to define this so that the member count doesn't have to be passed?
#define STRUCT_LAYOUT(m_name, m_member_count, ...) \
Copy link
Member

@reduz reduz Sep 24, 2023

Choose a reason for hiding this comment

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

I think passing member count is unnecesary, you can add some macro magic to guess the number of `VA_ARGS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out the proper way to guess the number of args. One method I saw worked using sizeof, but I don't think that will work here because sizeof StructMember won't be known yet? Another method involved explicitly enumerating all possible number of arguments up to some finite number (like 16, or 39, or whatever you have the patience to write). That method seemed pretty clunky to me, and would also limit the number of members a struct can have. That said, it would be great if the user didn't have to pass in the length somehow.

Copy link

@adrian17 adrian17 Sep 24, 2023

Choose a reason for hiding this comment

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

You could check Boost.preprocessor for inspiration, there the macro would probably look more like (note lack of delimiting semicolons)

STRUCT_LAYOUT(PropertyInfoLayout,
		("name", Variant::STRING)
		("type", Variant::INT)
		("hint", Variant::INT)
		("hint_string", Variant::STRING)
		("class_name", Variant::STRING_NAME)
);

At which point BOOST_PP_SEQ_SIZE would give you the size. (and it's more generic in general, in case you ever needed a macro iterate over the member list more than once, for example to generate get/set_hint_string etc instead of having C++ call get/set_named). I've seen a macro that did a very similar struct declaration exactly this way (not in public code though).
Or instead of getting size via a macro, you could put the field types into a tuple type and get tuple size at compile time via templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip! We won't to avoid unnecessary packages and libraries though, so tuple is off limits. A quick search on how BOOST_PP_SEQ_SIZE works suggests it falls in to the camp of explicitly enumerating sequence lengths up to some finite number. Maybe that's the way to go in the end, but it feels a little odd to restrict the number of members in a struct to a smallish number (like 64 or 256).

Copy link

@adrian17 adrian17 Sep 24, 2023

Choose a reason for hiding this comment

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

We won't to avoid unnecessary packages and libraries though, so tuple is off limits.

Sure, but even without an std::tuple, it still can be done about the same, with the sizeof... operator. So as long as you can get the macro to generate SomeTemplate<Variant::STRING, Variant::INT, Variant::INT, etc>, that should be enough. (...though at that point there are probably even simpler ways)

Copy link
Contributor

Choose a reason for hiding this comment

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

I apologise if I'm missing something, but if you moved up the static const StructMember members[member_count] = line, couldn't you get the member count with the classic sizeof(members)/sizeof(members[0])?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uh, if you changed it from static const StructMember members[member_count] = to static const StructMember members[] =, that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I thought that wouldn't work because StructMember isn't a primitive type (like int or something). I can try it out though, it would certainly be much simpler :)

Copy link
Contributor

@hikari-no-yume hikari-no-yume Sep 25, 2023

Choose a reason for hiding this comment

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

Oh sorry, when I try it I get an error about “in-class initialization of static data member 'const StructMember PropertyInfoLayout::members []' of incomplete type” (https://godbolt.org/z/vG8csqqjd). I'm not sure why C++ cares about that. It does work however if it is made a static variable (outside the class) rather than a static property. To namespace it you could use token concatenation like: const StructMember m_name##_members[] = { (https://godbolt.org/z/9P5aGGcfs) but I appreciate that might not be to everyone's taste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, your idea seems to be working for me :)

// Added struct stuff:
uint32_t struct_size = 0;
StringName * struct_member_names = nullptr;
bool struct_array = false;
Copy link
Contributor Author

@nlupugla nlupugla Sep 25, 2023

Choose a reason for hiding this comment

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

Isn't this redundant with the function is_struct_array()?

}

_FORCE_INLINE_ bool validate_member(uint32_t p_index,const Variant& p_value) {
// needs to check with ContainerValidate, return true is valid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented some methods in ContainerValidate, but I'm still not entirely sure what to put here.



template <class T>
class Struct : public Array {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Struct inheriting from Array is kind of awkward. There are many Array methods that need to be manually disabled for Struct (such as push_back) and several Array methods that just don't make sense for Struct (like map). I understand the desire to not make another Variant::Type, as this would have implications for the whole codebase. I wonder whether it would make sense for Struct to not inherit from Array, but still call itself a Variant::Type::ARRAY as far as the rest of the codebase is concerned.


// The idea here is that if GDScript code is typed, it should be able to access everything without any kind of validation or even copies. I will add this in the GDScript optimization proposal I have soon (pointer addressing mode).

// That said, I think we should consider changing ArrayPrivate::Array from Vector to LocalVector, this should enormously improve performance when accessing untyped (And eventually typed) arrays in GDScript. Arrays are shared, so there is not much of a need to use Vector<> here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have wondered about that too, though I feel it is beyond the scope of this PR.

Comment on lines 193 to 203
#define STRUCT_LAYOUT(m_class,m_name,...) \
struct m_name { \
_FORCE_INLINE_ static StringName get_class() { return SNAME(#m_class)); }
_FORCE_INLINE_ static StringName get_name() { return SNAME(#m_name)); }
static constexpr uint32_t member_count = GET_ARGUMENT_COUNT;\
_FORCE_INLINE_ static const StructMember& get_member(uint32_t p_index) {\
CRASH_BAD_INDEX(p_index,member_count)\
static StructMember members[member_count]={ __VA_ARGS__ };\
return members[p_index];\
}\
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have an alternative proposal that I think will be cleaner and have several advantages. I've written it up here: https://gist.github.com/nlupugla/f78a947f0f2d409a7ab7819d5d379a28

@@ -64,7 +64,7 @@ struct _ObjectDebugLock {

#endif

STRUCT_LAYOUT(PropertyInfoLayout, "PropertyInfo", 5,
STRUCT_LAYOUT(PropertyInfoLayout, "PropertyInfo",
Copy link
Member

Choose a reason for hiding this comment

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

This should go in the .h I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting it in the .h makes things messy because then object.h has to include struct.h (where STRUCT_LAYOUT is defined) and now most of the engine suddenly depends on struct.h. Keeping this part in object.cpp keeps struct.h much more isolated.

Copy link
Contributor Author

@nlupugla nlupugla Sep 27, 2023

Choose a reason for hiding this comment

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

Hmm. Well I just now realized that putting the definition of Struct<PropertyInfoLayout> in object.cpp means that anyone that wants to use Struct<PropertyInfoLayout> has to include object.cpp, which is not good. Maybe the STRUCT_LAYOUT macro should go somewhere more central than struct.h? What would make sense? type_defs.h? type_info.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no, I think the solution is to just put it in object.h or make another filed in the variant folder called struct_layout.h. I was getting issues with circular dependencies if I tried to put the definition in typed_defs.h or type_info.h.

static const Variant::Type VARIANT_TYPE = Variant::ARRAY;
static const GodotTypeInfo::Metadata METADATA = GodotTypeInfo::METADATA_NONE;
_FORCE_INLINE_ static PropertyInfo get_class_info() {
return PropertyInfo(Variant::ARRAY, String(), PROPERTY_HINT_ARRAY_TYPE, T::get_class_static());
Copy link
Member

Choose a reason for hiding this comment

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

Here you will probably need to do it like this:

PROPERTY_HINT_STRUCT_TYPE, String(T::get_class())+"."+T::get_name());

PROPERTY_HINT_STRUCT_TYPE will need to be added in object.h

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

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

Looking good, left a comment

@nlupugla
Copy link
Contributor Author

nlupugla commented Oct 9, 2023

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

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

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.

#include "core/object/class_db.h"
#include "core/variant/typed_array.h"

PropertyInfo::operator Dictionary() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few functions in here that aren't compat methods, including this one. These should probably just go in the normal object.cpp file?

return mi;
}

void Object::_add_user_signal_compat_99999(const String &p_name, const Array &p_args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The 99999 should be relplaced with 82198 - the number of this PR

Comment on lines 221 to 223
operator Dictionary() const;

static PropertyInfo from_dict(const Dictionary &p_dict);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure these can be removed when deprecated stuff is disabled? These are used in script_language_extension.h in methods that are definitely not deprecated. But I haven't looked through the whole PR to see if you've worked around that somewhere.

@@ -204,7 +264,9 @@ struct PropertyInfo {
}
};

#ifndef DISABLE_DEPRECATED
TypedArray<Dictionary> convert_property_list(const List<PropertyInfo> *p_list);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here: this is used in rendering_server.cpp in a method that isn't deprecated.

@nlupugla
Copy link
Contributor Author

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.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 10, 2023

Yep, that sounds about right!

@nlupugla
Copy link
Contributor Author

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?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 10, 2023

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.

@nlupugla
Copy link
Contributor Author

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

core/object/class_db.h Outdated Show resolved Hide resolved
core/core_bind.h Outdated
Comment on lines 462 to 504
#ifndef DISABLE_DEPRECATED
Dictionary class_get_signal_compat_82198(StringName p_class, StringName p_signal) const;
TypedArray<Dictionary> class_get_signal_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> class_get_property_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> class_get_method_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#ifndef DISABLE_DEPRECATED
Dictionary class_get_signal_compat_82198(StringName p_class, StringName p_signal) const;
TypedArray<Dictionary> class_get_signal_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> class_get_property_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> class_get_method_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
#endif

core/core_bind.h Outdated
@@ -423,6 +423,7 @@ class ClassDB : public Object {

protected:
static void _bind_methods();
static void _bind_compatibility_methods();
Copy link
Member

@AThousandShips AThousandShips Oct 11, 2023

Choose a reason for hiding this comment

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

Suggested change
static void _bind_compatibility_methods();
#ifndef DISABLE_DEPRECATED
Dictionary _class_get_signal_compat_82198(StringName p_class, StringName p_signal) const;
TypedArray<Dictionary> _class_get_signal_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> _class_get_property_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
TypedArray<Dictionary> _class_get_method_list_compat_82198(StringName p_class, bool p_no_inheritance = false) const;
#endif
static void _bind_compatibility_methods();

These should be kept together

ClassDB::bind_compatibility_method(D_METHOD("get_incoming_connections"), &Object::_get_incoming_connections_compat_82198);
}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif

}
}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif

core/object/object.cpp Outdated Show resolved Hide resolved
core/object/object.cpp Outdated Show resolved Hide resolved
Comment on lines 716 to 425
#ifndef DISABLE_DEPRECATED
void _add_user_signal_compat_82198(const String &p_name, const Array &p_args = Array());
TypedArray<Dictionary> _get_signal_list_compat_82198() const;
TypedArray<Dictionary> _get_signal_connection_list_compat_82198(const StringName &p_signal) const;
TypedArray<Dictionary> _get_incoming_connections_compat_82198() const;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#ifndef DISABLE_DEPRECATED
void _add_user_signal_compat_82198(const String &p_name, const Array &p_args = Array());
TypedArray<Dictionary> _get_signal_list_compat_82198() const;
TypedArray<Dictionary> _get_signal_connection_list_compat_82198(const StringName &p_signal) const;
TypedArray<Dictionary> _get_incoming_connections_compat_82198() const;
#endif

@@ -688,7 +774,8 @@ class Object {
virtual void _notificationv(int p_notification, bool p_reversed) {}

static void _bind_methods();
static void _bind_compatibility_methods() {}
static void _bind_compatibility_methods();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static void _bind_compatibility_methods();
#ifndef DISABLE_DEPRECATED
void _add_user_signal_compat_82198(const String &p_name, const Array &p_args = Array());
TypedArray<Dictionary> _get_signal_list_compat_82198() const;
TypedArray<Dictionary> _get_signal_connection_list_compat_82198(const StringName &p_signal) const;
TypedArray<Dictionary> _get_incoming_connections_compat_82198() const;
TypedArray<Dictionary> _get_property_list_bind_compat_82198() const;
TypedArray<Dictionary> _get_method_list_bind_compat_82198() const;
#endif
static void _bind_compatibility_methods();

Comment on lines 831 to 540
#ifndef DISABLE_DEPRECATED
TypedArray<Dictionary> _get_property_list_bind_compat_82198() const;
TypedArray<Dictionary> _get_method_list_bind_compat_82198() const;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#ifndef DISABLE_DEPRECATED
TypedArray<Dictionary> _get_property_list_bind_compat_82198() const;
TypedArray<Dictionary> _get_method_list_bind_compat_82198() const;
#endif

@AThousandShips
Copy link
Member

AThousandShips commented Feb 7, 2024

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
Copy link
Member

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

@nlupugla nlupugla force-pushed the struct branch 2 times, most recently from 8707fa5 to 154d057 Compare February 7, 2024 16:51
@nlupugla
Copy link
Contributor Author

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: #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. Add Structs to GDscript #90356
  3. Add Structs to editor. This allows users to export GDScript Structs to the editor. f36d5a1#diff-17781aa265945f5f8988f4d66150a04b73fcdd99f5600b24409d731d24e6fbf1R780
  4. Add doc gen for Structs. This enables Godot to automatically generate documentation for user and native Structs. Add doc gen for structs. #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.

@FireCatMagic
Copy link

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.

@nlupugla
Copy link
Contributor Author

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.

@Saul2022
Copy link

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.

@nlupugla nlupugla force-pushed the struct branch 3 times, most recently from f7f01b1 to 1076403 Compare October 1, 2024 00:37
@nlupugla nlupugla marked this pull request as ready for review October 1, 2024 01:41
@nlupugla nlupugla requested review from a team as code owners October 1, 2024 01:41
@FireCatMagic
Copy link

FireCatMagic commented Oct 8, 2024

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?

@nlupugla
Copy link
Contributor Author

nlupugla commented Oct 8, 2024

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.

@Delsin-Yu
Copy link
Contributor

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.

@dsnopek
Copy link
Contributor

dsnopek commented Oct 8, 2024

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?

@RobProductions
Copy link
Contributor

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 Implement exporting struct variables in C# godot-proposals#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

@Delsin-Yu
Copy link
Contributor

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.

@RobProductions
Copy link
Contributor

RobProductions commented Oct 8, 2024

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

@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Oct 8, 2024

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
Copy link
Contributor

Delsin-Yu commented Oct 9, 2024

@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);
    }
}

@FireCatMagic
Copy link

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

@Delsin-Yu
Copy link
Contributor

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.

@RobProductions
Copy link
Contributor

@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 :)

@Delsin-Yu
Copy link
Contributor

@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
Copy link
Contributor

Made a source generator that does what I said, the project won't compile , but hopefully provides useful information.

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.

10 participants