-
-
Notifications
You must be signed in to change notification settings - Fork 259
Update runtime_version to use non-deprecated get_godot_version pointer.
#1394
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
Conversation
1525d6a to
cb91099
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1394 |
Bromeon
left a comment
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 a lot! 👍
I could leave it as good first issue, oh well 😬.
We can't always leave things open in the hope that someone will pick them up one day. Many such issues are open for more than a year.
godot-ffi/src/interface_init.rs
Outdated
| #[cfg(before_api = "4.5")] | ||
| type GDExtensionInterfaceGetGodotVersion = sys::GDExtensionInterfaceGetGodotVersion; | ||
| #[cfg(since_api = "4.5")] | ||
| type GDExtensionInterfaceGetGodotVersion = sys::GDExtensionInterfaceGetGodotVersion2; |
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.
Please don't prefix anything we define with GDExtension.
If we do this in other places, we should fix it.
godot-ffi/src/interface_init.rs
Outdated
| #[cfg(before_api = "4.5")] | ||
| static GET_GODOT_VERSION: &[u8] = b"get_godot_version\0"; | ||
| #[cfg(since_api = "4.5")] | ||
| static GET_GODOT_VERSION: &[u8] = b"get_godot_version2\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.
Should be const and not static. See also next comment with grouping 🙂
godot-ffi/src/lib.rs
Outdated
| #[cfg(before_api = "4.5")] | ||
| pub type GodotVersion = GDExtensionGodotVersion; | ||
| #[cfg(since_api = "4.5")] | ||
| pub type GodotVersion = 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.
Would be nice to define this in the other file, next to the function pointer type, and re-export here.
If we keep all 3 symbols (func ptr, func name, version struct) together, it might also be worth an immediately re-exported module:
#[cfg(before_api = "4.5")]
mod version_symbols {
pub ...
}
#[cfg(since_api = "4.5")]
mod version_symbols {
pub ...
}
use version_symbols::*;
godot-ffi/src/lib.rs
Outdated
| pub type GodotVersion = GDExtensionGodotVersion; | ||
| pub type InterfaceGetGodotVersion = GDExtensionInterfaceGetGodotVersion; |
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.
Sorry, forgot to add this before:
I was thinking to add a high-level GodotVersion at some point, which could be used by GdextBuild or similar. In anticipation of that, would it make sense to include "Sys" here? Also in the function pointer, then we can drop the "Interface":
| pub type GodotVersion = GDExtensionGodotVersion; | |
| pub type InterfaceGetGodotVersion = GDExtensionInterfaceGetGodotVersion; | |
| pub type GodotSysVersion = super::GDExtensionGodotVersion; | |
| pub type GetGodotSysVersion = super::GDExtensionInterfaceGetGodotVersion; |
(Not sure if use super::* is still needed)
cb91099 to
f9dad7d
Compare
2747c76 to
55548db
Compare
Bromeon
left a comment
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!
get_godot_versionhas been deprecated in favor ofget_godot_version2: https://github.com/godotengine/godot/blob/master/core/extension/gdextension_interface.h#L854.I could leave it as good first issue, oh well 😬.