Add a type conversion method to Variant Utility and expose to scripting

[aaronfranke]

This PR adds a global scope method to Variant Utility to perform safe type conversion using Variant's operators.

type_convert("Hi!", TYPE_INT) # Returns 0
type_convert("123", TYPE_INT) # Returns 123
type_convert(123.4, TYPE_INT) # Returns 123
type_convert(5, TYPE_VECTOR2) # Returns (0, 0)

Unlike casting methods such as int(), this method will never throw any errors as long as the type is valid. It's intended for usage in more dynamic situations where you want the code to continue if a cast is invalid, rather than stop.

Note: This functionality seems to also exist as a helper method in GDScript (and, previously, in VisualScript), but it's fairly different code. I don't want to touch that code without knowing exactly how it works for fear of breaking things, but we could likely replace that logic with this core method in the future. EDIT: Actually, the method in this PR is superior to the old method, because this method allows many more conversions. EDIT 2: Now this PR deprecates the old method.

[anvilfolk]

Looks simple enough - it's just calling the underlying conversion operators. I'm still confused as to the exact use case though. Right now I can't imagine trying to convert something, it failing silently, and the code just continuing on as if nothing had happened. What am I missing? :)

Also, if this makes it in, please also make a test so we can codify the expected behavior and identify any regressions in the future :)

[aaronfranke]

Good idea, I added several test cases.

I'm still confused as to the exact use case though. Right now I can't imagine trying to convert something, it failing silently, and the code just continuing on as if nothing had happened. What am I missing? :)

I have a high-level scripting system that users edit in-game. When users are editing the script, I want the app to show them errors, not Godot. When users are not editing the script, I want to continue execution. In neither case do I want the game to crash because the user did something wrong with the script.

Aside from that, the operators already exist inside of Variant and are used in a ton of places around the engine. I want access to those same operators in GDScript. This PR basically just exposes them.

real_t stretch_scale = GLOBAL_GET("display/window/stretch/scale");

The above code is from main.cpp. It loads a Variant from the project settings and converts to real_t. If the value it loaded was "hi", do we want the engine to crash? No, it's acceptable to ignore and continue. With this PR you will be able to do the same thing in GDScript using var myfloat: float = type_convert(myvariant, TYPE_FLOAT). Although in my case with the scripting system I am storing the converted value inside of Variant.

[anvilfolk]

I don't have a fundamental disagreement with having this in, but somehow I still find it strange! I don't have your project in front of me though :) Clearly there's a fair amount of support for it and the code looks good to me :)

real_t stretch_scale = GLOBAL_GET("display/window/stretch/scale");

I would personally really want this to either 1) throw an error or 2) crash! So I'd either change the code to:

if (GLOBAL_HAS("display/window/stretch/scale"))
  stretch_scale = GLOBAL_GET("display/window/stretch/scale");
else
  print("Warning")

or just make it programmatically impossible for the GET to happen before the setting is set. If there's supposed to be something there and there isn't, then I should know rather than it failing silently and code behaving erratically from then on?

But I think this is besides the point right now! :)

Do you think it's worth having the test you just made in GDScript as well? Us GDScript folks mostly just use those GDScript tests so if something changes behind the scenes we won't really be notified :)

[aaronfranke]

So I'd either change the code to:

I don't think you understand. The value is always defined using GLOBAL_DEF, so it won't ever be undefined. But it can be defined to a value that isn't a float, which the Variant operator would turn into a float.

Anyway, I am just using this as an example of a place where Variant's operators are used (I just commented out the operator float and operator double and found a place where it failed). They are used extensively throughout the engine (idk how much, many thousands of times, hundreds of thousands?), and I want access to these from GDScript.

Do you think it's worth having the test you just made in GDScript as well? Us GDScript folks mostly just use those GDScript tests so if something changes behind the scenes we won't really be notified :)

I don't know what the policy is on that, which tests are desired to be written in GDScript vs in C++ (or both?).

[anvilfolk]

Oh, I think I see. Sorry I am far too tired to really make sense of things at the moment :)

But then you'd maybe check whether you had the Variant's default value, or just continue with a "bad" value?

[RevoluPowered]

LGTM exposes existing functionality available to C++

[fire]

I've always thought of this operator as a type_cast, but as mentioned this was used in visual script previously and it was already in c++.

I wonder if we can encode this secondary meaning into the name.

Unlike casting methods such as int(), this method will never throw any errors as long as the type is valid. It's intended for usage in more dynamic situations where you want the code to continue if a cast is invalid, rather than stop.

Also think this is a good idea.

[akien-mga]

Note: This functionality seems to also exist as a helper method in GDScript (and, previously, in VisualScript), but it's fairly different code. I don't want to touch that code without knowing exactly how it works for fear of breaking things, but we could likely replace that logic with this core method in the future. EDIT: Actually, the method in this PR is superior to the old method, because this method allows many more conversions.

How do the two differ exactly?

If the one in this PR is really superior, I think we should also deprecate the GDScript one (too late now to outright drop it) and mention this one in its documentation.

It would also be good to add invalid/impossible conversions to the test suite to validate that they do what users would expect.

[aaronfranke]

@akien-mga For example:

func _ready():
	var apple = "apple"
	print(convert(apple, TYPE_OBJECT)) # Invalid type in GDScript utility function '<unknown function>'. Cannot convert argument 1 from String to Nil.
	print(convert("apple", TYPE_OBJECT)) # Parse Error: Invalid argument for "convert()" function: argument 1 should be "Variant" but is "String".
	print(type_convert("apple", TYPE_OBJECT)) # Prints <Object#null>

	var x = 5
	print(convert(x, TYPE_VECTOR2)) # Invalid call. Nonexistent GDScript utility function '<unknown function>'.
	print(convert(5, TYPE_VECTOR2)) # Parser Error: Invalid call for function "convert".
	print(type_convert(5, TYPE_VECTOR2)) # Prints (0, 0)

The convert utility method is not ideal in dynamic situations since it does not guarantee conversion. It's pretty much only useful for things which have a cast operator in GDScript, like converting numbers to/from String.

Also, I think what reduz thought convert was is exactly what this PR does, because the type_convert method in this PR ensures you get the type no matter what input is given on the left:

Also against removing this. Its not unsafe because it ensures you get the type no matter the input. If you want the safe version, you just use a cast.

[aaronfranke]

#78108 must be merged before this PR to avoid the linker warnings. Or, I guess I could remove the unit tests, but that would be very unfortunate - unit tests are great!

Interesting that the same code did not fail before, I guess we did not have those linker checks on CI before.

[aaronfranke]

Since inability to write unit tests was blocking this PR for over a month, I removed the unit tests from this PR. I have the unit tests saved on another branch and I can add them in later after this PR is merged. EDIT: Since #78108 was merged I added back in the unit tests, as a separate commit.

[fire]

Oh no, I wanted to review but this was conflicted.

[akien-mga]

Oh no, I wanted to review but this was conflicted.

For the record, a minor conflict shouldn't prevent reviewing. It just means it will need a rebase at some point but being ready to merge is not a prerequisite for being ready to have a code review or test built from the PR branch.

[fire]

I am ok with this, but I remembered that @reduz had some commentary about the convert function. It's hazy for me to remember.

Did not check the test suite works.

[aaronfranke]

See #70080 (comment) for why this proposed method is superior and matches what reduz thought convert was.

[reduz]

Looks good to me, see nitpick below.

Nitpick fixed.

[akien-mga]

Approved on reduz's behalf, his request for changes was just a nitpick.

[akien-mga]

Thanks!