godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Add Array/Dictionary casting operators to GodotSharp #403

Open Spartan322 opened 4 years ago

Spartan322 commented 4 years ago

Describe the problem or limitation you are having in your project: GodotSharp's non-generic Array and Dictionary classes don't implement any casting operators to native C# array, common System classes, or to the generic versions, (leaving a one way explicit conversion from the generic to the non-generic) making all conversions extremely verbose for little reason. This is especially annoying when it comes to using any method that requires a Godot.Collection.Array, as it requires new Godot.Collections.Array(new [] { /* stuff */}) (in some cases you can remove Godot.Collection. but when you include the System namespace you can't)

If implemented implicitly we could turn say:

Connect("pressed", this, nameof(OnPress), new Godot.Collections.Array { 1 });

into

Connect("pressed", this, nameof(OnPress), new[] { 1 });

which would be a lot easier to deal with for most folks I'd say.

Describe how this feature / enhancement will help you overcome this problem or limitation: Currently a lot of cases where casting should be eased are instead obscured or outright blocked by the lacking of casting operators, (in such latter cases requiring use of the constructor explicitly) generally implicit casting operators. Most particularly any method that uses Godot Arrays as inputs requires a somewhat verbose declaration, especially in the case of including the System namespace since System.Array exists. There are some cases as well where you could certainly need a cast from System Collections or native C# Arrays into Godot Collections which I say is rather verbose to type out.

Describe implementation detail for your proposal (in code), if possible:

Foremost we could add:

public static implicit operator Array(System.Array array) => new Array(array);

to Godot.Collections.Array, which would implicit allow any native C# Array to be made into a Godot Array implicitly, opening up the option above.

For ease of conversion we could also perform casts back:

public static implicit operator System.Array(Array array) 
{
    var result = new object[array.Count];
    array.CopyTo(array, 0);
    return result;
}

and we could even perform casts between the generic and non-generic version. (Although I am still unsure why the generic version doesn't just inherit from the non-generic version) In any case it would ease the conversion between the types.

If we simply add:

public static implicit operator Array<T>(Array array) => new Array<T>(array);

to Array we could also make the same conversion between the non-generic and generic versions easier.

And it already contains an explicit cast to the non-generic version however given the nature of System includes and the fact the cast doesn't actually make a difference in behavior (other then type conversion which we should be striving for type specifics in most cases anyway) I don't see why not change

https://github.com/godotengine/godot/blob/91b0be18dcc3ba3b1ecd35e8a7e416883776cf7b/modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs#L256-L259

to an implicit cast in that case.

These casts can also be applied to common System Collection types include List, Queue, and Stack as examples.

The same could be said for Dictionary, while it is generally less capable of removing verbosity, (specifically since there is no native dictionary object that obscures the type system in the System namespace) a cast could still be found useful, I suggest implicit casting, but like all of this I'll leave discussion and decision foremost to the Project Heads.

Any referred to beforehand, the most significant question I leave in case of implementation acceptance is whether it should be implicit or explicit, or for specific cases of what should and should not be implicit in. Once decided I'll readily submit a PR.

If this enhancement will not be used often, can it be worked around with a few lines of script?: No, its about reducing verbosity specifically and easing casting generally across the whole of GodotSharp given the prevalence of Godot Collection types which can broadly be associated with System.Collections.List and System.Collections.Dictionary and their generic versions.

Is there a reason why this should be core and not an add-on in the asset library?: Not possible and it is a very common typing issue I find working with GodotSharp.

neikeq commented 4 years ago

What we can do is add ToArray() methods like List has. Implicit operators should be avoided as much as possible. Explicit operators are not an option either because they obfuscate the fact that an entire array copy is being done.

Spartan322 commented 4 years ago

That's the point of the casting operators though. And I don't know how explicit operators obscure anything given it still requires an explicit cast, there is almost never a case where you use a casting operator method and it doesn't do anything but copy data from one class to another. (otherwise you wouldn't need the operator) And its not like we don't constantly have to perform the copying specifically when it comes native arrays. In all cases its desirable to copy and it wouldn't cost anything to put it into a Godot Array, especially since we already natively support obscuring that fact in export variables.

Even for most enumerables that's an acceptable tradeoff since everything in Godot only use the non-generic Godot collection types, (I still don't get why the generic version needs an explicit cast to the non-generic array either, its not like you can make a mistake with that) native arrays are gonna be copied regardless and unless you prefer using multiple lines to define an array, the only choice you have is the IEnumerable Array constructor which is frustrating to type, specifically when you include the System namespace.

Having a method only solves the problem for specific types, though having an extension method for IEnumerable is exceedingly more useful then having to type out the Array constructor constantly. But it doesn't fix the fact that every case of native arrays should be converted to Godot Arrays. And of course the fact that Array for a class name is already massively obscuring. (I mean even having Dictionary for a class name is kinda obscuring but luckily I haven't had to include both collection namespaces a lot yet so its not like Array)