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

Remove convert() utility method #66116

Closed
wants to merge 1 commit into from
Closed

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 19, 2022

This method simply calls the constructor of the target type. It might be useful e.g. if you want to pass the type as parameter to have some generic conversion function, but it's some weird edge case and it's unsafe. You can achieve the same using a series of ifs..

Closes #5523
Closes #65919

@KoBeWi KoBeWi added this to the 4.0 milestone Sep 19, 2022
@KoBeWi KoBeWi requested a review from a team as a code owner September 19, 2022 16:02
@MikeSchulze
Copy link

PLEASE don't remove the convert function, i need it too!
To implement this is very anoing and will explode in a unreadable function with tons of if/else cases

@fire
Copy link
Member

fire commented Oct 15, 2022

I see some opposition. What are the weird edge cases and why is it unsafe?

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 15, 2022

It is unsafe, because you can put any type (or just any integer) as the second argument, but the conversion might be unsupported. The function doesn't provide type hint and may result in an error.

As for edge-cases, I referred to the use-case of this function. It's just very niche and if you need it, you can use match or a series of if else, which also gives you more control over what happens under the hood.

That said, I'm ok if the function is kept, but then we need to properly fix the linked issues somehow.

@fire
Copy link
Member

fire commented Oct 15, 2022

properly fix the linked issues

I have a feeling if we the contributor of the engine can't do write a correct convert function correctly in the engine. We can't expect the user-developers of Godot Engine to do it correctly either.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 15, 2022

Eh but GDScript implementation looks like this

func convert(variant, type):
	match type:
		TYPE_INT:
			return variant as int
		TYPE_FLOAT
			return variant as float
	etc.

The C++ implementation just uses some convoluted built-in logic, so it's more difficult to get right.

@Mickeon
Copy link
Contributor

Mickeon commented Oct 18, 2022

The fact that convert() does a strict conversion strikes as odd to me. The "in the best way possible" of the documentation feels misleading if, for example, a Color can't be converted into a PackedFloat32Array of size 4, as outstandingly situational as that may be. I wonder if it would be reasonable to upgrade this method instead of outright removing it. It can't even handle String conversion anymore.

@MikeSchulze
Copy link

I see it the same way, having a convert method makes perfect sense.
For unsupported conversion it can simply return an error.

@aaronfranke
Copy link
Member

aaronfranke commented Dec 15, 2022

For my uses, I want a loose conversion. I opened #70080 to accomplish this. I think that it would be a good idea to remove this old utility method and replace it with the superior method I propose in #70080. It uses the same logic as the internal Variant operators, so it is able to easily convert many things to many other things.

@reduz
Copy link
Member

reduz commented Jan 21, 2023

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.

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