Open ttencate opened 1 year ago
You can actually just use Engine::singleton().get_singleton(name)
to get a singleton by name
Looking at this a bit more. My suggestion for a "proper" solution like this would be something like:
#[singleton(name="SomeName")]
for GodotClass
, similar to #[class(base=Base)]
auto_register_classes
call Engine::singleton().register_singleton()
on each singletonget_singleton()
call into and properly cast a call to Engine::singleton().get_singleton()
A simpler implementation could also be doable, which is a simple wrapper around Engine::singleton().get_singleton()
.
As a first issue, this is mostly gonna involve copying a lot of pre-existing functionality, as every bit of steps 1-4 is already done for registering the class itself. Concretely:
For 1: parse_class_attr, this is where the #[class(base=Base)]
is parsed, and would likely look very similar for singleton.
For 2: transform, this is where the information about the class is actually added to the plugin system
For 3: auto_register_classes, this is where classes and their info is registered with godot.
For 4: There are many places that this function could be created to be included in the prelude, but im not entirely sure which place is the best one.
There are two problems:
If you want to discuss 2., I would suggest to open a separate issue, as 1. could already be solved today for existing engine classes and is labeled good first issue
.
I would however postpone "user-defined singletons" until after a threading model has been worked out. There is significant design needed if we want to provide an API beyond just "here's your mutex". Interaction with GDScript is also a concern. In other words, this is not on top of my priority list 😉
I'm not entirely sure what about this might require a more worked out threading model. Either the basic impl or the more complicated impl would both just call into Engine::get_singleton
, and using Engine
is thread safe according to the godot docs. And they'd return Gd<T>
objects.
The more complicated version would just allow you to get the singleton by type instead of by name only. I.e
get_singleton::<MySingleton>()
instead of
get_singleton::<MySingleton>("MySingletonName")
If this function is gonna be specific to autoloads, then we shouldn't name it get_singleton
, because Engine::get_singleton
cannot access autoloads, only classes registered with Engine::register_singleton
and so would likely cause confusion about why get_singleton
and Engine::get_singleton
work differently.
So we should maybe split this into several issues because there are several things we'd want:
Engine::register_singleton
and Engine::get_singleton
, preferably with Engine::register_singleton
happening automaticallyTrue, the name "singleton" is overloaded -- thanks for pointing it out. I agree we should separate the "autoload" terminology.
Either the basic impl or the more complicated impl would both just call into
Engine::get_singleton
, and usingEngine
is thread safe according to the godot docs.
I just realized this opens another door of issues... Engine::get_singleton
can be used to easily circumvent thread safety. Sure, the method is thread-safe itself, but that doesn't propagate to the class being returned. So I can set a singleton from one thread and access it from another, without synchronization (just a plain Gd<T>
). This is not limited to get_singleton
but can also apply to GDScript globals or the node tree. But it means that we need to add runtime checks if we don't want to cause race conditions.
It's just one example why this whole topic needs a holistic design, we cannot provide singletons as an isolated feature -- the fact that they're already available through engine APIs at best makes the status quo a bug.
True, since there's a lot of discussion here already we should probably make a new issue for making a convenient way of accessing an autoload.
Also probably just mark either register_singleton
or get_singleton
as unsafe for now (or both?)
It should be noted that the OP message is mixing the two Godot concepts (which Godot mixes itself very often, though).
Engine::singleton().get_singleton(name)
refers to the engine singleton, which is stored as an Object
. Mostly GodotClasses which have purely scripting logic (GDScript / Rust class).
For engine singletons there are many things to consider yet, mainly ownership, thread-safety and register/unregister mechanism.
self.get_node_as::<MySingleton>("/root/GlobalMySingleton")
refers to autoloads, which can be registered either by EditorPlugin
or by Godot Editor. It supports both GDscript classes (I don't think pure Rust class can be registered as autoload, only the GDScript class extending Rust class) and premade scenes (with some logic-containing GodotClass as its root). It is recommended to use if access to the current scene tree is required.
So, instead of suggested function fn get_singleton<T>(name: impl Into<GodotString>) -> Result<Gd<T>, Error>
it should be actually fn get_autoload
(though some things should be figured out first, eg. #566 ), possibly a method on Gd<T: Inherits<Node>>
, unless some global workaround could be find.
Something like:
Right now you have to do something like:
That's not very intuitive.
If we implement
get_singleton
like this, it would need access to the scene tree. I'm not sure if there is a better way.