godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 93 forks source link

Instantiate `RandomNumberGenerator` at global scope as `Random` singleton, or the functionality behind it #1741

Open Xrayez opened 3 years ago

Xrayez commented 3 years ago

Describe the project you are working on:

Goost - Godot Engine extension

Describe the problem or limitation you are having in your project:

GDScript doesn't provide a full set of methods of RandomNumberGenerator, such as:

See also #1590, the global scope currently pollutes the global namespace unnecessarily with general names such as seed or randomize.

See also https://github.com/godotengine/godot/pull/22314#issuecomment-717872712.

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

I propose that a global instance of RandomNumberGenerator has to be created to be available globally via script.

Likely helps to properly scope random-based methods/implementations, like with #1654.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

This is quite possible to do, see goostengine/goost#29 as Random singleton, extending RandomNumberGenerator:

Random.randomize()
Random.seed = hash("Godot")
var i = Random.randi() % 100
var f = Random.randf_range(-1.0, 1.0)
var v = Random.randv_circle()

What I propose is similar to Unity's Random API, which has some static methods in it.

Of course, this can be made alternative to GDScript built-in randomization methods, but I'd expect that we add more useful stuff, like Color randomization, which is likely better added to RandomNumberGenerator or similar.

One of the limitations I've stumbled upon is that it may be a bit problematic to create a local instance of RandomNumberGenerator, because if you register the same class as a singleton, the .new() operator won't be available for such as class, because the name of the instance shadows the name of the class in GDScript. Instead, I've just added new_instance() to circumvent this, but technically ClassDB.instance("RandomNumberGenerator") can also be used.

Registering the class as an Engine singleton in C++ under a different name seems to solve it, but then the class is not highlighted as seen in godotengine/godot#37319, and this may create further problems with C# bindings, as seen in godotengine/godot#37922.

Perhaps making methods in RandomNumberGenerator static could help this as well, as proposed in #1101 similarly, but that likely doesn't apply because we actually need an instance of the class (RandomPCG is the core behind RNG in Godot).

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

Sure, just instantiate a local instance of RandomNumberGenerator every time you want to generate random data with methods not available in GDScript.

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

It's about accessibility, consistency, and the ease of use of existing randomization features in Godot. I'd prefer that we implement all future randomization features under RandomNumberGenerator class (or just Random as implemented in goostengine/goost#29), preventing Godot from explosion of the core API in existing classes with randomization methods. πŸ’₯🐱

akien-mga commented 3 years ago

I'm OK with adding a singleton instance of RandomNumberGenerator and removing the GDScript rand* methods. IIRC that was something we discussed back when RandomNumberGenerator was implemented but removing GDScript methods was not an option then (it is now with 4.0).

I'd prefer that we implement all future randomization features under RandomNumberGenerator class (or just Random as implemented in goostengine/goost#29)

Since the question of naming things (one of the two hardest things in computer science) was brought up in https://github.com/godotengine/godot/pull/22314#issuecomment-717872712 (albeit the other way around, but let's settle it here as suggested on https://github.com/godotengine/godot/pull/22314):

Xrayez commented 3 years ago

Thanks for your feedback, Akien.

  • I wouldn't rename RandomNumberGenerator to Random as suggested in godotengine/godot#22314 (comment). Its explicit name is fine, and you only write it once when creating a new instance (var rng = RandomNumberGenerator.new()).

I'm not necessarily suggesting to rename the RandomNumberGenerator class and I'm not going to create a separate proposal for it. The way I start up a discussion is by providing a plethora of alternatives from which you can choose. πŸ™‚

But, I'd still prefer to rename RandomNumberGenerator to Random.

  • If this proposal is implemented, I'd give the singleton a shorter name like Random on the other hand. That solves the name conflicts with the class it actually implements, and that's shorter to type since it will be accessed by name (Random.randi()).

I totally agree with this. Yet the way I propose to implement it is to have the singleton and the class to be under the same name, because the way I see it, this could be implemented with static methods. But static methods support has to be first implemented in core for this to happen.

That said, there's no absolute need to create a separate class just to expose RandomNumberGenerator as a global instance (or the functionality behind it).

reduz commented 3 years ago

I am against this, I stated several times that I don't care about purism in Godot APIs.

The C++ side of Godot has this because its more practical, while GDScript has access to it because it just lacks it in the language. Other languages bound have their own random stuff and the only reason why you might want to use the Godot one is to have a local instance.

This is why the proposal form states specifically that you must be solving a real world problem, and this proposal does not.

Xrayez commented 3 years ago

@reduz I've re-read your response several times and I think there might be possible misunderstanding, because Akien agrees on the feature and you don't, and it makes me wonder why.

To clarify, I'm not proposing exposing RandomNumberGenerator as a class in Godot, because it's already added in godotengine/godot#22314. πŸ™‚

I propose to create a global instance of the existing class, to cover up the lack of various randomization methods currently accessible in GDScript, but yet available in RandomNumberGenerator, and that's the problem. For me, it means that I just tend to use RandomNumberGenerator nowadays, so GDScript methods become kind of pointless. The problem is that those methods are not available statically in RandomNumberGenerator, at least. The only possible way to make it available globally is via Engine singletons approach, which may or not be a good way to implement this proposal.

That said, we have a discrepancy between what GDScript currently provides, and RandomNumberGenerator methods. It's almost no different than having other singletons in Godot.

only reason why you might want to use the Godot one is to have a local instance.

Again, this is possible currently.

Xrayez commented 3 years ago

Things like #1654 could also be added to Random singleton.

GDScript has access to it because it just lacks it in the language

I'm perfectly fine with having randomization methods in GDScript functions and I'm not necessarily suggesting removing them, but it would be equally nice to have a complete randomization feature set to be available closer to global scope, and singletons are the most feasible way to achieve this currently.

Again, talking about convenience, which I think is the development philosophy behind Godot, but I'm not sure about it: #575.