-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
Re-enable ThemeDb
+ impl GodotConvert
for *mut c_void
#461
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-461 |
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! Two minor comments, could you amend your previous commit with them?
godot-codegen/src/util.rs
Outdated
if cfg!(before_api = "4.2") && class.name == "ThemeDB" { | ||
// registered in C++ register_scene_singletons(), after MODULE_INITIALIZATION_LEVEL_EDITOR happens. | ||
// fixed since 4.2 (https://github.com/godotengine/godot/pull/81305) | ||
ClassCodegenLevel::Lazy | ||
} else if class.name.ends_with("Server") { |
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 wonder if this is still necessary now...
- before 4.2,
ThemeDB
class is absent -> so the API level shouldn't be queried - since 4.2,
ThemeDB
hasScene
level
Could you try to entirely remove this branch, as well as the ClassCodegenLevel::Lazy
variant?
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're right, I was able to remove it without a problem 👍
@@ -240,3 +240,21 @@ impl FromGodot for *const std::ffi::c_void { | |||
Some(via as Self) | |||
} | |||
} | |||
|
|||
// mut void* is used by ScriptExtension::instance_create() |
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.
In C, it's just void*
, not mut void*
.
You could move this up to line 226, before // Other impls...
, then it should be clearer.
Also please end with a full stop 🙂
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.
fixed!
ThemeDb
+ impl GodotConvert
for *mut c_void
c5d5350
to
4650b82
Compare
Thank you! 🙂 |
ThemeDB has been fixed (Fix ThemeDB initialization in tests godotengine/godot#81305) and is instantiated before Editor level, so we can now reenable it
*mut c_void is needed for the
instance_create
signature of Godot'sScriptExtension
class, so I added the impls for it