Why limit to MemManual?

If #522 is the reason, should we try to fix that at some point? 🙂

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.

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.

Would Object still be automatically-managed (library destroys it on unload)

yep, that's implemented

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?

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.

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 🙂

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.

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.

Does that also mean they cannot be added/removed during hot reloads? If yes, maybe comment

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.

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:

Details

#[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());
    }
}

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.

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. 🤔

Could you create an issue for this? Maybe describe if it also has an impact outside singletons 🤔

For completeness, here is the issue: #1404

Maybe add "for now limited to the main thread" or so 🤔

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?

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.

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...

After some consideration – I think that #[class(singleton)] implying #[class(singleton, tool, base = Object)] wouldn't be out of the place if properly documented 🤔.

Yes, we should definitely document it 👍 maybe also in the place where base=... which currently says RefCounted is the default.

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()?

Huh, it is due to #[cfg(safeguards_strict)] {}.

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>()?

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.