Godot-proposals: Make @GDScript scope cleaner

[dalexeev]

Describe the project you are working on:

A game

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

I noticed that some of the global functions in GDScript duplicate/overlap with functionality of other built-in classes.

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

My proposal describes each individual case and possible solutions. Perhaps some cases will remain unchanged, but I will be glad if GDScript becomes a little cleaner.

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

1. to_json(), validate_json() and parse_json()

There is the JSON singleton that does the same.

var arr = [1, 2, 3]
print(to_json(arr))

var json = "[1, 2, 3]"
var err = validate_json(json)
if err:
    print("Error: ", err)
else:
    print(parse_json(json))

var arr = [1, 2, 3]
print(JSON.print(arr))

var json = "[1, 2, 3]"
var res = JSON.parse(json)
if res.error:
    print("Error: ", res.error_string)
else:
    print(res.result)

As we can see, the number of lines has not changed, the code has not become more complicated or much longer. Moreover, the JSON singleton and the JSONParseResult class provide several additional capabilities that GDScript functions do not.

_I propose remove the @GDScript.to_json(), @GDScript.validate_json() and @GDScript.parse_json() functions._

2. randomize(), seed() and rand_seed()

It seems to me that having control over the global RNG seed is a bad idea. The global RNG has to be randomized implicitly, as in many other scripting programming languages. Thus, @GDScript.rand*() functions will use a different seed each project run. For deterministic sequences of pseudo-random numbers, use the RandomNumberGenerator class.

If someone uses the @GDScript.seed() function - please respond and describe why you need it.

_I propose remove the @GDScript.randomize(), @GDScript.seed() and @GDScript.rand_seed() functions, and also make it so that the global RNG is randomized implicitly when the project starts._

3. str2var(), bytes2var(), dict2inst() and var2str(), var2bytes(), inst2dict()

In my opinion, these functions are relatively rarely used, they are not GDScript-specific, and therefore there is nothing wrong with moving them into the Marshalls singleton, which already contains similar functions:

By the way, the Marshalls singleton lacks the string_to_hex() and hex_to_string() functions, now this needs to be done through the intermediate PoolByteArray, which is not very convenient.

_I propose move @GDScript.{str2var,bytes2var,dict2inst,var2str,var2bytes,inst2dict}() functions to Marshalls singleton with consistent names._

4. Color8() and ColorN()

Perhaps these functions should be part of the Color class.

This is especially true of the ColorN function, because N still gives me associations with a number, not a name. Maybe, the Color(from: String) constructor should recognize not only hex colors, but also named colors (for example, Color('red')).

As for Color8, maybe we need a static method called Color.from_rgb8(), similar to Color.from_hsv()?

_I'm not sure, but I think we should discuss this?_

5. load()

See https://github.com/godotengine/godot-proposals/issues/263#issuecomment-560018422

@GDScript.load() and ResourceLoader.load() are the same. However, I think we shouldn't change this, because load() is used quite often and is a pair for preload(), which is not present in ResourceLoader.

_I'm opposed, but I must mention this for the sake of completeness._

6. db2linear() and linear2db()

_These are very specific functions, maybe they should be moved to the AudioServer singleton?_

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

No, this is part of the GDScript language.

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

Yes, this is part of the GDScript language. :smiley:

[aaronfranke]

For Color8/ColorN, see the second paragraph of this comment.

When it comes to Color.RED vs Colors.RED, the former isn't terrible, but IMO with the sheer amount of constants it makes sense to separate them. In C# there is also the concern of Color.Red etc obscuring unrelated methods like Color.Color8, and in GDScript these are just defined as global methods, but perhaps they should be moved to Color in GDScript too (and if so, that's a strong case for Colors.RED).

[Xrayez]

My criteria is the following: if functions are not essential to facilitate the process of quick prototyping and doesn't cater to most use cases, then those kind of functions can be removed.

I propose remove the @GDScript.to_json(), @GDScript.validate_json() and @GDScript.parse_json() functions.

Yeah, because you'd still be able to look for "json" and stumble upon JSON singleton immediately upon searching in the docs, people will be able to figure out the rest, these are not frequently used (for prototyping purposes at least).

There are other classes like XMLParser (not a singleton, btw). There are even modules which allow you to treat JSON as resources in a project: https://github.com/godot-extended-libraries/json.

I propose remove the @GDScript.randomize(), @GDScript.seed() and @GDScript.rand_seed() functions, and also make it so that the global RNG is randomized implicitly when the project starts.

I guess makes sense... But retaining @GDScript.randomize() would still be necessary if you do need to have deterministic outcome at the prototyping stages, and use @GDScript.randomize() explicitly if you truly need randomness "in production". See below.

If someone uses the @GDScript.seed() function - please respond and describe why you need it.

There's no absolute need for this to be at global scope, but last time I used this is to setup deterministic sequence of random test data for stress testing and to find out what kind of seed is needed to fail the test. But if you look at how people use this feature in C++, I think it's still preferable to use RandomNumberGenerator class: https://github.com/godotengine/godot/pull/42315/files#diff-d30431255b513ad5f4e905f59ad4237aR440-R442:

TEST_CASE("[Stress][CommandQueue] Stress test command queue") {
    const char *COMMAND_QUEUE_SETTING = "memory/limits/command_queue/multithreading_queue_size_kb";
    ProjectSettings::get_singleton()->set_setting(COMMAND_QUEUE_SETTING, 1);
    SharedThreadState sts;
    sts.init_threads();

    RandomNumberGenerator rng;

    rng.set_seed(1837267);

So, likely the same can be said for GDScript tests to achieve locality and reproducibility of testing.

I propose move @GDScript.{str2var,bytes2var,dict2inst,var2str,var2bytes,inst2dict}() functions to Marshalls singleton with consistent names.

they are not GDScript-specific

Surprise, inst2dict and dict2inst are specific to GDScript: https://github.com/godotengine/godot/issues/30077#issuecomment-549503883.

For the rest, I guess makes sense, because var2str and str2var and others would be also accessible to languages such as C# as far as I know, and not restricted to GDScript functions (these methods expose the functionality of VariantWriter and VariantParser C++ classes, used to serialize ConfigFiles, for instance).

4. Color8() and ColorN()

I'm not sure, but I think we should discuss this?

I'm not sure either, I'd prefer these functions to be moved to Color class indeed.

5. load()

Retain, because I've already written some docs for this: godotengine/godot#42439. 😄

6. db2linear() and linear2db()
_These are very specific functions, maybe they should be moved to the AudioServer singleton?_

I've never used those myself, but perhaps those are frequently used functions when you do need to deal with audio, I presume most people have not yet reached that point to care about this.

[dalexeev]

Surprise, inst2dict and dict2inst _are_ specific to GDScript

I don't understand why inst2dict/dict2inst is required at all.

tool
extends EditorScript

var a = 123
var b = "abc"

func _run():
    print(var2str(self))
    print(var2str(inst2dict(self)))

Object(EditorScript,"script":Resource( "res://addons/tools/_run.gd"),"a":123,"b":"abc")

{
"@path": "res://addons/tools/_run.gd",
"@subpath": NodePath(""),
"a": 123,
"b": "abc"
}

The only difference is that it works slightly better with inner classes.

[Xrayez]

Yeah it does work better with inner classes:

extends Node2D

class CustomNode extends Node:
    var abc = "abc"

func _ready():
    test_serialize_inst2dict_dict2inst()
    test_serialize_var2str_str2var() # This fails.

func test_serialize_var2str_str2var():
    var n = CustomNode.new()
    print(n)
    var inst_str = var2str(n)
    print(inst_str)
    n.free()
    var inst = str2var(inst_str)
    print(inst)
    assert(inst.get_script() != null)
    assert(inst.abc == "abc")

func test_serialize_inst2dict_dict2inst():
    var n = CustomNode.new()
    print(n)
    var inst_dict = inst2dict(n)
    print(inst_dict)
    n.free()
    var inst = dict2inst(inst_dict)
    print(inst)
    assert(inst.get_script() != null)
    assert(inst.abc == "abc")

inst2dict actually converts the instance to Dictionary, and not a String. And the dictionary can be then converted to JSON more easily than a String.

Also prevents serializing built-in properties like pause_mode which should likely not be serialized for gameplay "save game" needs.

I think the inst2dict should be read as "script instance to dictionary" rather than "object to dictionary", for which var2str works better.

There are other enhancements which could further improve existing inst2dict/dict2inst to be more useful than var2str/str2var: godotengine/godot#33348, godotengine/godot#33137, godotengine/godot#33360, alas those enhancements didn't make it to 3.2 back in the days when I worked on this. 😛

[Xrayez]

Idea: create a global instance of RandomNumberGenerator as a singleton (such as RNG, or Random for short), and remove all random methods from GDScript. This way, all random functions will be available statically in scripts, like Random.randf_range(), and opens the door for other languages which can bind to the Random global class. With an increasing number of functions being implemented (godotengine/godot#43103), it's better to have a dedicated singleton for this to provide all RNG functionality, without polluting the GDScript scope in any way.

[Xrayez]

The global RNG has to be randomized implicitly, as in many other scripting programming languages.

You may be right: godotengine/godot#43128. 😃

[akien-mga]

1. to_json(), validate_json() and parse_json()

Agreed.

2. randomize(), seed() and rand_seed()

rand_seed() can definitely be removed as it's confusing and not really useful.

The rest kind of depends on the outcome of #1774 and/or #1741.

3. str2var(), bytes2var(), dict2inst() and var2str(), var2bytes(), inst2dict()

_I propose move @GDScript.{str2var,bytes2var,dict2inst,var2str,var2bytes,inst2dict}() functions to Marshalls singleton with consistent names._

Makes sense I guess. Then there would be a need for better documentation of how to use the Marshalls singleton as I think many users might not be aware of it (nor even understand what "Marshalls" is aside from some sort of 19th century US police officers).

4. Color8() and ColorN()

Perhaps these functions should be part of the Color class.

This is especially true of the ColorN function, because N still gives me associations with a number, not a name. Maybe, the Color(from: String) constructor should recognize not only hex colors, but also named colors (for example, Color('red')).

As for Color8, maybe we need a static method called Color.from_rgb8(), similar to Color.from_hsv()?

The main issue is that GDScript until now does not support static functions. You can't use Color.from_hsv(), it has to be var col = Color(); col.from_hsv(), which is a hassle. ColorN and Color8 were added as workarounds for this limitation.

I agree that they're bad though and finding a way to improve this would be good.

We did talk about renaming ColorN and adding support for both hex colors (when starting with #) and named colors (otherwise) in the Color(from: String) constructor.

5. load()

No strong opinion here.

6. db2linear() and linear2db()

These are like deg2rad() and rad2deg(), they're mathematical conversion methods. Moving them to AudioServer wouldn't really make sense IMO, and there's no real location where you could put the angle conversion methods.

Unless we introduce a Math singleton and move all math stuff there.

[vnen]

Now many of the functions are part of core, so they will be used in GDScript as is (to not complicated things further). Those are mostly math and utility functions, very useful for GDScript anyway.

Still, some extra functions remain so since I'm working on it I'll take the time to clean those up. I'll comment on this here.

1. to_json(), validate_json() and parse_json()

[ ... ]

I propose remove the @GDScript.to_json(), @GDScript.validate_json() and @GDScript.parse_json() functions.

I agree with this. I don't see a reason to keep those since the JSON singleton is available. If need be, we can add more straightforward functions to JSON, when you don't need to validate for instance.

2. randomize(), seed() and rand_seed()

_I propose remove the @GDScript.randomize(), @GDScript.seed() and @GDScript.rand_seed() functions, and also make it so that the global RNG is randomized implicitly when the project starts._

Those are now in core. I bvelieve randomize() is still useful for extra randomness. You can still use those for determinism, but since you'll have to manage the seed, using the RNG class is better.

There is a discussion to move all this to a Random singleton, so we can use that instead.

3. str2var(), bytes2var(), dict2inst() and var2str(), var2bytes(), inst2dict()

[ ... ]

_I propose move @GDScript.{str2var,bytes2var,dict2inst,var2str,var2bytes,inst2dict}() functions to Marshalls singleton with consistent names._

Those are now in core too, and can be used by other languages. May also be removed from the global scope, but it's a bit out of my reach now.

Except the dict ones, those will have to be kept in GDScript if we're really keeping them.

4. Color8() and ColorN()

[ ... ]

_I'm not sure, but I think we should discuss this?_

I find ColorN a bit pointless given you can use the static constants (e.g. Color.RED instead of ColorN("red")). I think the Color(String) constructor has changed to also accept the color name (instead of only a hex code), so there's even less need. Now, Color8 is pretty much the only way to create a Color using 0-255 range values, so it should be kept as convenience.

5. load()

[ ... ]

_I'm opposed, but I must mention this for the sake of completeness._

This makes sense as a function still. It's used a lot so the convenience is of great help.

6. db2linear() and linear2db()

Those are also in core now. I guess they have some use in audio systems (like when making a volume slider). Not sure where else to put though, as the server might not be the best fit.

I don't understand why inst2dict/dict2inst is required at all.

I believe the most use is to convert instances to JSON, since making that from a dictionary is easier. But since it's not 1:1 I'm not sure if it's the best way to serialize things (it's just the easiest one).

[Xrayez]

The main issue is that GDScript until now does not support static functions. You can't use Color.from_hsv(), it has to be var col = Color(); col.from_hsv(), which is a hassle. ColorN and Color8 were added as workarounds for this limitation.

This actually works last time I checked (Godot 3.2.4):

extends Node2D

func _draw():
    randomize()
    var c = Color.from_hsv(randf(), randf(), randf())
    draw_circle(Vector2(100, 100), 64, c)

color_from_hsv.zip

So I guess functions of built-in Variant-compatible classes are indeed available statically?

[dalexeev]

We did talk about renaming ColorN and adding support for both hex colors (when starting with #) and named colors (otherwise) in the Color(from: String) constructor.

See also my PR godotengine/godot#35505. On the last rebase, I added the Color::from_string constructor.

6. db2linear() and linear2db()

These are like deg2rad() and rad2deg(), they're mathematical conversion methods. Moving them to AudioServer wouldn't really make sense IMO

The fact is that in 99% of cases these functions are used only together with these methods of the AudioServer class:

void set_bus_volume_db(bus_idx: int, volume_db: float)
float get_bus_volume_db(bus_idx: int) const

In my opinion, it makes sense to add the methods:

void set_bus_volume_linear(bus_idx: int, volume_linear: float)
float get_bus_volume_linear(bus_idx: int) const