-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
GDExtension: Add status
to get_godot_version
#103199
base: master
Are you sure you want to change the base?
Conversation
ec4bb90
to
71b599d
Compare
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!
This looks good, but we also need to remove the registration of the old get_godot_version
(without the 2
) if Godot is compiled with DISABLED_DEPRECATED
. This shouldn't affect the gdextension_interface.h
, only whether or not gdextension_get_godot_version()
is implemented and if REGISTER_INTERFACE_FUNC(get_godot_version)
is called.
What about other version fields, such as Lines 126 to 148 in f931a65
|
typedef struct { | ||
uint32_t major; | ||
uint32_t minor; | ||
uint32_t patch; | ||
const char *status; | ||
const char *string; | ||
} GDExtensionGodotVersion2; |
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.
Should we use the opportunity to document each field with a short example value, so it's clear how they look?
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.
Sounds good, I copied the examples from version.h
71b599d
to
8429df8
Compare
To be honest, I forgot |
Ah, yeah, we should probably include all the same information from
FYI, having |
…ersion` Added in a new `get_godot_version2` function with a new `GDExtensionGodotVersion2` to avoid breaking compatibility.
8429df8
to
05ffa21
Compare
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! Looks good to me :-)
I find myself needing the
status
for the .NET upgrade tool to determine the version of the packages to use in the upgraded project.