-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add editor plugins, and registration of classes at the proper initialization level #393
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-393 |
dc6abca
to
7c59a28
Compare
7c59a28
to
c60afa6
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.
Quite a few people have requested this, thanks a lot for coming up with a PR! 😊
Instead of a runtime check, could we create a compile error (aka bail
) if the user specifies #[class(editor_plugin)]
while under Godot < 4.1?
// Only on Android. | ||
| "JavaClassWrapper" | ||
| "JNISingleton" | ||
| "JavaClass" | ||
// Only on WASM. | ||
| "JavaScriptBridge" | ||
| "JavaScriptObject" |
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 nice, how did you find those classes? It looks like their presence didn't trigger an error (even now with full-ci checking everything)?
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 made a script to write a file that made a class inheriting from every single class in the gdextension api. some classes crashed on startup because i assume they're not meant to be extended (like AudioServer
), but these threw an error because they werent available.
impl #class_name { | ||
#[doc(hidden)] | ||
const __INHERITS_EDITOR_PLUGIN: () = <#class_name as ::godot::obj::Inherits<::godot::engine::EditorPlugin>>::INHERITS; | ||
} |
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 another place, we have used a slightly different idiom to ensure a trait bound at compile time (although currently commented-out):
fn __static_type_check() {
enforce_sync::<InstanceStorage<T>>();
}
fn enforce_sync<T: Sync>() {}
Do you see any advantages of one vs. the other idiom?
Maybe it makes sense to do generalize this to something like static_assert_bounds!(Type : Bounds);
or so? Although a macro might just put more load on the codegen 🤔
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 don't really think there's a big difference either way. Though this one is more discoverable at least. I couldn't quite find a place this had been done before. I can change to doing it that way.
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, just so we use same technique to achieve the same thing 🙂 we can of course discuss/change that at any time, if we find one approach works better.
Sure, i can also make the |
ee4b8eb
to
5bd20d3
Compare
One minor thing + conflict, also this is still open:
Would be nice to protect against non-functional attribute keys directly in the proc-macro API. After that, should be ready 🙂 |
if let Some(editor_plugin) = parser.handle_alone_ident("editor_plugin")? { | ||
if cfg!(before_api = "4.1") { | ||
return bail!( | ||
editor_plugin, | ||
"Editor plugins are not supported in Godot 4.0" | ||
); | ||
} | ||
|
||
is_editor_plugin = true; | ||
} |
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.
@Bromeon i did make it bail
if you try to use it before godot 4.1
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, right, I was confused by this code:
#[cfg(before_api = "4.1")]
assert!(!info.is_editor_plugin);
But that's then only to double-check that. Makes sense.
fd3ccd6
to
06ba63f
Compare
Require that editor plugins inherit from `EditorPlugin` Make classes get registered at the base class's init level Unexclude `ProjectSettings`, since it isn't experimental
06ba63f
to
e05dc40
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 a lot! Don't hesitate to split unrelated changes into separate commits, but that's definitely not a blocker now 😊
Editor Plugin
Adds
editor_plugin
as another attribute for classes. See here for usage.The project will fail to compile if you try to make something an editor plugin that doesn't inherit from
EditorPlugin
. This is done by adding a doc-hidden constant toInherits
and trying to access it.Attempting to register an editor plugin in Godot 4.0 will print an error, and not add the editor plugin. Since this feature was added in 4.1.
Class registration
This is needed for editor plugin registration to work, as editor plugins must be registered at the
Editor
level.GodotClass
now has anINIT_LEVEL
constant which states the level at which a class is registered, and thus which level its subclasses must also be registered.If an engine class has an unknown init level, the library panics. Which should be fine since it means something has gone very wrong. However user-classes just do an
godot_error
and fail to register, since that might be user-error and making godot crash for that would probably be more frustrating than anything else.Other smaller things
I found some more classes that only exist for WASM/Android so i added them to be excluded, you can't inherit from them anyway.
Unexcluded
ProjectSettings
, it seems to have been excluded by mistake. I also added a test just to ensure that it actually does work.I didn't find a good way of testing editor plugins. They require the editor to be running, and we dont have any way to test stuff that only runs in the editor afaik.