-
-
Notifications
You must be signed in to change notification settings - Fork 20.9k
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
base: master
Are you sure you want to change the base?
Conversation
core/variant/array.cpp
Outdated
/// structs | ||
|
||
uint32_t struct_size = 0; | ||
const StructMember *struct_members = nullptr; |
There was a problem hiding this comment.
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.
core/variant/struct.h
Outdated
#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, ...) \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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])
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
core/variant/reduz_struct_proposal.h
Outdated
// Added struct stuff: | ||
uint32_t struct_size = 0; | ||
StringName * struct_member_names = nullptr; | ||
bool struct_array = false; |
There was a problem hiding this comment.
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()
?
core/variant/reduz_struct_proposal.h
Outdated
} | ||
|
||
_FORCE_INLINE_ bool validate_member(uint32_t p_index,const Variant& p_value) { | ||
// needs to check with ContainerValidate, return true is valid |
There was a problem hiding this comment.
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.
core/variant/reduz_struct_proposal.h
Outdated
|
||
|
||
template <class T> | ||
class Struct : public Array { |
There was a problem hiding this comment.
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.
core/variant/reduz_struct_proposal.h
Outdated
|
||
// 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. |
There was a problem hiding this comment.
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.
core/variant/reduz_struct_proposal.h
Outdated
#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];\ | ||
}\ | ||
}; |
There was a problem hiding this comment.
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
core/object/object.cpp
Outdated
@@ -64,7 +64,7 @@ struct _ObjectDebugLock { | |||
|
|||
#endif | |||
|
|||
STRUCT_LAYOUT(PropertyInfoLayout, "PropertyInfo", 5, | |||
STRUCT_LAYOUT(PropertyInfoLayout, "PropertyInfo", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
core/variant/struct.h
Outdated
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()); |
There was a problem hiding this comment.
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
There was a problem hiding this 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
Edited my OP to link to a list of potentially structable methods in Godot's API: https://gist.github.com/nlupugla/01d59094d5fa31230e15bd991b63d33f. |
There was a problem hiding this 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.
core/object/object.compat.inc
Outdated
#include "core/object/class_db.h" | ||
#include "core/variant/typed_array.h" | ||
|
||
PropertyInfo::operator Dictionary() const { |
There was a problem hiding this comment.
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?
core/object/object.compat.inc
Outdated
return mi; | ||
} | ||
|
||
void Object::_add_user_signal_compat_99999(const String &p_name, const Array &p_args) { |
There was a problem hiding this comment.
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
core/object/object.h
Outdated
operator Dictionary() const; | ||
|
||
static PropertyInfo from_dict(const Dictionary &p_dict); |
There was a problem hiding this comment.
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.
core/object/object.h
Outdated
@@ -204,7 +264,9 @@ struct PropertyInfo { | |||
} | |||
}; | |||
|
|||
#ifndef DISABLE_DEPRECATED | |||
TypedArray<Dictionary> convert_property_list(const List<PropertyInfo> *p_list); |
There was a problem hiding this comment.
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.
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 |
Yep, that sounds about right! |
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 |
The compatibility methods are only for GDExtension. For GDScript stuff you'll have to handle compatibility in a different way. |
Ah, interesting! Well, I guess that's an argument for moving GDScript to being a GDExtension :) |
core/core_bind.h
Outdated
#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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
core/object/object.compat.inc
Outdated
ClassDB::bind_compatibility_method(D_METHOD("get_incoming_connections"), &Object::_get_incoming_connections_compat_82198); | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif | |
#endif | |
core/core_bind.compat.inc
Outdated
} | ||
} | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif | |
#endif | |
core/object/object.h
Outdated
#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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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 |
core/object/object.h
Outdated
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
core/object/object.h
Outdated
#ifndef DISABLE_DEPRECATED | ||
TypedArray<Dictionary> _get_property_list_bind_compat_82198() const; | ||
TypedArray<Dictionary> _get_method_list_bind_compat_82198() const; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifndef DISABLE_DEPRECATED | |
TypedArray<Dictionary> _get_property_list_bind_compat_82198() const; | |
TypedArray<Dictionary> _get_method_list_bind_compat_82198() const; | |
#endif |
My bad missed that was a direct change, should be added at the end probably though to keep compatibility |
I'd strongly recommend rebasing this and fixing the format so we can get the CI to show some details |
8707fa5
to
154d057
Compare
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!
Am I perhaps doing something wrong with the compatibility methods for core_bind? Log
Is this perhaps triggering because I Log
Same as above?
Maybe related to a mistake in my compatibility methods as well? Log
|
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 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 Btw I'm very interested in seeing this merged because it would allow me to implement godotengine/godot-proposals#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! |
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 |
f24ca52
to
5e6a92b
Compare
5b2c7e6
to
7e9d0b6
Compare
e3e2397
to
defe5d5
Compare
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.
If the maintainers would prefer item 1. as a standalone PR, then this PR is ready for review. |
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 |
There are no concrete plans as of yet regarding replacing |
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. |
f7f01b1
to
1076403
Compare
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.
Array
class.Struct<T>
class as a child ofArray
.STRUCT_LAYOUT
and related macros to facilitate creatingStruct<T>
specializations from actual C++ structs.PropertyInfo
used inObject::get_property_list
.TypedArray<TypedArray<T>>
).ClassDB
.