Skip to content
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

Move the "Singleton" concept out of the autoload one #5832

Open
groud opened this issue Nov 23, 2022 · 17 comments
Open

Move the "Singleton" concept out of the autoload one #5832

groud opened this issue Nov 23, 2022 · 17 comments

Comments

@groud
Copy link
Member

groud commented Nov 23, 2022

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

I common feature that has been asked for a while is the possibility to register a node as globally accessible from anywhere. This is quite similar to what autoloaded nodes marked as "global variable" provide, but it means that you cannot register your own node, at runtime, as a singleton.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The idea would be to split the the autoload tab into an autoload tab, and a singleton one. That means user would be able to register their own objects/nodes as singletons.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

test_autoloads_sigletons_interface

The autoload tab would look a bit like the current one. I just added and "Enabled" tickbox at the left, so that an autoload can be disabled without the removing the line. I also changed the "Global variable" column title to "Use as singleton".

When a node is marked a "used as a singleton", it is automatically moved as a non-editable entry in the Singleton tab, with its autoload name used as the singleton name. It's type is also set according to the script/scene you used to create the singleton.

Then, on the Singleton tab, you would be able to add / remove other singletons to the list (provided they have unique names of course) and define a optional type for it. This type will mainly be used for autocompletion.

If you try to add a singleton with an already used name, either by adding it manually, or by setting "use as singleton" on an autoload, the operation would fail with an error message.

When the game starts, singletons are not instantiated. Aside from autoloaded nodes, for which the singleton would be automatically set by the engine, users will have to register an object of the correct type as a singleton using something like SceneTree->register_singleton(&"MySingleton", my_object_to_register_as_singleton) (it could be added to any global-scope API, not necessarily SceneTree.). Then, any code running MySingleton.do_something() would call do_something() on the registered object.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Yes.
It can be worked around with an autoload whose properties are set to handle the list of singletons, that's what most users do. But it is less convenient for something that could be used often.

Is there a reason why this should be core and not an add-on in the asset library?

This is core.

Edit: Related:
#4854
#4831
#165

@theraot
Copy link

theraot commented Nov 23, 2022

I bring to your attention the add_autoload_singleton and remove_autoload_singleton API in EditorPlugin. Which - as far as I can tell - do not allow to register something with the global variable checkbox off. So I believe should be updated with this proposal.

@nonunknown
Copy link

I'm not sure if I got what you want to be implemented, because atm singletons just work fine, do you have some end-user cases?

@groud
Copy link
Member Author

groud commented Nov 23, 2022

I'm not sure if I got what you want to be implemented, because atm singletons just work fine, do you have some end-user cases?

Autoloads are, as their name imply, simply nodes that are instantiated as a child of the root node automatically. In the project settings, an autoload can be marked as a "global variable", which makes it available as a singleton. However, there are situations where you would want a singleton that is either not a node, or not automatically loaded.

As an example, you may have a single 3D camera system in your game (if you are doing a strategy game for example), and would like to access it from anywhere as it's more convenient. One solution would be to create a custom autoload called Global with "global variable" enabled, and then add a camera property to it. Once the property is set, you could then access your node from anywhere using Global.camera. Similarly, if you want some player data to be accessible from anywhere, you would also have to add that field to the Global node too. This solution works, but it means that you need to create some sort of a messy carryall node (here Global) to store everything.

In the end, it seems like this Global node is simply a workaround for Godot not having proper support for singletons, and that such a common workaround should probably be built-in instead. So this proposal is trying to remove the need for this workaround, and cleaning up things a bit.

@nonunknown
Copy link

makes sense for me now! thanks for the explanation!!!

@RedMser
Copy link

RedMser commented Nov 23, 2022

Just FYI, Engine.register_singleton already exists, and I think this feature should be implemented with that (though it does not add the global variable to gdscript. I assume this is currently hardcoded to work only for class_name and autoloads).

Not sure I like the UI split into two tabs that kind of sync each other. Why not rename the autoload project settings tab to "Singletons" (open for other ideas) and just allow selecting any script inheriting Object, or a scene? Then for scenes or scripts inheriting Node, it would be added to the tree as autoload like before, and any object would just be a singleton.

I think I misunderstood. If singletons are instantiated at runtime, then I suppose splitting does make more sense. But in that case, I don't see why the list would also show the autoloads with global variables again, if those are the only singletons that automagically get created on game start. As it stands it's a bit confusing to introduce another "global variable / instance" system, and it would be better to improve the existing ones and integrate into those. Sorry if I misunderstood some of the proposal still!

@groud
Copy link
Member Author

groud commented Nov 23, 2022

I think I misunderstood. If singletons are instantiated at runtime, then I suppose splitting does make more sense. But in that case, I don't see why the list would also show the autoloads with global variables again, if those are the only singletons that automagically get created on game start.

The list would show them, but they would not be editable. This is mainly to make it clear which singletons exist, even if they are automatically created. As you cannot create two singletons with the same name, I think it is pretty important to show all existing singletons in the same list.

As it stands it's a bit confusing to introduce another "global variable / instance" system, and it would be better to improve the existing ones and integrate into those. Sorry if I misunderstood some of the proposal still!

The problem is the following. Right now you can have autoloaded nodes that can be exposed as singletons or not. The proposal aims to allow for singletons not to be autoloads, and to not necessarily be nodes (they could be any type of object).
So the question is, considering you may have singletons that are not autoloads, and autoloads that may not be singletons, how do you group all of this in the same "category". I was not able to find a correct name to have all of those in the same list, so I think it is less confusing if we split it into two, as the proposal suggests, while still having some link between the two by displaying autoloads registered as singletons in the singleton list.

@TheDuriel
Copy link

TheDuriel commented Nov 24, 2022

There seem to be two things going on here:

  1. The desire for Singletons. (That are not Nodes.)

  2. The desire for Singletons accessible under a Global Name.

The proposal aims to allow for singletons not to be autoloads, and to not necessarily be nodes (they could be any type of object).

  1. Is already possible as the Engine treats Resources the same way we users want to treat a Singleton. Only one instance of a specific resource can ever exist. The first load() call instances it, all successive load() calls will retrieve this same instance.

While load() could be considered clunky, it does force explicit access. You are not polluting the global name space with Resources.

There is one drawback: You risk accidentally losing the instance if all objects that have load()ed your Resource happen to be freed. (Reference count hits 0. Realistically you could prevent this by calling reference() within the Singleton Resource on itself, and then dereferencing it when you quit the application. But we can file that under 'advanced user' knowledge.)

  1. Is an interesting notion. That I would like to have attached to Resources specifically. Similar to how we have a list of Nodes that are always instanced, how about having a list of Resources that are always loaded? And then additionally accessible through a name if the user enables that option. The same as Autoloads are currently handled.

This way, it seems, there's no fundamentally new feature / rework of autoloads required either.

I am personally against allowing Nodes to be Singletons. It's bound to invite bad practices and complications.

@theraot
Copy link

theraot commented Nov 24, 2022

@TheDuriel As you correctly point out, Resource is a reference counted type (it extends Resource). Thus, if you load a Resource, drop all references, and then load it again, you might not get the same the instance (there is a cache, so you might get the same instance, but it is not idea relying on it). On the other hand if you preload the Resource, you don't have this problem.

@TheDuriel
Copy link

preloading is not safe. If you discard every object which has preloaded a resource, you also discard that resource. The only safe way to ensure it is kept is to manually tick up the ref counter.

Since you are mentioning the Reference typo though. One could extend the idea in 2. of my post to mean Reference types instead of Resource types. But, I think Resource is universally more useful than plain references, and less trouble to implement.

@theraot
Copy link

theraot commented Nov 24, 2022

If you discard every object which has preloaded a resource, you also discard that resource

You would also have to remove it from the ResourcePreloader.

@mberdev
Copy link

mberdev commented Jan 14, 2024

YES please oh god YES, I wholeheartedly support this proposal.

For two reasons:

1) this overlap in concepts is just not right as a matter of principle. You don't just "steal" the word "singleton" (which means the same thing in every single language) and suddenly give it a different meaning in Godot.
I know that the documentation tries to discard it by saying "an autoload is not necessarily a singleton", but what good does it do if everyone uses autoloads as singletons anyways? It just makes it worse.

2) in practice it makes it hard to Google questions related to programming patterns in Godot.
At the beginning I had a very hard time googling things such as "how do you implement a singleton" (among the other gazillion things I had to Google to get up and running with Godot) when the answers were always polluted by unwanted things — in this case, autoloads.
Worse: those answers were written by godot practical users who understand how to use singletons but do not understand what they are (again: because Godot stole the word) and genuinely don't see the problem . Or even if they do, got used to it and don't want to change it (like the second reply in this thread).

More generally, Godot needs to stop trying so hard to be different from other languages. If the concept exists somewhere else then take it as-is and use the same keywords (looking at you, GDscript pretending to be python but having weird keywords like class_name and extends and func) (looking at you, signals).
Then if you want to add a dimension to a standard concept (e.g. add autoloads next to singletons) then create a clear separation, both in concepts and in syntax.

@popcar2
Copy link

popcar2 commented Feb 14, 2024

I can sort of understand why people would want this but I'm not a fan of the proposal. Splitting autoloads into two (effectively "Autoloads" and "Singletons") will make the system way more complicated than it has to be. I can already imagine the questions confusing the two.

If you need autoload scripts, is there any real downside to having a Node hold the script and use that as an autoload?

If you need to have global variables and resources, why not make an autoload hold variables and data and access it from there?

I get that some people dislike that pattern but making a whole separate tab for singletons seems a bit rough. I'm a bigger fan of #4854 which suggests making autoloads easier to manage at runtime, rather than creating a whole separate system that almost does the same thing. I've always thought the autoload system reinforces the node structure of Godot rather than have random data floating around as singletons.

@groud
Copy link
Member Author

groud commented Feb 14, 2024

I can sort of understand why people would want this but I'm not a fan of the proposal. Splitting autoloads into two (effectively "Autoloads" and "Singletons") will make the system way more complicated than it has to be. I can already imagine the questions confusing the two.

To avoid some confusion, in my PR, I renamed is to "global variables". I believe this is indeed more accurate. So in general, you would have a tab for "nodes automatically loaded at startup" and another for "things that are globally accessible". Those are in fact quite different concept IMO.

Also, the fact that autoloads would be configurable as globally accessible would just become a nicety. As, technically, you could also assign the global variable value in the _init() function of the autoloaded node too.

If you need to have global variables and resources, why not make an autoload hold variables and data and access it from there?

It is just less practical to have to use something like Globals.main_camera. than MainCamera.. But, yeah, that's the most common workaround users use otherwise.

@mberdev
Copy link

mberdev commented Feb 14, 2024

The worry of breaking tutorials is legitimate, however if the name "Autoload" is dropped altogether in favour of two things mentionned by @groud ("Singletons" and the other one, whatever it gets named) then yes it's not ideal but at least there's no possible confusion.

@groud
Copy link
Member Author

groud commented Feb 19, 2024

I do understand why some people want that, but I'm against it, as the confusion it will cause on novices, by breaking every single tutorial, to be too much for the little QOL that it will bring. If it was up to me, I would leave to 5.x or something.

I am not sure why would those be problems. The proposal is clear that it does not change how autoloads work right now, so there's no problem possible with existing tutorials.

If you decide the move forward, please, treat it with the care that it deserves, by giving people a ample heads-up and making it abundantly clear on the release notes.

The proposal would not break compatibility in any way. But sure, I guess it will be in the release notes anyway.

The worry of breaking tutorials is legitimate, however if the name "Autoload" is dropped altogether

I didn't stated anywhere I would drop autoloads. On my mockups they are still visible as a dedicated tab.

@dalexeev
Copy link
Member

This approach was proposed as an alternative to static variables (see godotengine/godot#6840 (comment)), but static variables are now implemented.

What happens if you delete/rename an autoload, will the linked global (with the same name) change? I find it confusing that they are in different tabs, it's not obvious and inconvenient when adding/editing an autoload/global. I would prefer to combine this in one tab:

Also, we discussed on the #gdscript channel whether we would like to add full-fledged globals: arbitrary variables (of any type, not just singleton-like), functions, constants, etc. to the GDScript global scope. See also #4740.

@groud
Copy link
Member Author

groud commented Feb 19, 2024

What happens if you delete/rename an autoload, will the linked global (with the same name) change?

I'd say yes. The idea would simply have the "global variables" tab just list the autoloads registered as globals in the other panel. Technically, this would only help listing all your global variables in one single place.

I find it confusing that they are in different tabs, it's not obvious and inconvenient when adding/editing an autoload/global. I would prefer to combine this in one tab:

I don't mind much if we only have this implemented for nodes. But for other types of global variables it would probably be a bit annoying to have the management of autoloaded nodes in the same place as the global variables list. So well, as it was discussed indeed, it might be something to keep in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants