-
-
Notifications
You must be signed in to change notification settings - Fork 260
Allow to register user defined engine singletons. #1399
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
base: master
Are you sure you want to change the base?
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1399 |
| T: UserSingleton, | ||
| { | ||
| fn singleton() -> Gd<T> { | ||
| unsafe { |
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 is missing a SAFETY 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.
Very silly overlook on my part – I added properly panicking version for safeguards_strict level and wrote proper SAFETY remark for disengaged ones.
eafbcd6 to
20baefb
Compare
|
Note – our test Backtrace |
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!
Out of curiosity, what does a separate UserSingleton marker trait buy us, that implementing Singleton for each user class individually does not?
| pub trait UserSingleton: | ||
| GodotClass + Bounds<Declarer = bounds::DeclUser, Memory = bounds::MemManual> | ||
| { | ||
| } |
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.
Why limit to MemManual?
If #522 is the reason, should we try to fix that at some point? 🙂
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, honestly I haven't been checking this case – RefCounted Engine Singletons just didn't make sense to me 🤔. Engine Singleton should be valid as long as library is loaded (as opposed to RefCounted which are valid as long as something keeps reference to them – and in this case engine should always keep a reference to it) and must be pruned in-between hot reloads.
I can look into supporting this case as well.
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 Object still be automatically-managed (library destroys it on unload)? If yes, that would be OK to start with, and we may indeed not need to support RefCounted.
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 Object still be automatically-managed (library destroys it on unload)
yep, that's implemented
godot-core/src/registry/class.rs
Outdated
| // Will imminently add given class to the editor in Godot before 4.4.1. | ||
| // It is expected and beneficial behaviour while we load library for the first time | ||
| // but (for now) might lead to some issues during hot reload. | ||
| // but might lead to some issues during hot reload. |
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.
What do you mean with 4.4.1 -- what happens after that version?
Further, I know the comment about hot-reload issues was there before, but do you think we can concretize it?
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.
4.4.1 has cherry-picked fix which postpones adding EditorPlugins: godotengine/godot#109310 (linked in the issue).
As for concretization – I think I can distill it to one comment over let mut editor_plugins: Vec<ClassId> = Vec::new(); in line 226.
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.
Whenever there's a change in behavior that you deem relevant for our godot-rust logic, please always link the relevant upstream PR/issue from code.
"before 4.4.1" without any context will look like magic in a couple months 🙂
godot-core/src/obj/traits.rs
Outdated
| crate::classes::Engine::singleton() | ||
| .get_singleton(&<T as GodotClass>::class_id().to_string_name()) | ||
| .unwrap_or_else(|| panic!("Singleton {} not found. User singleton must be registered under its class name.", T::class_id())).cast() |
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 make this a bit easier to understand by declaring T::class_id() in a separate variable. Then you can also use inline {class}.
The formatting seems weird, I'd expect cast() to be on another line...
Maybe it's worth a comment why downcasting will not panic in this case.
20baefb to
55a62ff
Compare
Mostly blanket implementation – having it in one concrete place makes it much easier to debug + I didn't want to expose |
godot/src/prelude.rs
Outdated
| pub use crate::obj::NewAlloc as _; | ||
| pub use crate::obj::NewGd as _; | ||
| pub use crate::obj::Singleton as _; // singleton() | ||
| pub use crate::obj::UserSingleton as _; // User singleton. |
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 comments list the methods that the trait inserts into the prelude. Since there are none for UserSingleton, there is no need for a comment.
godot-macros/src/lib.rs
Outdated
| /// let val = MySingleton::singleton().bind().my_field; | ||
| /// ``` | ||
| /// | ||
| /// Engine Singletons always run in the editor (similarly to `#[class(tool)]`) and will never be represented by a placeholder instances. |
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.
Does that also mean they cannot be added/removed during hot reloads? If yes, maybe 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.
will never be represented by a placeholder instances
How is this true? From what I see, singletons are always instantiated like all other Godot classes. So when they are not class(tool), they will be instantiated as a placeholder instance.
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.
How is this true?
It is counterintuitive, but instances created via GDExtension won't be placeholders – in the same fashion if we create some GDExtension class inheriting Node via godot-rust and add it to the tree its lifecycle methods (enter tree/ready) will be fired (i.e. there isn't any singleton/library magic involved).
One can easily check it in practice:
// no `#[class(tool)]`
#[derive(GodotClass)]
#[class(init, singleton, base = Object)]
struct MyEngineSingleton {
}
#[godot_api]
impl MyEngineSingleton {
#[func]
fn test_func2(&self) -> u32 {
44
}
}@tool
extends Node2D
func _ready() -> void:
print(MyEngineSingleton.test_func2())
print(MyEngineSingleton.test_var)prints:
44
59
As for the second case:
#[derive(GodotClass)]
#[class(init, base = Node)]
struct NonToolNode {}
#[godot_api]
impl INode for NonToolNode {
fn ready(&mut self) {
godot_print!("Hello from editor :)");
}
}
#[derive(GodotClass)]
#[class(init, tool, base = Node)]
struct MyClass {
base: Base<Node>,
}
#[godot_api]
impl INode for MyClass {
fn ready(&mut self) {
let mut non_tool_node = NonToolNode::new_alloc();
self.base_mut().add_child(&non_tool_node);
non_tool_node.set_owner(self.base().get_owner().as_ref());
}
}
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.
honestly I do wonder if we shouldn't require (or imply) #[class(tool)] to prevent breakage in case if this quirk would be addressed 🤔 Up to you.
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.
Yeah, it looks like all the placeholder logic is handled in ClassDB::_instantiate_internal and since our Gd::new_alloc skips the ClassDB and directly calls the extension's create_instance function, we will never create placeholder instances...
I think it would be a valid use case to create singletons for runtime classes that are only instantiated in the game and not the editor.
One way to work around this would be to go through ClassDB::instantiate() for singletons. But I think it's still very unexpected that Gd::new_alloc bypasses the runtime class system. Maybe this should be discussed in a separate issue. 🤔
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.
Could you create an issue for this? Maybe describe if it also has an impact outside singletons 🤔
godot-macros/src/lib.rs
Outdated
| /// // Can be accessed as any other engine singleton. | ||
| /// let val = MySingleton::singleton().bind().my_field; |
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.
| /// // Can be accessed as any other engine singleton. | |
| /// let val = MySingleton::singleton().bind().my_field; | |
| /// // Can be accessed like any other engine singleton. | |
| /// let val = MySingleton::singleton().bind().my_field; |
Maybe add "for now limited to the main thread" or so 🤔
| #[derive(GodotClass)] | ||
| #[class(init, singleton, base = Object)] | ||
| struct SomeUserSingleton {} |
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 do wonder -- should we change the default base to Object if singleton is specified?
Is there a good use case to use other base classes like Node?
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 do wonder -- should we change the default base to Object if singleton is specified?
Not really, we don't do so anywhere else (everywhere else no base implies RefCounted) – we can restrict singletons to Objects via UserSingleton trait.
Is there a good use case to use other base classes like Node?
Theoretically there is but already covered by EnginePlugins and autoloads 🤔. In the past I was registering autoloads as engine singletons using enter/exit tree but this workflow is ultra janky and prone to failure.
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.
It does mean there is useless boilerplate base=Object required for every singleton though, even though in this case memory management is abstracted from the user.
Even more so if tool is also required...
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.
After some consideration – I think that #[class(singleton)] implying #[class(singleton, tool, base = Object)] wouldn't be out of the place if properly documented 🤔.
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.
Yes, we should definitely document it 👍 maybe also in the place where base=... which currently says RefCounted is the default.
godot-core/src/obj/traits.rs
Outdated
| let singleton = crate::classes::Engine::singleton() | ||
| .get_singleton(&class.to_string_name()) | ||
| .unwrap_or_else(|| panic!("Singleton {class} not found. User singleton must be registered under its class name.")); | ||
| singleton.try_cast().unwrap_or_else(|obj| panic!("Downcasting of User Engine Singleton {class} failed. Expected: {class}, found: {}", obj.dynamic_class_string())) |
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.
Formatting is not great, is it one of the cases where rustfmt gives up due to panic! being a macro?
Does it get better if you don't use an intermediate variable, but directly chain try_cast() after unwrap_or_else()?
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.
Huh, it is due to #[cfg(safeguards_strict)] {}.
55a62ff to
29b323f
Compare
godot-core/src/obj/traits.rs
Outdated
| fn singleton() -> Gd<T> { | ||
| if !cfg!(safeguards_strict) { | ||
| // Note: with any safeguards enabled `singleton_unchecked` will panic if Singleton can't be retrieved. | ||
|
|
||
| // SAFETY: The caller must ensure that `class_name` corresponds to the actual class name of type `T`. | ||
| // This is always true for proc-macro derived user singletons. | ||
| unsafe { | ||
| crate::classes::singleton_unchecked(&<T as GodotClass>::class_id().to_string_name()) | ||
| } | ||
| } else { | ||
| let class = <T as GodotClass>::class_id(); | ||
| crate::classes::Engine::singleton() | ||
| .get_singleton(&class.to_string_name()) | ||
| .unwrap_or_else(|| | ||
| panic!( | ||
| "Singleton {class} not found. User singleton must be registered under its class name." | ||
| )) | ||
| .try_cast() | ||
| .unwrap_or_else(|obj| | ||
| panic!( | ||
| "Downcasting of User Engine Singleton {class} failed. Expected: {class}, found: {}", | ||
| obj.dynamic_class_string() | ||
| ) | ||
| ) | ||
| } | ||
| } |
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.
Formatting is still incorrect (e.g. last panic! not indented). Also, the if !cond { ... } else { ... } is an antipattern, use the positive condition first. In this case it might make more sense to do #[cfg(safeguards_strict)] across the entire singleton function, since the implementations are completely different.
Maybe general question -- what exactly are we protecting against here? The validation failing would be a bug in godot-rust right?
And is there any advantage of Engine::get_singleton() vs. using this function directly, but with Gd::<Object>::from_obj_sys(...).cast::<T>()?
gdext/godot-core/src/classes/class_runtime.rs
Lines 199 to 205 in b2a2a58
| pub(crate) unsafe fn singleton_unchecked<T>(class_name: &StringName) -> Gd<T> | |
| where | |
| T: GodotClass, | |
| { | |
| let object_ptr = unsafe { sys::interface_fn!(global_get_singleton)(class_name.string_sys()) }; | |
| Gd::<T>::from_obj_sys(object_ptr) | |
| } |
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 only difference were more informative panics – I just checked and Engine provide relevant error by itself:
ERROR: Failed to retrieve non-existent singleton 'SomeUserSingleton'.
at: get_singleton_object (core/config/engine.cpp:294)
Therefore I simplified impl to only one case.
Maybe general question -- what exactly are we protecting against here? The validation failing would be a bug in godot-rust right?
Our singletons should always be valid, but it might fail in case of manual registration.
| impl ::godot::obj::GodotClass for #class_name { | ||
| type Base = #base_class; | ||
|
|
||
| #init_level_impl |
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 is not an impl.
Also, it is only relevant for singletons.
| #init_level_impl | |
| #singleton_init_level_const |
|
|
||
| fn make_with_base_impl(base_field: &Option<Field>, class_name: &Ident) -> TokenStream { | ||
| if let Some(Field { name, ty, .. }) = base_field { | ||
| // Apply the span of the field's type so that errors show up on the field's type. |
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.
Did you mean "on the field"?
| // Apply the span of the field's type so that errors show up on the field's type. | |
| // Apply the span of the field's type so that errors show up on the field. |
|
|
||
| /// Generates registration for user singleton and proper INIT_LEVEL declaration. | ||
| /// | ||
| /// Before Godot4.4 built-in Engine singleton – required for registration – wasn't available before `InitLevel::Scene`. |
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.
| /// Before Godot4.4 built-in Engine singleton – required for registration – wasn't available before `InitLevel::Scene`. | |
| /// Before Godot4.4, built-in engine singleton -- required for registration -- wasn't available before `InitLevel::Scene`. |
godot-macros/src/lib.rs
Outdated
| /// ``` | ||
| /// | ||
| /// Engine singletons always run in the editor (similarly to `#[class(tool)]`) and will never be represented by a placeholder instances. | ||
| /// During hot reload user-defined engine singleton will be freed while unloading the library and then freshly instantiated after class registration. |
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.
| /// During hot reload user-defined engine singleton will be freed while unloading the library and then freshly instantiated after class registration. | |
| /// During hot reload, user-defined engine singletons will be freed while unloading the library, and then freshly instantiated after class registration. |
29b323f to
16f292a
Compare
Allow to register user-defined engine singletons via #[class(singleton)]`.
16f292a to
3391ae0
Compare
Allows to register user-defined engine singletons via #[class(singleton)]`.
For now it has same API as engine singletons and follows the same logic –
T::singleton()returns some instance.GDScript will prohibit creating a new instance of the singleton (both in the Godot Editor and on runtime):
One can register their own user singleton without the proc-macro like so:
Creating new instance
initis required to properly register given singleton – mostly for Editor purposes (I decided to not mess with it so far). GDScript won't allow to create new instances of class registered as engine singleton, and in the future it might be good idea to block doing so from godot-rust side as well.Caching
Unimplemented for now. Godot keeps engine singletons on its side in big hashmap; While implementing caching we must make sure it is not redundant 😅.